Move the `Preferences` initialization/fetching code to the top of `PDFViewerApplication.initialize`, to enable using them when initializing e.g. the viewer components #7886

Snuffleupagus · 2016-12-09T08:24:30Z

This is a new attempt at implementing, the main feature of, PR #7586. With the changes made to the 'localized' event in PR #7789, and the first commits here, this should avoid the issues with the Firefox addon that caused the previous PR to be backed out; see #7586 (comment).

I'd suggest looking at one commit at a time, and using the ?w=1 flag should help reduce (or at least simplify) the larger diffs somewhat. Please also note that GitHub may list the commits in a sub-optimal order, according to date, but they should be reviewed as follows:

Refactor PDFViewerApplication.initialize into two methods, one that reads the Preferences and one that initializes the various viewer components
Move the Preferences initialization/fetching code to the top of PDFViewerApplication.initialize, to enable using them when initializing e.g. the viewer components

Note that in quick testing using console.time/timeEnd, both locally and with the Firefox addon, the total run time of the entire PDFViewerApplication.initialize function does not seem to change with this patch.
Don't call bindEvents() until PDFViewerApplication has been initialized, and move binding of window event listeners to a helper method, to prevent errors if an event manages to arrive too soon

With bindEvents() now being called after the viewer has been initialized, we no longer need to have PDFViewerApplication.initialized checks in the event handler functions.
Furthermore by moving the window.addEventListeners to a helper method, PDFViewerApplication.initialized checks are no longer necessary in the event handlers, hence we thus address part of issue Non-determinism during viewer initialization. #7797 here as well.
Convert the only remaining consumer (in hand_tool.js) of the 'localized' event to use the localized Promise instead, and only re-dispatch the 'localized' event on the eventBus for GENERIC builds

Ideally we'd remove the 'localized' event from the eventBus, but for backwards compatibility we keep it in GENERIC builds.
Note that while we want to ensure that the direction attribute of the HTML is updated as soon as the localized Promise is resolved, we purposely wait until the viewer has been initialized to ensure that the 'localized' event will always be dispatched.

This change is

yurydelendik

Looks good with following changes:

we still need to dispatch('localized') event even if we will not handle it (for backwards compatibility)
we have lots of closures inside the initialize, can we refactor and move them in some private smaller methods, and just chain calls there.

Snuffleupagus · 2016-12-10T21:50:33Z

Looks good with following changes:

Thank you for the review comments!

we still need to dispatch('localized') event even if we will not handle it (for backwards compatibility)

Sure, I'll undo the removal. A couple of questions first though:
Would it be enough to only re-dispatch in GENERIC builds, or do we still need it everywhere?
Also, since the eventBus will now be initialized ever so slightly later than before, do we need to consider that the localized Promise might be resolved before the eventBus is defined, i.e. should I move the re-dispatching such that we know for sure that it always works?

we have lots of closures inside the initialize, can we refactor and move them in some private smaller methods, and just chain calls there.

Good point, I'll try to do that.

yurydelendik · 2016-12-10T21:55:56Z

Would it be enough to only re-dispatch in GENERIC builds, or do we still need it everywhere?
Also, since the eventBus will now be initialized ever so slightly later than before, do we need to consider that the localized Promise might be resolved before the eventBus is defined

Just GENERIC build is fine and it can done sometime during initialization (in addition to what you have now)

Snuffleupagus · 2016-12-11T09:39:48Z

I've now no restored the 'localized' event in GENERIC builds, and also re-factored PDFViewerApplication into the two methods _readPreferences and _initializeViewerComponents (both async).

/botio-linux preview

Snuffleupagus · 2016-12-12T12:14:50Z

Having just remembered issue #7797, should I simply move the various window.addEventListeners into PDFViewerApplication.bindEvents() instead of adding if (!PDFViewerApplication.initialized) { return; } checks in all the window listeners?

Snuffleupagus · 2016-12-13T12:04:52Z

Besides re-factoring the viewer initialization into helper methods, the latest version of the also PR moves the window event listeners into a helper method thus addressing part of #7797.

/botio-windows preview

pdfjsbot · 2016-12-13T12:04:52Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/02730e8a742eeb1/output.txt

pdfjsbot · 2016-12-13T12:08:13Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/02730e8a742eeb1/output.txt

Total script time: 3.35 mins

Published

Snuffleupagus · 2016-12-14T12:06:22Z

@yurydelendik Since you reviewed, and approved, this PR a number of changes have been made: PDFViewerApplication.initialize has been re-factored into _readPreferences/_intializeViewerComponents, and the binding of window event listeners was moved to a bindWindowEvents method: hence it'd probably be a good idea if you looked through this once more before landing!

yurydelendik

Looks good, thank you.

yurydelendik · 2016-12-14T14:14:07Z

web/app.js


