Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline single-line event handlers in the web/app.js file #18527

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Jul 31, 2024

We have a fair number of (effectively) single-line event handlers in the web/app.js file, which leads to unnecessarily verbose code. These can, without affecting readability too much, be replaced either by:

  • Using bind for the simplest cases.
  • Using arrow-functions for the remaining ones.

Note that this is possible since we started removing event listeners with AbortSignal, which means that we no longer need to keep a reference to the event handler functions to be able to remove them.

Given that the old event handler functions use fairly long function names, and the way that they access PDFViewerApplication (given their scope), they impact the overall code-size unnecessarily.
Note: This patch reduces the size of the gulp mozcentral output by ~3.7 kilo-bytes, which isn't a lot but still cannot hurt.

@Snuffleupagus Snuffleupagus force-pushed the app-inline-short-listeners branch 2 times, most recently from 83b143a to 879d0bb Compare August 1, 2024 07:53
We have a fair number of (effectively) single-line event handlers in the  `web/app.js` file, which leads to unnecessarily verbose code. These can, without affecting readability too much, be replaced either by:
 - Using `bind` for the simplest cases.
 - Using arrow-functions for the remaining ones.

Note that this is possible since we started removing event listeners with `AbortSignal`, which means that we no longer need to keep a reference to the event handler functions to be able to remove them.

Given that the old event handler functions use fairly long function names, and the way that they access `PDFViewerApplication` (given their scope), they impact the overall code-size unnecessarily.
*Note:* This patch reduces the size of the `gulp mozcentral` output by `~3.7` kilo-bytes, which isn't a lot but still cannot hurt.
@mozilla mozilla deleted a comment from moz-tools-bot Aug 1, 2024
@mozilla mozilla deleted a comment from moz-tools-bot Aug 1, 2024
@mozilla mozilla deleted a comment from moz-tools-bot Aug 1, 2024
@mozilla mozilla deleted a comment from moz-tools-bot Aug 1, 2024
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/377aa1b13f4ff09/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/c0ee1d01c7b2f70/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/377aa1b13f4ff09/output.txt

Total script time: 8.46 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/c0ee1d01c7b2f70/output.txt

Total script time: 17.45 mins

  • Integration Tests: FAILED

@timvandermeij
Copy link
Contributor

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/e4a5c1f0315c219/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/e4a5c1f0315c219/output.txt

Total script time: 1.04 mins

Published

@timvandermeij timvandermeij merged commit b80e552 into mozilla:master Aug 2, 2024
6 checks passed
@timvandermeij
Copy link
Contributor

Looks much better and easier to manage like this; nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants