[api-major] Move the remaining options from, and completely remove, the global `PDFJS` object #9493

Snuffleupagus · 2018-02-18T10:04:47Z

This is the final (major) piece of work required before we can release PDF.js version 2.0, the other remaining TODOs are basically of the nice-to-have kind.
I've tried to split it into as many commits as possible to aid reviewing; in some cases ignoring whitespace changes with ?w=1 should also help.

@yurydelendik, @timvandermeij Please check if the new way of handling (default) viewer options makes sense here!

Snuffleupagus · 2018-02-18T15:27:07Z

/botio test

pdfjsbot · 2018-02-18T15:27:08Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2018-02-18T15:27:08Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/f9371548bb9379e/output.txt

pdfjsbot · 2018-02-18T15:45:08Z

From: Bot.io (Linux m4)

Success

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

Total script time: 17.99 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2018-02-18T15:52:53Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/f9371548bb9379e/output.txt

Total script time: 24.28 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2018-02-22T22:30:44Z

I think I'll have time during the weekend to review this.

timvandermeij

Looks good to me with the comments addressed. I'm really impressed with how much work has been put in this patch! The new way of handling app options is much more consistent in my opinion.

timvandermeij · 2018-02-25T19:48:25Z

web/app_options.js

+  },
+  externalLinkRel: {
+    /** @type {string} */
+    value: null,


What is the reason you're not using the LinkTarget enumeration and DEFAULT_LINK_REL constant here? Especially for the latter, the default was different from null (see https://github.com/Snuffleupagus/pdf.js/blob/3c7909f2fe8329e3544942690ebf8d172bd8ae63/src/display/dom_utils.js#L22). Moreover, since this is a string type, I would at least expect ''. If this is to avoid a dependecy on code from src, can we add a comment somewhere in this file to indicate that?

The same applies for other options below.

Moreover, since this is a string type, I would at least expect ''.

You're absolutely correct that this should be a string, however an empty string wouldn't work here; see:

pdf.js/src/display/dom_utils.js

Line 317 in 5f98f9b

link.rel = (typeof rel === 'string' ? rel : DEFAULT_LINK_REL);

I've changed this particular line to explicitly list the "default" link rel instead; but see below as well.

If this is to avoid a dependecy on code from src, can we add a comment somewhere in this file to indicate that?

Correct, I view this (new) file somewhat similar to https://github.com/mozilla/pdf.js/blob/master/web/default_preferences.json. Even though we can, as opposed to a .json file, import constants/types from elsewhere here I nonetheless wanted to avoid that pattern. I've tried to add a comment to that effect.

timvandermeij · 2018-02-25T19:52:54Z

web/app_options.js

+  },
+  maxCanvasPixels: {
+    /** @type {number} */
+    value: viewerCompatibilityParams.maxCanvasPixels || 16777216,


Let's add a comment that this magic number is calculated from 4096 * 4096 (since this way in the old, removed comment). Perhaps also mention that -1 removes any limit.

I'd very much prefer to not do that, for a couple of reasons.

First of all, the "source of truth" in the new code is the JSDocs added to the viewer components and API methods, rather than this file itself; again compare with https://github.com/mozilla/pdf.js/blob/master/web/default_preferences.json.
In this particular case, the documentation is already present in

pdf.js/web/base_viewer.js

Lines 59 to 61 in 5f98f9b

* @property {number} maxCanvasPixels - (optional) The maximum supported canvas

* size in total pixels, i.e. width * height. Use -1 for no limit.

* The default value is 4096 * 4096 (16 mega-pixels).

and

pdf.js/web/base_viewer.js

Lines 59 to 61 in 5f98f9b

* @property {number} maxCanvasPixels - (optional) The maximum supported canvas

* size in total pixels, i.e. width * height. Use -1 for no limit.

* The default value is 4096 * 4096 (16 mega-pixels).

Second of all, if we start adding that kind of comments in this file we'll probably end up doing it for a lot more options. That would add maintenance burden, since each JSDoc comment would then be duplicated twice in the code-base. Furthermore, this file would become much longer and quite unwieldy in my opinion.

timvandermeij · 2018-02-25T19:56:11Z

web/app_options.js

+  workerSrc: {
+    /** @type {string} */
+    value: (typeof PDFJSDev === 'undefined' || !PDFJSDev.test('PRODUCTION') ?
+           '../src/worker_loader.js' : '../build/pdf.worker.js'),


Nit: add an extra space here.

timvandermeij · 2018-02-25T19:59:48Z

web/app.js

@@ -1512,6 +1515,9 @@ if (typeof PDFJSDev === 'undefined' || PDFJSDev.test('GENERIC')) {

 function loadFakeWorker() {
  return new Promise(function(resolve, reject) {
+    if (!GlobalWorkerOptions.workerSrc) {


Why is this if statement required? As far as I can see, this should always be present because of the default in the app options.

With the new way of setting the API/worker options, in PDFViewerApplication.open, we haven't actually set the workerSrc by the time this code runs (which happens during initialization of the viewer itself).

Previously the workerSrc was set earlier during the viewer initialization, see:

pdf.js/web/app.js

Line 53 in 5f98f9b

function configure(PDFJS) {

However, with all this refactoring I wanted to unify how/when the API/worker options are set, which necessitated setting the workerSrc here as well.
If your question is why we don't just set the workerSrc directly here, and simply skip this check, it's to be ever so slightly more accommodating to a custom deployment where globalWorkerOptions.workerSrc was manually set.

timvandermeij · 2018-02-25T20:23:46Z

src/display/api.js

@@ -26,6 +26,7 @@ import {
  RenderingCancelledException, StatTimer
 } from './dom_utils';
 import { FontFaceObject, FontLoader } from './font_loader';


It looks like the commit message of this commit is copy-pasted from the first one ;-)

I've tried to trim the commit messages a bit.

timvandermeij · 2018-02-25T20:26:03Z

src/display/api.js

@@ -169,6 +169,9 @@ function setPDFNetworkStreamFactory(pdfNetworkStreamFactory) {
 *   of PDF files. When enabled, and if the server supports partial content
 *   requests, then the PDF will be fetched in chunks.
 *   The default value is `false`.


It looks like the commit message of this commit is copy-pasted from the first one ;-)

I've tried to trim the commit messages a bit.

timvandermeij · 2018-02-25T20:26:59Z

src/display/api.js

@@ -178,6 +178,9 @@ function setPDFNetworkStreamFactory(pdfNetworkStreamFactory) {
 *   The default value is `false`.
 *   NOTE: It is also necessary to disable streaming, see above,
 *         in order for disabling of pre-fetching to work correctly.


It looks like the commit message of this commit is copy-pasted from the first one ;-)

I've tried to trim the commit messages a bit.

timvandermeij · 2018-02-25T20:42:55Z

examples/components/singlepageviewer.js

  container: container,
  linkService: pdfLinkService,
 });
 pdfLinkService.setViewer(pdfSinglePageViewer);

 // (Optionally) enable find controller.
-var pdfFindController = new PDFJS.PDFFindController({
+var pdfFindController = new pdfjsDistWebPdfViewer.PDFFindController({


For consistency with the other examples, could you add a trailing comma on the next line?

timvandermeij · 2018-02-25T20:50:19Z

web/pdf_viewer.component.js

-PDFJS.ProgressBar = ProgressBar;
-PDFJS.GenericL10n = GenericL10n;
-PDFJS.NullL10n = NullL10n;
+const pdfjsVersion = PDFJSDev.eval('BUNDLE_VERSION');


I'm not entirely sure why these two constants are added here because they weren't in the code before and they are not exported. What am I missing here?

This was probably not entirely relevant to the rest of the patch, however it addresses an in my opinion existing shortcoming.
Note how both https://github.com/mozilla/pdf.js/blob/master/src/pdf.js and https://github.com/mozilla/pdf.js/blob/master/src/pdf.worker.js contains this information (without exporting it). That can be very handy if you're looking at the built files and want to know which version/build it corresponds to.

Snuffleupagus · 2018-02-28T11:39:17Z

@timvandermeij Thanks a lot for taking the time to review this, much appreciated :-)
I've attempted to address and/or answer your review comments; care to take another look?

…y.js` and into a separate file Unfortunately, as far as I can tell, we still need the ability to adjust certain API options depending on the browser environment in PDF.js version `2.0`. However, we should be able to separate this from the general compatibility code in the `src/shared/compatibility.js` file.

…factoring of the way that viewer options are handled The way that various options are handled in the default viewer is currently a bit of a mess (to say the least). Some viewer options reside in the global `PDFJS` object, while others reside in `Preferences`. To make matters worse, some options even exist in both of the two. Since the goal, with PDF.js version `2.0`, is to reduce our usage of the global `PDFJS` object, we'll instead want pass in the options when initializing the viewer components and when calling API methods (such as `getDocument`). However given the current state of things in the default viewer, this wouldn't be exactly easy to implement. Hence this patch, which attempts to consolidate the way that viewer (and later API) options are handled by introducing a `AppOptions` singleton that provides *one* centralized way of interacting with the various options in the default viewer.

…tion.viewerPrefs` and into `AppOptions` instead

…ions` instead The `appConfig` contains (mostly) references to various DOM elements, used when initializing the viewer components. Hence `defaultUrl` seem like a slightly better fit for the new `AppOptions` abstraction, not to mention that it should thus be easier to set/modify it for custom deployments of the default viewer.

… use of `AppOptions` instead of the global `PDFJS` object

…o `getDocument` instead

…bject and into `getDocument` instead

…into `getDocument` instead

… into `getDocument` instead One additional complication with removing this option from the global `PDFJS` object, is that the viewer currently needs to check `disableAutoFetch` in a couple of places. To address this I'm thus proposing adding a getter in `PDFDocumentProxy`, to allow checking the *actually* used values for a particular `getDocument` invocation.

…g `PDFDataTransportStream`/`PDFNetworkStream` in `src/display/api.js` With options being moved from the global `PDFJS` object and into `getDocument`, a side-effect is that we're now passing in a fair number of useless parameters to the various transport/network streams. Even though this doesn't *currently* cause any problems, it nonetheless seem like a good idea to explicitly provide the parameters that are actually necessary.

pdfjsbot · 2018-03-01T17:21:40Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/9a338c627674a08/output.txt

pdfjsbot · 2018-03-01T17:24:19Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/9a338c627674a08/output.txt

Total script time: 2.64 mins

Published

Snuffleupagus · 2018-03-02T06:47:02Z

/botio test

pdfjsbot · 2018-03-02T06:47:03Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/a537ac405e38a32/output.txt

pdfjsbot · 2018-03-02T06:47:04Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2018-03-02T07:05:17Z

From: Bot.io (Linux m4)

Success

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

Total script time: 18.21 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2018-03-02T07:11:25Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/a537ac405e38a32/output.txt

Total script time: 24.36 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2018-03-03T13:50:03Z

It's really good to finally get rid of the global PDFJS object and making PDF.js version 2.0 ready for release. Thank you so much!

PR mozilla#9493 moved from `appConfig.defaultUrl` to `AppOptions.get('defaultUrl')`. However, it forgot to replace `appConfig.defaultUrl` in chromecom.js, and as a result the extension is not able to open any PDF file. This patch fixes that issue.

PR mozilla#9493 moved from `appConfig.defaultUrl` to `AppOptions.get('defaultUrl')`. However, it forgot to replace `appConfig.defaultUrl` in chromecom.js, and as a result the extension is not able to open any PDF file. This patch fixes that issue. (cherry picked from commit 39e0b4b)

agsingletrack · 2018-04-24T09:07:30Z

Hi,

I couldn't raise an issue on the pdfjs-dist repo.

We were on version 2.0.376 of pdfjs-dist and a later patch release broke the build for us as it removed the PDFJS export — we had an import line of import {PDFJS as PDFJSReader} from 'pdfjs-dist/web/pdf_viewer'.

How does the versioning of pdfjs-dist work? Why was this breaking change made in a patch release?

brendandahl · 2018-04-24T20:00:43Z

@agsingletrack see #9440

PR mozilla#9493 moved from `appConfig.defaultUrl` to `AppOptions.get('defaultUrl')`. However, it forgot to replace `appConfig.defaultUrl` in chromecom.js, and as a result the extension is not able to open any PDF file. This patch fixes that issue.

[api-major] Move the remaining options from, and completely remove, the global `PDFJS` object

PR mozilla#9493 moved from `appConfig.defaultUrl` to `AppOptions.get('defaultUrl')`. However, it forgot to replace `appConfig.defaultUrl` in chromecom.js, and as a result the extension is not able to open any PDF file. This patch fixes that issue.

Snuffleupagus added core viewer other test labels Feb 18, 2018

Snuffleupagus requested a review from yurydelendik February 18, 2018 15:18

Snuffleupagus requested a review from timvandermeij February 22, 2018 08:59

timvandermeij approved these changes Feb 25, 2018

View reviewed changes

mozilla deleted a comment from pdfjsbot Feb 28, 2018

Snuffleupagus mentioned this pull request Feb 28, 2018

Offically release version 2.0 #9521

Closed

3 tasks

Snuffleupagus added 10 commits March 1, 2018 18:11

Move various viewer components options from PDFJS/`PDFViewerApplica…

81c5509

…tion.viewerPrefs` and into `AppOptions` instead

Refactor how the default viewer handles the worker options, by making…

b0956a5

… use of `AppOptions` instead of the global `PDFJS` object

Move the maxImageSize option from the global PDFJS object and int…

b674409

…o `getDocument` instead

Move the cMapUrl and cMapPacked options from the global PDFJS o…

3c2fbdf

…bject and into `getDocument` instead

Move the isEvalSupported option from the global PDFJS object and …

f3900c4

…into `getDocument` instead

Move the disableFontFace option from the global PDFJS object and …

c7c5835

…into `getDocument` instead

Snuffleupagus added 2 commits March 1, 2018 18:11

[api-major] Completely remove the global PDFJS object

b8606ab

mozilla deleted a comment from pdfjsbot Mar 1, 2018

timvandermeij approved these changes Mar 3, 2018

View reviewed changes

timvandermeij merged commit c33bf80 into mozilla:master Mar 3, 2018

timvandermeij removed the request for review from yurydelendik March 3, 2018 13:49

rhendric mentioned this pull request Mar 3, 2018

Add scrolling and spread modes to web viewer #9208

Merged

Snuffleupagus deleted the refactor-api-options branch March 4, 2018 10:43

Snuffleupagus mentioned this pull request Mar 4, 2018

Refactor PDFJS global var usage and move it initialization into single file #6779

Closed

2 tasks

Rob--W mentioned this pull request Mar 8, 2018

[CRX] Fix error that broke the Chrome extension #9542

Merged

Rob--W mentioned this pull request Mar 10, 2018

Have an alias (e.g. PDFJS or PDFJS2) instead of window['pdfjs-dist/build/pdf'] or window.pdfjsDistBuildPdf #9545

Closed

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

Merge pull request mozilla#9493 from Snuffleupagus/refactor-api-options

2db8dc8

[api-major] Move the remaining options from, and completely remove, the global `PDFJS` object

czapkowicz mentioned this pull request Jul 30, 2018

#221 PDFJS is not defined working fix, compatible with latest pdf.js release sayanee/angularjs-pdf#222

Open

	* @property {number} maxCanvasPixels - (optional) The maximum supported canvas
	* size in total pixels, i.e. width * height. Use -1 for no limit.
	* The default value is 4096 * 4096 (16 mega-pixels).

[api-major] Move the remaining options from, and completely remove, the global PDFJS object #9493

[api-major] Move the remaining options from, and completely remove, the global PDFJS object #9493

Conversation

Snuffleupagus commented Feb 18, 2018 • edited Loading

Snuffleupagus commented Feb 18, 2018

pdfjsbot commented Feb 18, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 18, 2018

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 18, 2018

From: Bot.io (Linux m4)

Success

pdfjsbot commented Feb 18, 2018

From: Bot.io (Windows)

Success

timvandermeij commented Feb 22, 2018

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Feb 25, 2018 • edited Loading

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 Feb 26, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Feb 28, 2018 • edited Loading

pdfjsbot commented Mar 1, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Mar 1, 2018

From: Bot.io (Linux m4)

Success

Published

Snuffleupagus commented Mar 2, 2018

pdfjsbot commented Mar 2, 2018

From: Bot.io (Windows)

Received

pdfjsbot commented Mar 2, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Mar 2, 2018

From: Bot.io (Linux m4)

Success

pdfjsbot commented Mar 2, 2018

From: Bot.io (Windows)

Success

timvandermeij commented Mar 3, 2018

agsingletrack commented Apr 24, 2018

brendandahl commented Apr 24, 2018

[api-major] Move the remaining options from, and completely remove, the global `PDFJS` object #9493

[api-major] Move the remaining options from, and completely remove, the global `PDFJS` object #9493

Snuffleupagus commented Feb 18, 2018 •

edited

Loading

timvandermeij Feb 25, 2018 •

edited

Loading

Snuffleupagus Feb 26, 2018 •

edited

Loading

Snuffleupagus commented Feb 28, 2018 •

edited

Loading