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

Actually disable TextLayerRenderTask.prototype.#processItems when MAX_TEXT_DIVS_TO_RENDER is reached (PR 18089 follow-up) #18103

Merged
merged 1 commit into from
May 16, 2024

Conversation

Snuffleupagus
Copy link
Collaborator

I broke this accidentally in PR #18089, sorry about that!
Note that since #processItems is private we can no longer just "replace" the method as was done in PR #18052.

…MAX_TEXT_DIVS_TO_RENDER` is reached (PR 18089 follow-up)

I broke this accidentally in PR 18089, sorry about that!
Note that since `#processItems` is private we can no longer just "replace" the method as was done in PR 18052.
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/5fb4ad3c748a1c9/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/16022565d59f25d/output.txt

Total script time: 27.71 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 17
  different first/second rendering: 2

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/5fb4ad3c748a1c9/output.txt

Total script time: 42.79 mins

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

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

@timvandermeij timvandermeij merged commit 128705c into mozilla:master May 16, 2024
9 checks passed
@timvandermeij
Copy link
Contributor

Thank you for fixing this! I also managed to miss this in the review somehow, but I do like this solution better anyway because the previous approach of replacing the method felt a bit hacky anyway.

I do wonder why the unit test for this didn't fail though; do you happen to know why? It might indicate that we lack coverage there somehow, and if so we might want to open a follow-up for that.

@Snuffleupagus Snuffleupagus deleted the pr-18089-followup branch May 16, 2024 12:30
@Snuffleupagus
Copy link
Collaborator Author

Thank you for fixing this! I also managed to miss this in the review somehow, but I do like this solution better anyway because the previous approach of replacing the method felt a bit hacky anyway.

As always, thanks a lot for the review!

I do wonder why the unit test for this didn't fail though; do you happen to know why? It might indicate that we lack coverage there somehow, and if so we might want to open a follow-up for that.

The only way to trigger this code-path is by trying to render a textLayer with more than 100000 DOM elements, and nowadays finding such PDF documents are difficult (since we're usually able to coalesce text runs in the worker-thread).

@timvandermeij
Copy link
Contributor

The only way to trigger this code-path is by trying to render a textLayer with more than 100000 DOM elements

Ah yes, that was the part I missed here. That explains it then, so we can leave this as-is in terms of testing; 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