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

Restore broken functionality and simplify the implementation in src/display/text_layer.js #18052

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

Please refer to the individual commit messages for additional information.

@marco-c
Copy link
Contributor

marco-c commented May 7, 2024

@mozilla mozilla deleted a comment from moz-tools-bot May 7, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 7, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 7, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 7, 2024
@Snuffleupagus Snuffleupagus force-pushed the textLayer-only-ReadableStream branch from dc5c1eb to 0665ab5 Compare May 7, 2024 10:58
…ay/text_layer.js`

The only reason that this code still accepts `TextContent` is for backward-compatibility purposes, so we can simplify the implementation by always using a `ReadableStream` internally.
…lure

By also moving it to the `TextLayerRenderTask`-instance, we can avoid a bit of manual parameter passing.
This limit is currently completely non-functional, since the check happens *after* the entire textLayer has been parsed and appended to the DOM. It seems that this has been *accidentally* broken ever since the introduction of `ReadableStream` support.
The reason that this hasn't caused noticeable textLayer-related performance issues in practice is probably because we nowadays manage to coalesce the textLayer into fewer overall DOM elements, whereas years ago many PDF documents ended up with one DOM element *per* glyph.

By moving this check, and thus restoring the functionality, we're also able to remove the `render` helper function and simplify the code.
@Snuffleupagus Snuffleupagus force-pushed the textLayer-only-ReadableStream branch from 0665ab5 to 8d86e18 Compare May 7, 2024 11:05
@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/1f6acc852542871/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/d9c49bf327d6871/output.txt

@Snuffleupagus
Copy link
Collaborator Author

I wonder if the limit will help with https://bugzilla.mozilla.org/show_bug.cgi?id=1890537

I don't think so, since the number of textLayer elements are quite low there with < 400 per page.

or https://bugzilla.mozilla.org/show_bug.cgi?id=1838865.

The PDF link in the bug seems dead, but a version of the file is available through the Internet Archive. Even in that case we're below the limit, with approximately 28000 respectively 66000 textLayer elements for the two pages.

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 27.45 mins

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

Image differences available at: http://54.241.84.105:8877/1f6acc852542871/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/d9c49bf327d6871/output.txt

Total script time: 60.00 mins

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

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

Looks good; thank you for simplifying this! (Initially I was a bit confused about the render method not doing much anymore after the first commit, but luckily the third commit got rid of the indirection altogether.)

r=me, with passing Windows tests once the bot is working again.

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@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/4a719d06cd9232b/output.txt

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 27.43 mins

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

Image differences available at: http://54.241.84.105:8877/dee4a25c534ecfb/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/4a719d06cd9232b/output.txt

Total script time: 42.35 mins

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

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

@Snuffleupagus Snuffleupagus merged commit c0b5d93 into mozilla:master May 14, 2024
9 checks passed
@Snuffleupagus Snuffleupagus deleted the textLayer-only-ReadableStream branch May 14, 2024 10:30
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.

4 participants