-
Notifications
You must be signed in to change notification settings - Fork 10.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move all PDFJS display/ usages into global.js file. #7126
Conversation
2ef3f38
to
d7a9d3c
Compare
deprecated('PDFJS.openExternalLinksInNewWindow, please use ' + | ||
'"PDFJS.externalLinkTarget = PDFJS.LinkTarget.BLANK" instead.'); | ||
} | ||
PDFJS.externalLinkTarget = value ? LinkTarget.BLANK : LinkTarget.NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It might not matter too much given that we should completely remove PDFJS.openExternalLinksInNewWindow
at some point, but note that in the current code setting PDFJS.openExternalLinksInNewWindow = true;
is respected if and only if PDFJS.externalLinkTarget === LinkTarget.NONE
, and not unconditionally like this (since PDFJS.externalLinkTarget
should always take precedence).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
d7a9d3c
to
409658f
Compare
link.target = LinkTargetStringMap[PDFJS.externalLinkTarget]; | ||
var target = params.target; | ||
if (typeof target === 'undefined') { | ||
target = getDefaultSetting('externalLinkTarget'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PDFJS.externalLinkTarget
is an enumeration value, so LinkTargetStringMap
still needs to be used, otherwise link.target
isn't set correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
409658f
to
c9f79a2
Compare
@@ -280,6 +280,7 @@ var PDFViewerApplication = { | |||
this.pdfSidebar.onToggled = this.forceRendering.bind(this); | |||
|
|||
var self = this; | |||
var PDFJS = pdfjsLib.PDFJS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we get rid of this? If we keep it around, we should also use it, e.g. at https://github.com/mozilla/pdf.js/pull/7126/files#diff-dd42e9b2d2a652b8e312efa567698aa6R832
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's used below a lot.
c9f79a2
to
fbafbc0
Compare
@@ -1457,7 +1285,9 @@ var WorkerTransport = (function WorkerTransportClosure() { | |||
this.loadingTask = loadingTask; | |||
this.pdfDataRangeTransport = pdfDataRangeTransport; | |||
this.commonObjs = new PDFObjects(); | |||
this.fontLoader = new FontLoader(loadingTask.docId); | |||
this.fontLoader = new FontLoader(loadingTask.docId, { | |||
isEvalSupported: getDefaultSetting('isEvalSupported'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In https://github.com/mozilla/pdf.js/pull/7126/files#diff-c0a7c347be94f54892dea2ead8b3df4cR39, there's no parameter that takes this isEvalSupported
argument. And as far as I can tell, isEvalSupported
is not used within FontLoader
, so is this line really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not. Reverting the change.
fbafbc0
to
9c5a27e
Compare
9c5a27e
to
e2d6425
Compare
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/baa7951d81369ff/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/baa7951d81369ff/output.txt Total script time: 1.02 mins Published |
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/5dc09b9c2d459cc/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/ae6fdf2adff0b68/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/ae6fdf2adff0b68/output.txt Total script time: 20.35 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/5dc09b9c2d459cc/output.txt Total script time: 22.61 mins
|
5b7bccd
to
26c577a
Compare
26c577a
to
1e4886a
Compare
The dom_utils.js may use global PDFJS to pull values (if the object exists), see getDefaultSetting.
Also, exposed more functions/classes for generic viewer and examples and changed PDFJS object access for these.
Partially addresses #6779