Skip to content
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

JS -- Implement doc object #12582

Merged
merged 1 commit into from
Nov 10, 2020
Merged

JS -- Implement doc object #12582

merged 1 commit into from
Nov 10, 2020

Conversation

calixteman
Copy link
Contributor

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More generally, do we actually need to run this._initializeJavaScript(pdfDocument); as early as is done currently?
Is it absolutely impossible for things to work correctly if we e.g. waited for onePageRendered, as we do for a number of other API-calls, to prevent them running very early and thus possibly delay initial rendering?

For example: If you're able to delay this._initializeJavaScript(pdfDocument); slightly you'd not need to re-fetch the "metadata" again and could instead use the already cached data.

web/app.js Outdated Show resolved Hide resolved
web/app.js Outdated Show resolved Hide resolved
web/app.js Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

More generally, do we actually need to run this._initializeJavaScript(pdfDocument); as early as is done currently?
Is it absolutely impossible for things to work correctly if we e.g. waited for onePageRendered, as we do for a number of other API-calls, to prevent them running very early and thus possibly delay initial rendering?

Good point, as far as I understand the specs, there is nothing which indicates that some actions must be ran before rendering (some of them must ran on "Page open" or "Document open" events).
Anyway I guess that the delay between end of rendering and the moment the user begins to interact with the document is a way enough to load the sandbox stuff.
@Snuffleupagus is it ok for you to move this._initializeJavaScript(pdfDocument); at the end of

pdf.js/web/app.js

Line 1330 in 8b652d6

onePageRendered.then(() => {
?

For example: If you're able to delay this._initializeJavaScript(pdfDocument); slightly you'd not need to re-fetch the "metadata" again and could instead use the already cached data.

@Snuffleupagus
Copy link
Collaborator

is it ok for you to move this._initializeJavaScript(pdfDocument); at the end of

pdf.js/web/app.js

Line 1330 in 8b652d6

onePageRendered.then(() => {

That's most definitely the preferred solution here, assuming if works of course :-)

web/app.js Outdated
Comment on lines 1396 to 1411
case "print":
this.triggerPrinting();
return;
case "zoom":
if (typeof detail.value === "string") {
this.pdfViewer.currentScaleValue = detail.value;
} else {
this.pdfViewer.currentScale = detail.value;
}
return;
case "layout":
this.pdfViewer.spreadMode = apiPageLayoutToSpreadMode(detail.value);
return;
case "page-num":
this.pdfViewer.currentPageNumber = detail.value + 1;
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure that you sort all of these cases alphabetically, since that will make it much easier to find things as the number of cases grow.

web/app.js Outdated
docInfo: {
...info,
metadata,
filesize: this.pdfDocumentProperties.maybeFileSize,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is first of all not guaranteed to even be set at this point in time and/or it may not actually be correct. Secondly, it's not really correct to reach into that component in this way, since that property was never intended to be "public" in the first place.

@brendandahl
Copy link
Contributor

Looks like a good starting point. Now that we are interacting with the viewer I'd really like to see some tests when we have quickjs wired up.

 * https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/js_api_reference.pdf#page=335
 * it has all the properties/methods defined in the spec
 * unimplemented methods are there but with an empty body to avoid exception when calling an undefined method
 * implement zoom, zoomType, layout, pageNum, ...
@calixteman calixteman merged commit 83658c9 into mozilla:master Nov 10, 2020
@calixteman calixteman deleted the doc branch November 10, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants