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

Remove support for the scope parameter in the MessageHandler.on method #11110

Merged
merged 1 commit into from Sep 1, 2019

Conversation

Snuffleupagus
Copy link
Collaborator

At this point in time it's easy to convert the MessageHandler.on call-sites to use arrow functions, and thus let the JavaScript engine handle scopes for us, rather than having to manually keep references to the relevant scopes in MessageHandler.[1]
An additional benefit of this is that a couple of Function.prototype.call() instances can now be converted into "normal" function calls, which should be a tiny bit more efficient.

All in all, I don't see any compelling reason why it'd be necessary to keep supporting custom scopes in the MessageHandler implementation.


[1] In the event that a custom scope is ever needed, simply using bind on the handler function when calling MessageHandler.on ought to work as well.

…ethod

At this point in time it's easy to convert the `MessageHandler.on` call-sites to use arrow functions, and thus let the JavaScript engine handle scopes for us, rather than having to manually keep references to the relevant scopes in `MessageHandler`.[1]
An additional benefit of this is that a couple of `Function.prototype.call()` instances can now be converted into "normal" function calls, which should be a tiny bit more efficient.

All in all, I don't see any compelling reason why it'd be necessary to keep supporting custom `scope`s in the `MessageHandler` implementation.

---
[1] In the event that a custom scope is ever needed, simply using `bind` on the handler function when calling `MessageHandler.on` ought to work as well.
@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/574aa29f2d1fabb/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/41914fd4e4f27b1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/574aa29f2d1fabb/output.txt

Total script time: 2.60 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/41914fd4e4f27b1/output.txt

Total script time: 5.07 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/fccbeeea173f32d/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/94854afad9ab05d/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/94854afad9ab05d/output.txt

Total script time: 2.58 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/fccbeeea173f32d/output.txt

Total script time: 5.01 mins

  • Unit Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/9d88873832d30ff/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/473a5d70aa0ddc2/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/9d88873832d30ff/output.txt

Total script time: 17.65 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/473a5d70aa0ddc2/output.txt

Total script time: 25.90 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/b73d712774847f3/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/e83bce01bf15cab/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/e83bce01bf15cab/output.txt

Total script time: 2.59 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 1, 2019

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/b73d712774847f3/output.txt

Total script time: 5.03 mins

  • Unit Tests: Passed

@Snuffleupagus Snuffleupagus marked this pull request as ready for review September 1, 2019 09:39
@timvandermeij timvandermeij merged commit 10165c0 into mozilla:master Sep 1, 2019
@timvandermeij
Copy link
Contributor

Nice find! Thank you for simplifying this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants