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

Don't attempt to re-create the annotationLayer, for pages without any annotations, on zooming and rotation #15806

Conversation

Snuffleupagus
Copy link
Collaborator

  • Don't attempt to re-create the annotationLayer, for pages without any annotations, on zooming and rotation

    For pages without any annotations, applies e.g. to the tracemonkey.pdf document, we'll repeatedly try to re-create the annotationLayer on every zoom and rotation operation.
    The reason that this happens is because we don't insert the annotationLayer-div into the DOM unless there's annotations present on the page, which thus means that we miss the existing annotationLayer-caching present in the PDFPageView implementation.

    This is a very old issue, and the easiest solution is to simply always insert an empty (and hidden) annotationLayer-div such that the existing code/caching starts working for the "no annotations" case as well.
    Note that this is consistent with other layers, since e.g. the textLayer and/or annotationEditorLayer may be empty. Given that only a limited, by default ten, number of pages are ever active at once the additional DOM-elements shouldn't effect things negatively here.

  • Remove the API-caching of annotation-data

    This was essentially done only to compensate for the viewer calling PDFPageProxy.getAnnotations unconditionally on every annotationLayer-rendering invocation. With the previous patch that's no longer happening, and this API-caching should thus no longer be necessary.

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/8a663acf549899d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/8a663acf549899d/output.txt

Total script time: 1.26 mins

Published

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/0aebd2fd43b2c9e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/d31aad86c8432a6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/0aebd2fd43b2c9e/output.txt

Total script time: 4.14 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/d31aad86c8432a6/output.txt

Total script time: 14.81 mins

  • Integration Tests: FAILED

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

…ny annotations, on zooming and rotation

For pages without any annotations, applies e.g. to the `tracemonkey.pdf` document, we'll repeatedly try to re-create the `annotationLayer` on every zoom and rotation operation.
The reason that this happens is because we don't insert the `annotationLayer`-div into the DOM unless there's annotations present on the page, which thus means that we miss the existing `annotationLayer`-caching present in the `PDFPageView` implementation.

This is a very old issue, and the easiest solution is to simply always insert an *empty* (and hidden) `annotationLayer`-div such that the existing code/caching starts working for the "no annotations" case as well.
Note that this is consistent with other layers, since e.g. the `textLayer` and/or `annotationEditorLayer` may be empty. Given that only a limited, by default ten, number of pages are ever active at once the additional DOM-elements shouldn't effect things negatively here.
This was essentially done only to compensate for the viewer calling `PDFPageProxy.getAnnotations` unconditionally on every annotationLayer-rendering invocation. With the previous patch that's no longer happening, and this API-caching should thus no longer be necessary.
@Snuffleupagus Snuffleupagus force-pushed the AnnotationLayerBuilder-no-annotations branch from 0ad985d to 9b6d0d9 Compare December 11, 2022 17:12
@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/ddf801b8f01fa5f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/35d759b3edbe4be/output.txt

@Snuffleupagus Snuffleupagus removed the request for review from timvandermeij December 11, 2022 17:14
@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/ddf801b8f01fa5f/output.txt

Total script time: 4.17 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/35d759b3edbe4be/output.txt

Total script time: 16.91 mins

  • Integration Tests: FAILED

@Snuffleupagus Snuffleupagus merged commit 8e11cf9 into mozilla:master Dec 11, 2022
@Snuffleupagus Snuffleupagus deleted the AnnotationLayerBuilder-no-annotations branch December 11, 2022 17:32
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