Skip to content

Use the worker created in the child actor (bug 1966721) #19935

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

Merged
merged 1 commit into from
May 15, 2025

Conversation

calixteman
Copy link
Contributor

No description provided.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Using a global pdfjsWorker property seems problematic here, since we're using the existence of globalThis.pdfjsWorker as an indication that fake-workers are being used.

@calixteman
Copy link
Contributor Author

Using a global pdfjsWorker property seems problematic here, since we're using the existence of globalThis.pdfjsWorker as an indication that fake-workers are being used.

Correct. What do you think about pdfjsParserWorker ?

@Snuffleupagus
Copy link
Collaborator

Using a global pdfjsWorker property seems problematic here, since we're using the existence of globalThis.pdfjsWorker as an indication that fake-workers are being used.

Correct. What do you think about pdfjsParserWorker ?

Naming things is hard, but maybe pdfjsPreloadedWorker to try and explain "more" what this is?

Another thing that just occurred to me is that we already have the workerPort option, hence rather than "hacking" this into the API it might be possible to just do the following. (Which might also be a tiny bit more efficient, since it avoids some checking during worker-setup.)

diff --git a/web/app_options.js b/web/app_options.js
index 461336dc1..34615b8fc 100644
--- a/web/app_options.js
+++ b/web/app_options.js
@@ -502,7 +502,10 @@ const defaultOptions = {

   workerPort: {
     /** @type {Object} */
-    value: null,
+    value:
+      typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")
+        ? globalThis.pdfjsPreloadedWorker
+        : null,
     kind: OptionKind.WORKER,
   },
   workerSrc: {

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Nit: In the commit message (and PR title) there seems to be a missing word.
Use the worker created when the child actor has been {word missing here} (bug 1966721)

r=me, thank you.

@calixteman calixteman changed the title Use the worker created when the child actor has been (bug 1966721) Use the worker created in the child actor (bug 1966721) May 15, 2025
@calixteman calixteman merged commit f4a3e47 into mozilla:master May 15, 2025
7 checks passed
@calixteman calixteman deleted the bug1966721 branch May 15, 2025 19:21
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.

2 participants