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

[api-minor] Improve how we disable PDFThumbnailView.setImage for documents with Optional Content #15215

Merged
merged 5 commits into from
Jul 30, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

Please refer to the individual commit messages for additional information.

Note that I'm not sure if the second commit is actually necessary, and it may quite frankly be overkill here. (More generally I think it's interesting, since it shows one way of effectively making a method "private" despite it still being available from the outside.)

…methods

To ensure that this data cannot be directly changed from the outside, use private fields/methods now that those are available.
@Snuffleupagus Snuffleupagus marked this pull request as draft July 24, 2022 12:14
@Snuffleupagus Snuffleupagus marked this pull request as ready for review July 24, 2022 13:35
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

…e "outside"

Given that Optional Content visibility is only intended/supported to be updated via the `OptionalContentConfig.setVisibility`-method, this patch actually enforces that now.
Note that this will be used by the next patch in the series, and will help prevent inconsistent state in the `OptionalContentConfig`-class.

*Please note:* This patch also uncovered a pre-existing bug, related to iterating through the visibility groups in the constructor, for the `baseState === "OFF"` case.
…he initial Optional Content visibility state

This will allow us to improve the `PDFThumbnailView.setImage` handling in the viewer, and thanks to the added caching this should be reasonbly efficient.
… Optional Content (PR 12170 follow-up)

Rather than always disable `PDFThumbnailView.setImage` as soon as user has changed the visibility of the Optional Content, we can utilize the new method added in the previous patch to improve thumbnail performance. Note in particular how, in the old code, even *resetting* of the Optional Content to its default state wouldn't enable `PDFThumbnailView.setImage` again.

While slightly unrelated, this patch also removes the `PDFThumbnailViewer._optionalContentConfigPromise`-property since it's completely unused.
This code is completely unnecessary in e.g. the Firefox PDF Viewer.
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jul 24, 2022
@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/1f36ee9bec8cfc3/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/1f36ee9bec8cfc3/output.txt

Total script time: 25.65 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 10

Image differences available at: http://54.241.84.105:8877/1f36ee9bec8cfc3/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 29.09 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/b16d1fb576842b5/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit c7b71a3 into mozilla:master Jul 30, 2022
@timvandermeij
Copy link
Contributor

Thanks!

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