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

Limit the maximum thumbnail canvas width/height, similar to pages (PR 19604 follow-up) #19621

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

Snuffleupagus
Copy link
Collaborator

The changes in PR #19604 weren't enough for thumbnail canvases in some cases, see e.g. https://web.archive.org/web/20231204152348if_/https://user.informatik.uni-bremen.de/cabo/rfc9000.pdf#pagemode=thumbs

@Snuffleupagus Snuffleupagus added viewer release-blocker Blocker for the upcoming release labels Mar 6, 2025
@Snuffleupagus Snuffleupagus requested a review from calixteman March 6, 2025 21:58
@mozilla mozilla deleted a comment from moz-tools-bot Mar 6, 2025
@mozilla mozilla deleted a comment from moz-tools-bot Mar 6, 2025
… 19604 follow-up)

The changes in PR 19604 weren't enough for thumbnail canvases in some cases, see e.g. https://web.archive.org/web/20231204152348if_/https://user.informatik.uni-bremen.de/cabo/rfc9000.pdf#pagemode=thumbs
@moz-tools-bot
Copy link
Collaborator

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/a89a971f0a18f5e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 0.89 mins

Published

Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

These changes look good, but should we the thumbnails also respect maxCanvasPixels?

@Snuffleupagus
Copy link
Collaborator Author

but should we the thumbnails also respect maxCanvasPixels?

Technically yes, however we've (as far as I know) never seen a case where that limit becomes a problem given that thumbnails have a consistent width and that a page would have to be extremely narrow for it to become an issue; note

const THUMBNAIL_WIDTH = 98; // px

Hence I'd be inclined to defer maxCanvasPixels support in the thumbnail code to another patch, since that's been the case for over a decade without it seemingly breaking anything :-)

@Snuffleupagus Snuffleupagus merged commit 1bc98df into mozilla:master Mar 10, 2025
7 checks passed
@Snuffleupagus Snuffleupagus removed the request for review from calixteman March 10, 2025 10:32
@Snuffleupagus Snuffleupagus deleted the bug-1943094-thumbs branch March 10, 2025 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Blocker for the upcoming release viewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants