[api-major] Refactor viewer components initialization to reduce their dependency on the global PDFJS object
#9479
Conversation
…lity.js` and into a separate file Unfortunately, as far as I can tell, we still need the ability to adjust certain viewer 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.
This removes the `PDFJS.useOnlyCssZoom` dependency from the viewer components, but please note that as a *temporary* solution the default viewer still uses it.
This removes the `PDFJS.maxCanvasPixels` dependency from the viewer components, but please note that as a *temporary* solution the default viewer still uses it.
…`AnnotationLayerBuilder` option This removes the `PDFJS.imageResourcesPath` dependency from the viewer components and the test-suite, but please note that as a *temporary* solution the default viewer still uses it.
…nkService` options This removes the `PDFJS.externalLinkTarget`/`PDFJS.externalLinkRel` dependency from the viewer components, but please note that as a *temporary* solution the default viewer still uses it.
…ferences with a single `textLayerMode` option/preference Rather than having two different (but connected) options for the textLayer, I think that it makes sense to try and unify this. For example: currently if `disableTextLayer === true`, then the value of `enhanceTextSelection` is simply ignored. Since PDF.js version `2.0` already won't be backwards compatible in lots of ways, I don't think that we need to worry about migrating existing preferences here.
…assed, via `BaseViewer`/`PDFPageView`, to `PDFPageProxy.render` Please note that the, pre-existing, viewer preference is already named `enableWebGL`; fixes 4919.
This allows us to remove an otherwise unnecessary `PDFJS` dependency from the `web/genericcom.js` file.
There's no need to expose these properties multiple times, since they're already exported by `src/display/api.js`.
In order to simplify things, the undocumented `enableStats` option was removed and `pdfBug` is now instead used to enabled general debugging *and* page request/rendering stats. Considering that in the default viewer the `stats` was only used when debugging was also enabled, this simplification (code wise) definitely seem worthwhile to me.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b1940c3c6b3e8ce/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/b1940c3c6b3e8ce/output.txt Total script time: 8.37 mins Published |
Snuffleupagus
changed the title
PDFJS objectPDFJS object
There was a problem hiding this comment.
Sorry for long response. I looked the overall approach and I like it. I did not do any nitpicking, if @timvandermeij agrees/glances too, we can land it. Thank you for the patch.
|
Really good. I'll take a close look in the coming few days. |
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3d6bbced84e8f27/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/493e7f074e246ea/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/493e7f074e246ea/output.txt Total script time: 24.37 mins
|
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/3d6bbced84e8f27/output.txt Total script time: 40.09 mins
|
|
Thank you for improving this! It's quite a bit to review, but I really like how this simplifies things. The subtle improvements for the text layer are also good to have. |
|
@yurydelendik, @timvandermeij Thank you both for helping review this! |
|
I coincidentally came across this change, and I believe that the decision to not migrate preferences is taken too lightly. The Chrome extension allows administrators to control PDF.js via the preferences, and it is important that these identifiers are stable. Even if you don't want to write migration code, I'd appreciate if you ping me whenever there is a backwards-incompatible preference change. This is also necessary so I can mention the new preference name in release notes. This is the second time that I notice breaking changes to preferences. For the previous one (hand tool -> hand+cursor tool), I wrote this migration code: 19549bb (similar logic needs to be applied here). |
Generally I'd agree that we should always try to migrate preferences, however I think there's extenuating circumstances in this case!
That's a fair point, sorry about that!
Unfortunately I don't have time, and my sole focus right now is to get https://github.com/mozilla/pdf.js/projects/5 over the line to avoid holding up future releases any longer. |
|
The Chrome extension is built off master, so it's already using 2.x. I'll create a new issue and assign it to me. |
|
Eventually these changes will also make it into Firefox, won't it? Firefox is also already using 2.x, shouldn't these preferences be migrated in Firefox too? |
That's correct.
Considering that the preferences are not easily accessible in the UI, but rather only available through |
mozilla/pdf.js#9479 broke the PDF viewer, so keep using the older version until we fix the compatibility issues
…tions [api-major] Refactor viewer components initialization to reduce their dependency on the global `PDFJS` object
Since PR #9185 is, at this point in time, the main remaining blocker for PDF.js
2.0, I've tried to split that one up a bit to hopefully be able to move forward here. As indicated in #9185 (comment) this PR will result in a less than ideal intermediate state, but hopefully that's acceptable.Fixes #4919.
Fixes #9322.
@timvandermeij Sorry to ask, but I'm hoping that you might be willing (time permitting) to review this?