Split pdfjschildbootstrap.js to avoid sync IPC #8218

amccreight · 2017-03-30T21:39:06Z

See the commit messages for a more detailed explanation.

In terms of local testing, I only ran the linter but it doesn't seem like this should really affect the addon.

timvandermeij · 2017-03-30T21:43:01Z

extensions/firefox/content/pdfjschildbootstrap-enabled.js

+
+if (Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_CONTENT) {
+  // register various pdfjs factories that hook us into content loading.
+  PdfJs.ensureRegistration();


Did you mean ensureRegistered?

Oops! I guess I should test it a little more.

You can run gulp firefox to build the Firefox extension XPI locally, which should help with testing this.

timvandermeij · 2017-03-30T21:43:49Z

extensions/firefox/content/PdfJs.jsm

@@ -173,7 +173,7 @@ var PdfJs = {

  updateRegistration: function updateRegistration() {
    if (this.enabled) {
-      this._ensureRegistered();
+      this.ensureRegistered();


If this one is public, i.e., has no underscore, then I think it would be good to make _ensureUnregistered public as well, so remove the underscore there as well for consistency.

That sounds reasonable. I just left it private because I didn't actually need it, but it is a little weird to make it different.

timvandermeij · 2017-03-30T21:52:35Z

extensions/firefox/content/pdfjschildbootstrap-enabled.js

@@ -0,0 +1,31 @@
+/* Copyright 2014 Mozilla Foundation


How is this new file used at the moment? I don't see an include of it anywhere. Do you perhaps need to include it in build process like

pdf.js/gulpfile.js

Line 822 in cd5acf5

gulp.src(FIREFOX_CONTENT_DIR + 'pdfjschildbootstrap.js')

?

Yeah, like the pdfjschildbootstrap.js file, I think isn't actually used by the extension. It is a script called directly by Firefox code, in browser/components/nsBrowserGlue.js. But yeah I guess I need to do whatever packaging thing this is doing. Maybe that's how the in-tree version in Firefox gets updated.

There is difference in few files for firefox extension and mozilla-central. That includes PdfJsTelemetry.jsm and PdfJs.jsm files -- notice that extension "disables" embedded version of pdf.js by adding PdfJs-stub.

mozilla-central is using pdfjschildbootstrap.js+PdfJs.jsm, but firefox extension only bootstrap.js

That's good to know. I have some other changes I want to make to avoid loading .jsms when no PDF has been loaded, and it looks like that will help me.

timvandermeij · 2017-03-31T20:16:14Z

Could you perhaps squash the commits into one commit (see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits)?

timvandermeij · 2017-03-31T20:16:34Z

/botio-linux preview

pdfjschildbootstrap.js will always be run, but pdfjschildbootstrap-enabled.js will only be run if PdfJs.enabled is true. This will let us avoid some work in the child process in the next patch. This will need to be landed in the mozilla-central repository at the same time as a change to nsBrowserGlue.js. See bug 1352218.

If we only invoke the bootstrap-enabled script when PdfJs.enabled is true, then we don't need to check it again in the script. This avoids a sync IPC call to the parent process. It also keeps PdfJs.jsm from importing PdfjsContentUtils.jsm in the child process until it is actually needed, which is one steps towards not loading it until it is really needed.

amccreight · 2017-03-31T20:26:26Z

I folded the fixes into the original two patches. Is that enough, or do you really just want it in one commit? (Each patch should be a separate logical change.)

timvandermeij · 2017-03-31T20:27:54Z

I'm fine with having two logically separate commits for this. Thanks!

I'll create a new preview for this so we have an XPI file for testing.

timvandermeij · 2017-03-31T20:28:00Z

/botio-linux preview

pdfjsbot · 2017-03-31T20:28:00Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/d88eb0e9ed0951e/output.txt

pdfjsbot · 2017-03-31T20:30:44Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/d88eb0e9ed0951e/output.txt

Total script time: 2.72 mins

Published

yurydelendik · 2017-04-03T22:37:32Z

https://bugzilla.mozilla.org/show_bug.cgi?id=1352218

Thank you for the patch.

amccreight · 2017-04-03T22:45:37Z

Thanks for all of the help with my patch.

One is always run, the other is only run when PdfJs.enabled is true in the parent process. This refactoring enables the next patch. The extensions changes are from: mozilla/pdf.js#8218 MozReview-Commit-ID: HwQ3yk8Jck4 --HG-- extra : rebase_source : 94f1516989685176a95e235f2d3ef8658adaf8b7

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ --HG-- extra : rebase_source : e02829e2508bb75caf44c0a3c86b06fac3bf167f

One is always run, the other is only run when PdfJs.enabled is true in the parent process. This refactoring enables the next patch. The extensions changes are from: mozilla/pdf.js#8218 MozReview-Commit-ID: HwQ3yk8Jck4

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ

One is always run, the other is only run when PdfJs.enabled is true in the parent process. This refactoring enables the next patch. The extensions changes are from: mozilla/pdf.js#8218 MozReview-Commit-ID: HwQ3yk8Jck4

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ

Split pdfjschildbootstrap.js to avoid sync IPC

One is always run, the other is only run when PdfJs.enabled is true in the parent process. This refactoring enables the next patch. The extensions changes are from: mozilla/pdf.js#8218 MozReview-Commit-ID: HwQ3yk8Jck4 UltraBlame original commit: 20def3cc99f49c2682bc69c53f7cac57bdfb7596

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ UltraBlame original commit: 752d857f85cd9374ea28c309b74585c2548a2d7a

One is always run, the other is only run when PdfJs.enabled is true in the parent process. This refactoring enables the next patch. The extensions changes are from: mozilla/pdf.js#8218 MozReview-Commit-ID: HwQ3yk8Jck4 UltraBlame original commit: 20def3cc99f49c2682bc69c53f7cac57bdfb7596

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ UltraBlame original commit: 752d857f85cd9374ea28c309b74585c2548a2d7a

One is always run, the other is only run when PdfJs.enabled is true in the parent process. This refactoring enables the next patch. The extensions changes are from: mozilla/pdf.js#8218 MozReview-Commit-ID: HwQ3yk8Jck4 UltraBlame original commit: 20def3cc99f49c2682bc69c53f7cac57bdfb7596

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ UltraBlame original commit: 752d857f85cd9374ea28c309b74585c2548a2d7a

timvandermeij reviewed Mar 30, 2017

View reviewed changes

timvandermeij added the addon label Mar 30, 2017

amccreight added 2 commits March 31, 2017 13:22

amccreight force-pushed the no-sync branch from 60edfc3 to 740e1ab Compare March 31, 2017 20:24

yurydelendik merged commit a2ddf2f into mozilla:master Apr 3, 2017

amccreight deleted the no-sync branch April 3, 2017 22:45

Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Apr 11, 2017

Bug 1352218, part 2 - Avoid PdfJs.enabled call. r=Gijs

621adf6

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ

JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request Apr 12, 2017

Bug 1352218, part 2 - Avoid PdfJs.enabled call. r=Gijs

2ac3b74

This avoids a sync IPC message from child to parent. Changes entirely from: mozilla/pdf.js#8218 MozReview-Commit-ID: 3Egayok3DBZ

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8218 from amccreight/no-sync

d1ff0a9

Split pdfjschildbootstrap.js to avoid sync IPC

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split pdfjschildbootstrap.js to avoid sync IPC #8218

Split pdfjschildbootstrap.js to avoid sync IPC #8218

amccreight commented Mar 30, 2017

timvandermeij Mar 30, 2017

amccreight Mar 30, 2017

timvandermeij Mar 30, 2017

timvandermeij Mar 30, 2017 •

edited

Loading

amccreight Mar 30, 2017

timvandermeij Mar 30, 2017 •

edited

Loading

amccreight Mar 30, 2017

yurydelendik Mar 30, 2017

amccreight Mar 30, 2017

timvandermeij commented Mar 31, 2017

timvandermeij commented Mar 31, 2017

amccreight commented Mar 31, 2017

timvandermeij commented Mar 31, 2017 •

edited

Loading

timvandermeij commented Mar 31, 2017

pdfjsbot commented Mar 31, 2017

pdfjsbot commented Mar 31, 2017

yurydelendik commented Apr 3, 2017

amccreight commented Apr 3, 2017

Split pdfjschildbootstrap.js to avoid sync IPC #8218

Split pdfjschildbootstrap.js to avoid sync IPC #8218

Conversation

amccreight commented Mar 30, 2017

timvandermeij Mar 30, 2017

Choose a reason for hiding this comment

amccreight Mar 30, 2017

Choose a reason for hiding this comment

timvandermeij Mar 30, 2017

Choose a reason for hiding this comment

timvandermeij Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

amccreight Mar 30, 2017

Choose a reason for hiding this comment

timvandermeij Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

amccreight Mar 30, 2017

Choose a reason for hiding this comment

yurydelendik Mar 30, 2017

Choose a reason for hiding this comment

amccreight Mar 30, 2017

Choose a reason for hiding this comment

timvandermeij commented Mar 31, 2017

timvandermeij commented Mar 31, 2017

amccreight commented Mar 31, 2017

timvandermeij commented Mar 31, 2017 • edited Loading

timvandermeij commented Mar 31, 2017

pdfjsbot commented Mar 31, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Mar 31, 2017

From: Bot.io (Linux)

Success

Published

yurydelendik commented Apr 3, 2017

amccreight commented Apr 3, 2017

timvandermeij Mar 30, 2017 •

edited

Loading

timvandermeij Mar 30, 2017 •

edited

Loading

timvandermeij commented Mar 31, 2017 •

edited

Loading