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

Use createPromiseCapability in /web files and avoid unnecessary PDFPageView.setPdfPage calls #8295

Merged
merged 2 commits into from
Apr 16, 2017
Merged

Use createPromiseCapability in /web files and avoid unnecessary PDFPageView.setPdfPage calls #8295

merged 2 commits into from
Apr 16, 2017

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Apr 16, 2017

In various viewer files, there's a number of cases where we basically duplicate the functionality of `createPromiseCapability` manually.
As far as I can tell, a couple of these cases have existed for a very long time, and notable even before the `createPromiseCapability` utility function existed.

Also, since we can write ES6 code now, the patch also replaces a couple of `bind` usages with arrow functions in code that's touched in the patch.
…`, to avoid potentially calling `PDFPageView.setPdfPage` multiple times for the same page

Since calling `PDFPageView.setPdfPage` will in turn call `PDFPageView.reset`, which cancels all rendering and completely resets the page, it's thus possible that we currently cause some unnecessary re-rendering during the initial loading phase of the viewer.

Depending on the order in which data arrives, it's possible (and in practice always seem to happen) that the `pdfPage` property of the *second* page has already been set during `PDFViewer.setDocument`, by the time that the request for the `pdfPage` is resolved in `PDFViewer._ensurePdfPageLoaded`.

Also, note how the `setPdfPage` call in `PDFViewer.setDocument` is already guarded by this kind of check.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/987e0ced039dea5/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/987e0ced039dea5/output.txt

Total script time: 3.11 mins

Published

@timvandermeij timvandermeij merged commit 3eb51f1 into mozilla:master Apr 16, 2017
@timvandermeij
Copy link
Contributor

Looks really good. Thanks!

@Snuffleupagus Snuffleupagus deleted the web-createPromiseCapability branch April 16, 2017 22:10
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…apability

Use `createPromiseCapability` in `/web` files and avoid unnecessary `PDFPageView.setPdfPage` calls
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.

3 participants