-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[api-minor] Move getContext call to InternalRenderTask #20016
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
Conversation
2027870
to
f89b738
Compare
f89b738
to
680988d
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/e6ca5fca607aefc/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/80c5eb5c6660194/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/e6ca5fca607aefc/output.txt Total script time: 28.18 mins
Image differences available at: http://54.241.84.105:8877/e6ca5fca607aefc/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/80c5eb5c6660194/output.txt Total script time: 52.70 mins
Image differences available at: http://54.193.163.58:8877/80c5eb5c6660194/reftest-analyzer.html#web=eq.log |
19bf452
to
755099d
Compare
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/9ee77ced85e7946/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/425d8c04b161167/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/425d8c04b161167/output.txt Total script time: 30.61 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/9ee77ced85e7946/output.txt Total script time: 58.29 mins
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, but I'd like to have a final look after the comments are addressed just to make sure I didn't miss anything because this involves a (now fortunately non-breaking, so thanks for that!) API change.
755099d
to
0dd8b9c
Compare
Thanks for the review and the suggestions @timvandermeij. Let me know if you think the test should suffice or if I should |
0dd8b9c
to
344a0dc
Compare
Okay I added an expectation in the test to make sure the canvas is not entirely blank, making sure something was rendered. |
344a0dc
to
0fec688
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with the final comments addressed and passing full test suite. Thanks!
Please also amend the commit message to prepend Once this PR is merged, please create another PR to bump the minor version number to 5.4 similar to 6c803e8 (the |
6961ea4
to
8f054ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an integration test in order to check that the thumbnails are correctly rendered. Thank you.
8f054ec
to
a538ca0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for incorporating the last feedback and including more tests! Looks good to me after the final comments on the new test that was added (which should also address the linting failure from the CI), and with passing tests.
This is a precursor to moving the call into a worker thread to let us use `OffscreenCanvas`. The current position wouldn't work since we make transformations to the canvas object after the getContext call, which isn't allowed for OffscreenCanvas. Also it isn't allowed to clone or `transferControlToOffscreen` the canvas after the `getContext` call.
a538ca0
to
b1b728d
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/4e00c208abb783d/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/cc91921838b8808/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/4e00c208abb783d/output.txt Total script time: 30.52 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/cc91921838b8808/output.txt Total script time: 50.84 mins
|
The failed integration tests are the flaky tests that also fail on master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with a follow-up PR after merging to bump pdfjs.config
. Thank you!
@calixteman Please have another look at this; feel free to merge it if you're happy with it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested locally and everything works fine.
Thank you.
Move the
getContext
call out of_createContext
and intoInternalRenderTask#initializeGraphics
and adapt tests accordingly.This is a precursor to moving the call into a worker thread to let us use
OffscreenCanvas
. The current position wouldn't work since we make transformations to the canvas object after thegetContext
call, which isn't allowed forOffscreenCanvas
. Also it isn't allowed to clone ortransferControlToOffscreen
the canvas after thegetContext
call.