Convert the overlay manager to ES6 syntax #8450

timvandermeij · 2017-05-26T13:12:12Z

This pull request is a redo of #8119.

You can test that the web/chromecom.js change works using the steps from #6233. The other overlays (document properties, password prompt and print) can be tested using the viewer itself. For convenience, you can use http://async5.org/moz/passwordOU.pdf (password: 123456) to test the password prompt.

Snuffleupagus

Note that I've not actually tested this yet, but generally it looks good!
However, I'm adding a couple of smaller comments based on a quick look at the code.

Snuffleupagus · 2017-05-26T13:32:51Z

web/app.js

      this.pdfDocumentProperties =
-        new PDFDocumentProperties(appConfig.documentProperties);
+        new PDFDocumentProperties(documentPropertiesConfig);


What you could perhaps do instead (here and elsewhere in the patch), to avoid adding even more of these hacks, is something similar to what PR #8394 does; see https://github.com/mozilla/pdf.js/pull/8394/files#diff-529d1853ee1bba753a0fcb40ea778723R372.

Good idea! Done in the new commit.

Snuffleupagus · 2017-05-26T13:34:01Z

web/app.js

@@ -119,6 +119,9 @@ var PDFViewerApplication = {
  store: null,
  /** @type {DownloadManager} */
  downloadManager: null,
+  /** @type {OverlayManager} */
+  // TODO: move initialization to `_initializeViewerComponents` if possible


Nit: missing a period at the end.

Done in the new commit.

Snuffleupagus · 2017-05-26T13:35:31Z

web/chromecom.js

 */
-ChromeCom.resolvePDFFile = function ChromeCom_resolvePDFFile(file, callback) {
+ChromeCom.resolvePDFFile =
+    function ChromeCom_resolvePDFFile(file, overlayManager, callback) {


This fits on one line if you just use:
ChromeCom.resolvePDFFile = function(file, overlayManager, callback) {

Done in the new commit.

Snuffleupagus · 2017-05-26T13:36:20Z

web/chromecom.js

-  var appConfig = PDFViewerApplication.appConfig;
-  ChromeCom.resolvePDFFile(appConfig.defaultUrl,
+  let appConfig = PDFViewerApplication.appConfig;
+  let overlayManager = PDFViewerApplication.overlayManager;


This can be let { appConfig, overlayManager, } = PDFViewerApplication; instead.

Done in the new commit.

Snuffleupagus · 2017-05-26T16:10:54Z

web/overlay_manager.js

-var OverlayManager = {
-  overlays: {},
-  active: null,
+const ESC_KEYCODE = 27;


I'm not sure that adding this as a constant really brings much value; it's not like the keycode of Esc is ever going to change!?

I think that's exactly the reason why it should be a constant, both for readability and because it does not change. However, I can see that that is a personal choice, so I reverted the change in the new commit.

timvandermeij · 2017-05-26T19:29:03Z

/botio-linux preview

Snuffleupagus · 2017-05-27T09:26:12Z

I left a TODO for the initialization of the overlay manager. We cannot move that (yet) because the viewer is usually not yet initialized when e.g., web/chromecom.js needs it.

Looking at the code, I'm cannot help wondering if this problem wasn't limited to the prior PR, but actually isn't an issue any more in this PR!?
In this patch, you're now accessing PDFViewerApplication.overlayManager from inside of the externalServices.initPassiveLoading function. So tracing through the code pertaining to the CHROME build target, this is what I see:

externalServices.initPassiveLoading is called from https://github.com/mozilla/pdf.js/blob/master/web/app.js#L484, within the PDFViewerApplication.initPassiveLoading method (see https://github.com/mozilla/pdf.js/blob/master/web/app.js#L481).
PDFViewerApplication.initPassiveLoading is called from https://github.com/mozilla/pdf.js/blob/master/web/app.js#L1523, which is inside of the webViewerOpenFileViaURL function (see https://github.com/mozilla/pdf.js/blob/master/web/app.js#L1493).
webViewerOpenFileViaURL is called from https://github.com/mozilla/pdf.js/blob/master/web/app.js#L1486, which is the last thing that webViewerInitialized does (see https://github.com/mozilla/pdf.js/blob/master/web/app.js#L1344).
webViewerInitialized is called from https://github.com/mozilla/pdf.js/blob/master/web/app.js#L392, once the PDFViewerApplication.initialize method has resolved.
PDFViewerApplication.initialize (see https://github.com/mozilla/pdf.js/blob/master/web/app.js#L149), will resolve only once PDFViewerApplication._initializeViewerComponents has successfully been run, see https://github.com/mozilla/pdf.js/blob/master/web/app.js#L156.
Finally, note how ChromeExternalServices.initPassiveLoading accesses PDFViewerApplication.appConfig in https://github.com/mozilla/pdf.js/blob/master/web/chromecom.js#L341. Unless the current code is broken, we can thus at the very least be sure that regardless of all of the above, we've reached line https://github.com/mozilla/pdf.js/blob/master/web/app.js#L153 when initPassiveLoading is called.

Unless I've made some sort of fundamental error in the analysis above, it should be possible to place the this.overlayManager = new OverlayManager(); call after line

pdf.js/web/app.js

Line 153 in ed661e7

this.appConfig = appConfig;

instead.
Edit: It could probably be placed in _initializeViewerComponents too, but let's be a bit defensive here!

Edit2: D'oh, but the actual issue would now probably be in pdf_print_service.js rather than in chromecom.js; I completely missed this :-(
However, we might be able to get around that by just defining let overlayManager = null; at https://github.com/mozilla/pdf.js/pull/8450/files#diff-8d1ea0bdf9b33774c7895b5fb752f6a8R21, and then adding overlayManager = PDFViewerApplication.overlayManager; before line https://github.com/mozilla/pdf.js/pull/8450/files#diff-8d1ea0bdf9b33774c7895b5fb752f6a8R313.

This obviously assumes that the PDFPrintService isn't initialized before the viewer itself somehow, but looking through the logic in PDFViewerApplication.beforePrint (see https://github.com/mozilla/pdf.js/blob/master/web/app.js#L1138) I think it should be fine!

timvandermeij · 2017-05-27T12:59:52Z

You're absolutely right! Everything works as expected with the changes you described. Here is the interdiff of the change:

diff --git a/web/app.js b/web/app.js
index 58653bfe..0b99c732 100644
--- a/web/app.js
+++ b/web/app.js
@@ -120,8 +120,7 @@ var PDFViewerApplication = {
   /** @type {DownloadManager} */
   downloadManager: null,
   /** @type {OverlayManager} */
-  // TODO: Move initialization to `_initializeViewerComponents` when possible.
-  overlayManager: new OverlayManager(),
+  overlayManager: null,
   /** @type {Preferences} */
   preferences: null,
   /** @type {Toolbar} */
@@ -264,6 +263,8 @@ var PDFViewerApplication = {
     let appConfig = this.appConfig;
 
     return new Promise((resolve, reject) => {
+      this.overlayManager = new OverlayManager();
+
       let eventBus = appConfig.eventBus || getGlobalEventBus();
       this.eventBus = eventBus;
 
diff --git a/web/pdf_print_service.js b/web/pdf_print_service.js
index 3b44da12..dde9d447 100644
--- a/web/pdf_print_service.js
+++ b/web/pdf_print_service.js
@@ -18,7 +18,7 @@ import { PDFPrintServiceFactory, PDFViewerApplication } from './app';
 import { PDFJS } from './pdfjs';
 
 let activeService = null;
-let overlayManager = PDFViewerApplication.overlayManager;
+let overlayManager = null;
 
 // Renders the page to the canvas of the given print service, and returns
 // the suggested dimensions of the output page.
@@ -310,6 +310,7 @@ if ('onbeforeprint' in window) {
 var overlayPromise;
 function ensureOverlay() {
   if (!overlayPromise) {
+    overlayManager = PDFViewerApplication.overlayManager;
     overlayPromise = overlayManager.register('printServiceOverlay',
       document.getElementById('printServiceOverlay'), abort, true);
     document.getElementById('printCancel').onclick = abort;

timvandermeij · 2017-05-27T13:01:51Z

/botio-linux preview

Snuffleupagus

I've tested all the current overlays (document properties, password prompt, printing, Chrome local file access), and everything still works as it should :-)

However, looking through pdf_print_service.js one last time, there's a couple of cases where we may want to pro-actively avoid possible, but very unlikely, errors.

Aside from that, this looks great; thank you for persisting with the class conversion of the OverlayManager!

Snuffleupagus · 2017-05-27T14:00:46Z

web/pdf_print_service.js

@@ -310,7 +310,8 @@ if ('onbeforeprint' in window) {
 var overlayPromise;
 function ensureOverlay() {
  if (!overlayPromise) {
-    overlayPromise = OverlayManager.register('printServiceOverlay',
+    overlayManager = PDFViewerApplication.overlayManager;


Given that this file binds itself to window.print, I'm wondering if there's any way that a user might be able to trigger printing before PDFViewerApplication is completely initialized?

I've not been successful in triggering this myself; but we might want to add an additional check here just for good measure, to avoid an uncaught Error if the overlayManager isn't defined yet!? Something like this, perhaps:

if (!overlayManager) { throw new Error('OverlayManager not initialized.'); // Might need a better error message... }

I have not been able to trigger that as well, but for safety I added this check in the new commit.

Snuffleupagus · 2017-05-27T14:05:36Z

web/pdf_print_service.js

@@ -217,8 +217,8 @@ window.print = function print() {
  } finally {
    if (!activeService) {
      console.error('Expected print service to be initialized.');
-      if (OverlayManager.active === 'printServiceOverlay') {
-        OverlayManager.close('printServiceOverlay');
+      if (overlayManager.active === 'printServiceOverlay') {


In line with the comment below, and the existing code in PDFPrintService.destroy, we might want change this to the follow instead (to avoid an unlikely uncaught Error):

ensureOverlay().then(function () { if (overlayManager.active !== 'printServiceOverlay') { return; // overlay was already closed } overlayManager.close('printServiceOverlay'); });

Note: I'd actually consider this a pre-existing issue with the code!

Indeed, ensureOverlay should have been there. I added it in the new commit.

timvandermeij · 2017-05-28T19:09:08Z

/botio-linux preview

timvandermeij · 2017-05-28T19:22:29Z

/botio-linux preview

pdfjsbot · 2017-05-28T19:22:29Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/ee74696060bee42/output.txt

pdfjsbot · 2017-05-28T19:24:31Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/ee74696060bee42/output.txt

Total script time: 2.02 mins

Published

Convert the overlay manager to ES6 syntax

timvandermeij added the viewer label May 26, 2017

Snuffleupagus self-requested a review May 26, 2017 13:24

Snuffleupagus reviewed May 26, 2017

View reviewed changes

timvandermeij force-pushed the es6-overlay-manager branch from b887195 to 80e0504 Compare May 26, 2017 19:24

timvandermeij force-pushed the es6-overlay-manager branch from 80e0504 to fda964f Compare May 27, 2017 13:00

Snuffleupagus approved these changes May 27, 2017

View reviewed changes

timvandermeij force-pushed the es6-overlay-manager branch from fda964f to 7c363df Compare May 28, 2017 19:08

Convert the overlay manager to ES6 syntax

e7a04fc

timvandermeij force-pushed the es6-overlay-manager branch from 7c363df to e7a04fc Compare May 28, 2017 19:20

timvandermeij merged commit 63a0328 into mozilla:master May 28, 2017

timvandermeij deleted the es6-overlay-manager branch May 28, 2017 19:26

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

Merge pull request mozilla#8450 from timvandermeij/es6-overlay-manager

c4dca92

Convert the overlay manager to ES6 syntax

This was referenced Aug 12, 2020

[Snyk] Security upgrade react-scripts from 3.2.0 to 3.4.2 yoonpsu/pdf.js#3

Open

[Snyk] Security upgrade react-scripts from 3.2.0 to 3.4.3 yoonpsu/pdf.js#4

Open

snyk-bot mentioned this pull request Oct 21, 2020

[Snyk] Security upgrade react-scripts from 3.2.0 to 3.4.4 yoonpsu/pdf.js#5

Open

snyk-bot mentioned this pull request Aug 28, 2021

[Snyk] Security upgrade react-scripts from 3.2.0 to 3.4.4 yoonpsu/pdf.js#17

Open

snyk-bot mentioned this pull request Sep 17, 2021

[Snyk] Security upgrade react-scripts from 3.2.0 to 3.4.4 yoonpsu/pdf.js#19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert the overlay manager to ES6 syntax #8450

Convert the overlay manager to ES6 syntax #8450

timvandermeij commented May 26, 2017 •

edited

Loading

Snuffleupagus left a comment

Snuffleupagus May 26, 2017

timvandermeij May 26, 2017

Snuffleupagus May 26, 2017

timvandermeij May 26, 2017

Snuffleupagus May 26, 2017

timvandermeij May 26, 2017

Snuffleupagus May 26, 2017

timvandermeij May 26, 2017

Snuffleupagus May 26, 2017 •

edited

Loading

timvandermeij May 26, 2017

timvandermeij commented May 26, 2017

Snuffleupagus commented May 27, 2017 •

edited

Loading

timvandermeij commented May 27, 2017

timvandermeij commented May 27, 2017

Snuffleupagus left a comment

Snuffleupagus May 27, 2017

timvandermeij May 28, 2017

Snuffleupagus May 27, 2017 •

edited

Loading

timvandermeij May 28, 2017

timvandermeij commented May 28, 2017

timvandermeij commented May 28, 2017

pdfjsbot commented May 28, 2017

pdfjsbot commented May 28, 2017

Convert the overlay manager to ES6 syntax #8450

Convert the overlay manager to ES6 syntax #8450

Conversation

timvandermeij commented May 26, 2017 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented May 26, 2017

Snuffleupagus commented May 27, 2017 • edited Loading

timvandermeij commented May 27, 2017

timvandermeij commented May 27, 2017

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented May 28, 2017

timvandermeij commented May 28, 2017

pdfjsbot commented May 28, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 28, 2017

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented May 26, 2017 •

edited

Loading

Snuffleupagus May 26, 2017 •

edited

Loading

Snuffleupagus commented May 27, 2017 •

edited

Loading

Snuffleupagus May 27, 2017 •

edited

Loading