Restore broken functionality and simplify the implementation in src/display/text_layer.js
#18052
Conversation
|
I wonder if the limit will help with https://bugzilla.mozilla.org/show_bug.cgi?id=1890537 and/or https://bugzilla.mozilla.org/show_bug.cgi?id=1838865. |
Snuffleupagus
force-pushed
the
textLayer-only-ReadableStream
branch
from
dc5c1eb
to
0665ab5
Compare
…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
force-pushed
the
textLayer-only-ReadableStream
branch
from
0665ab5
to
8d86e18
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/1f6acc852542871/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/d9c49bf327d6871/output.txt |
I don't think so, since the number of textLayer elements are quite low there with < 400 per page. 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. |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/1f6acc852542871/output.txt Total script time: 27.45 mins
Image differences available at: http://54.241.84.105:8877/1f6acc852542871/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/d9c49bf327d6871/output.txt Total script time: 60.00 mins |
There was a problem hiding this comment.
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.
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/4a719d06cd9232b/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/dee4a25c534ecfb/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/dee4a25c534ecfb/output.txt Total script time: 27.43 mins
Image differences available at: http://54.241.84.105:8877/dee4a25c534ecfb/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/4a719d06cd9232b/output.txt Total script time: 42.35 mins
Image differences available at: http://54.193.163.58:8877/4a719d06cd9232b/reftest-analyzer.html#web=eq.log |
Please refer to the individual commit messages for additional information.