-    this.toolbar = new Toolbar(appConfig.toolbar, container, eventBus);
+    return this._readPreferences().then(function () {
+      return self._initializeViewerComponents().then(function () {


nit: to reduce indentation level return _initializeViewerComponents promise and move closure to the outer promise then:

return self._initializeViewerComponents(); }).then(function() {

Good call, fixed now!

… reads the Preferences and one that initializes the various viewer components

…FViewerApplication.initialize`, to enable using them when initializing e.g. the viewer components Note that in quick testing using `console.time/timeEnd`, both locally and with the Firefox addon, the total run time of the *entire* `PDFViewerApplication.initialize` function does not seem to change with this patch.

…alized, and move binding of `window` event listeners to a helper method, to prevent errors if an event manages to arrive too soon With `bindEvents()` now being called after the viewer has been initialized, we no longer need to have `PDFViewerApplication.initialized` checks in the event handler functions. Furthermore by moving the `window.addEventListener`s to a helper method, `PDFViewerApplication.initialized` checks are no longer necessary in the event handlers, hence we thus address part of issue 7797 here as well.

…ized' event to use the `localized` Promise instead, and only re-dispatch the 'localized' event on the `eventBus` for `GENERIC` builds Ideally we'd remove the 'localized' event from the `eventBus`, but for backwards compatibility we keep it in `GENERIC` builds. Note that while we want to ensure that the direction attribute of the HTML is updated as soon as the `localized` Promise is resolved, we purposely wait until the viewer has been initialized to ensure that the 'localized' event will always be dispatched.

Snuffleupagus · 2016-12-14T16:22:33Z

Thanks for the review!

/botio-linux preview

pdfjsbot · 2016-12-14T16:22:34Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/96340f6e4b36559/output.txt

pdfjsbot · 2016-12-14T16:24:56Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/96340f6e4b36559/output.txt

Total script time: 2.15 mins

Published

timvandermeij · 2016-12-14T16:35:09Z

Nice work! This makes the code much more readable.

…alization Move the `Preferences` initialization/fetching code to the top of `PDFViewerApplication.initialize`, to enable using them when initializing e.g. the viewer components

Snuffleupagus added the viewer label Dec 9, 2016

yurydelendik approved these changes Dec 10, 2016

View reviewed changes

timvandermeij assigned yurydelendik Dec 11, 2016

yurydelendik approved these changes Dec 14, 2016

View reviewed changes

Snuffleupagus added 4 commits December 14, 2016 16:13

Refactor PDFViewerApplication.initialize into two methods, one that…

2c1436f

… reads the Preferences and one that initializes the various viewer components

Snuffleupagus merged commit b629be0 into mozilla:master Dec 14, 2016

timvandermeij mentioned this pull request Dec 14, 2016

Non-determinism during viewer initialization. #7797

Closed

Snuffleupagus deleted the viewer-async-initialization branch December 14, 2016 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the `Preferences` initialization/fetching code to the top of `PDFViewerApplication.initialize`, to enable using them when initializing e.g. the viewer components #7886

Move the `Preferences` initialization/fetching code to the top of `PDFViewerApplication.initialize`, to enable using them when initializing e.g. the viewer components #7886

Snuffleupagus commented Dec 9, 2016 •

edited

Loading

yurydelendik left a comment

Snuffleupagus commented Dec 10, 2016 •

edited

Loading

yurydelendik commented Dec 10, 2016

Snuffleupagus commented Dec 11, 2016

Snuffleupagus commented Dec 12, 2016

Snuffleupagus commented Dec 13, 2016

pdfjsbot commented Dec 13, 2016

pdfjsbot commented Dec 13, 2016

Snuffleupagus commented Dec 14, 2016 •

edited

Loading

yurydelendik left a comment

yurydelendik Dec 14, 2016 •

edited

Loading

Snuffleupagus Dec 14, 2016

Snuffleupagus commented Dec 14, 2016

pdfjsbot commented Dec 14, 2016

pdfjsbot commented Dec 14, 2016

timvandermeij commented Dec 14, 2016

Move the Preferences initialization/fetching code to the top of PDFViewerApplication.initialize, to enable using them when initializing e.g. the viewer components #7886

Move the Preferences initialization/fetching code to the top of PDFViewerApplication.initialize, to enable using them when initializing e.g. the viewer components #7886

Conversation

Snuffleupagus commented Dec 9, 2016 • edited Loading

yurydelendik left a comment

Choose a reason for hiding this comment

Snuffleupagus commented Dec 10, 2016 • edited Loading

yurydelendik commented Dec 10, 2016

Snuffleupagus commented Dec 11, 2016

Snuffleupagus commented Dec 12, 2016

Snuffleupagus commented Dec 13, 2016

pdfjsbot commented Dec 13, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 13, 2016

From: Bot.io (Windows)

Success

Published

Snuffleupagus commented Dec 14, 2016 • edited Loading

yurydelendik left a comment

Choose a reason for hiding this comment

yurydelendik Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Dec 14, 2016

Choose a reason for hiding this comment

Snuffleupagus commented Dec 14, 2016

pdfjsbot commented Dec 14, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 14, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Dec 14, 2016

Move the `Preferences` initialization/fetching code to the top of `PDFViewerApplication.initialize`, to enable using them when initializing e.g. the viewer components #7886

Move the `Preferences` initialization/fetching code to the top of `PDFViewerApplication.initialize`, to enable using them when initializing e.g. the viewer components #7886

Snuffleupagus commented Dec 9, 2016 •

edited

Loading

Snuffleupagus commented Dec 10, 2016 •

edited

Loading

Snuffleupagus commented Dec 14, 2016 •

edited

Loading

yurydelendik Dec 14, 2016 •

edited

Loading