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

Output pdf.scripting.js as a JavaScript module (PR 17055 follow-up) #17080

Merged
merged 1 commit into from Oct 7, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Oct 6, 2023

To avoid problems with export statements in the QuickJS Javascript Engine, we can work-around that by explicitly exposing pdfjsScripting globally instead.

@calixteman
Copy link
Contributor

The sandbox is initialized with some code:

const code = [PDFJSDev.eval("PDF_SCRIPTING_JS_SOURCE")];

and the generated code (the value of PDFJSDev.eval("PDF_SCRIPTING_JS_SOURCE")) contains at the end:

export { __webpack_exports__initSandbox as initSandbox };

and it's likely why it isn't working correctly.
If try to export something on this quickjs demo: http://numcalc.com/ then we get a syntax error for the export keyword.
That said, I'm not sure to understand what it'd mean to export something here.

@Snuffleupagus
Copy link
Collaborator Author

If try to export something on this quickjs demo: http://numcalc.com/ then we get a syntax error for the export keyword.

I wonder if that demo is just outdated, since the "Introduction"-section at https://bellard.org/quickjs/ contains (emphasis mine):

QuickJS is a small and embeddable Javascript engine. It supports the ES2020 specification including modules, asynchronous generators, proxies and BigInt.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Oct 6, 2023

That said, I'm not sure to understand what it'd mean to export something here.

It appears that removing that, and manually exposing the necessary object globally, works; note the updated patch.

@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 2.50 mins

  • Unit Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

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

Total script time: 10.72 mins

  • Unit Tests: Passed

@Snuffleupagus Snuffleupagus force-pushed the scripting-module branch 4 times, most recently from d548419 to 808e63d Compare October 7, 2023 11:07
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last commit here looks good to me, with one optional comment. Thanks!

src/pdf.scripting.js Show resolved Hide resolved
To avoid problems with `export` statements in the QuickJS Javascript Engine, we can work-around that by *explicitly* exposing `pdfjsScripting` globally instead.
@Snuffleupagus Snuffleupagus marked this pull request as ready for review October 7, 2023 13:30
@Snuffleupagus
Copy link
Collaborator Author

/botio xfatest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_xfatest from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.193.163.58:8877/3c7a1b26b910079/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_xfatest from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.241.84.105:8877/7a1d688b5468e3b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/7a1d688b5468e3b/output.txt

Total script time: 10.52 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 2

Image differences available at: http://54.241.84.105:8877/7a1d688b5468e3b/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/3c7a1b26b910079/output.txt

Total script time: 19.94 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/3c7a1b26b910079/reftest-analyzer.html#web=eq.log

@Snuffleupagus Snuffleupagus merged commit e6c3257 into mozilla:master Oct 7, 2023
3 checks passed
@Snuffleupagus Snuffleupagus deleted the scripting-module branch October 7, 2023 14:09
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