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

Convert web/debugger.js to a *basic* module #14745

Merged
merged 1 commit into from
Apr 3, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

The various functionality in web/debugger.js is currently indirectly added to the global scope, since that's how var works when it's used outside of any functions/closures.
Given how this functionality is being accessed/used, not just in the viewer but also in the API and textLayer, simply converting the entire file to a module isn't trivial[1]. However, we can at least export the PDFBug-part properly and then import that dynamically in the viewer.
Also, to improve the code a little bit, we're now explicitly exporting the necessary functionality globally.

According to MDN, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#browser_compatibility, all the browsers that we now support have dynamic imports implementations.


[1] We could probably pass around references to the necessary functionality, but given how this is being used I'm just not sure it's worth the effort. Also, adding official support for these viewer-specific debugging tools in the API feels both unnecessary and unfortunate.

The various functionality in `web/debugger.js` is currently *indirectly* added to the global scope, since that's how `var` works when it's used outside of any functions/closures.
Given how this functionality is being accessed/used, not just in the viewer but also in the API and textLayer, simply converting the entire file to a module isn't trivial[1]. However, we can at least export the `PDFBug`-part properly and then `import` that dynamically in the viewer.
Also, to improve the code a little bit, we're now *explicitly* exporting the necessary functionality globally.

According to MDN, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#browser_compatibility, all the browsers that we now support have dynamic `imports` implementations.

---
[1] We could probably pass around references to the necessary functionality, but given how this is being used I'm just not sure it's worth the effort. Also, adding *official* support for these viewer-specific debugging tools in the API feels both unnecessary and unfortunate.
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Apr 3, 2022

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/e10e55cd5ee0a1b/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 3, 2022

From: Bot.io (Linux m4)


Success

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

Total script time: 2.57 mins

Published

@timvandermeij timvandermeij merged commit 3e50e57 into mozilla:master Apr 3, 2022
@timvandermeij
Copy link
Contributor

Nice refactoring; thanks!

@Snuffleupagus Snuffleupagus deleted the debugger-module branch April 3, 2022 13:11
@calixteman
Copy link
Contributor

calixteman commented Apr 15, 2022

This change is making a test failing in m-c:
https://treeherder.mozilla.org/logviewer?job_id=374645040&repo=autoland&lineNumber=2185
It's reproducible locally in m-c in running the command:
./mach mochitest --headless browser_parsable_script.js

@Snuffleupagus, could you have a look please ?

@Snuffleupagus
Copy link
Collaborator Author

Given the name of the failing test, it seems that you'll need to add an entry for the web/debugger.js file in this list:
https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/browser/base/content/test/static/browser_parsable_script.js#15

@calixteman
Copy link
Contributor

Cool it's an easy fix, thank you.

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.

None yet

4 participants