Skip to content

[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

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

ryzokuken
Copy link
Collaborator

@ryzokuken ryzokuken commented Jun 17, 2025

Move the getContext call out of _createContext and into InternalRenderTask#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 the getContext call, which isn't allowed for OffscreenCanvas. Also it isn't allowed to clone or transferControlToOffscreen the canvas after the getContext call.

@ryzokuken ryzokuken self-assigned this Jun 17, 2025
@ryzokuken ryzokuken force-pushed the move-getcontext branch 3 times, most recently from 2027870 to f89b738 Compare June 17, 2025 13:14
@calixteman
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/e6ca5fca607aefc/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/80c5eb5c6660194/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 28.18 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  errors: 3903

Image differences available at: http://54.241.84.105:8877/e6ca5fca607aefc/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/80c5eb5c6660194/output.txt

Total script time: 52.70 mins

  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED
  errors: 3903

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

@ryzokuken ryzokuken force-pushed the move-getcontext branch 4 times, most recently from 19bf452 to 755099d Compare June 25, 2025 14:33
@calixteman
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/9ee77ced85e7946/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/425d8c04b161167/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/425d8c04b161167/output.txt

Total script time: 30.61 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/9ee77ced85e7946/output.txt

Total script time: 58.29 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@calixteman calixteman requested a review from timvandermeij June 25, 2025 17:58
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.

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.

@ryzokuken
Copy link
Collaborator Author

Thanks for the review and the suggestions @timvandermeij. Let me know if you think the test should suffice or if I should expect a few other things.

@ryzokuken
Copy link
Collaborator Author

ryzokuken commented Jul 1, 2025

Okay I added an expectation in the test to make sure the canvas is not entirely blank, making sure something was rendered.

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 to me, with the final comments addressed and passing full test suite. Thanks!

@timvandermeij
Copy link
Contributor

timvandermeij commented Jul 2, 2025

Please also amend the commit message to prepend [api-minor] because this is a patch that affects the API surface but not in a breaking way because it's a backwards-compatible change (otherwise it would have been [api-major]). This makes it so that readers of the changelog are aware that the new parameter exists.

Once this PR is merged, please create another PR to bump the minor version number to 5.4 similar to 6c803e8 (the baseVersion has will then be the hash of your [api-minor] commit on master).

@ryzokuken ryzokuken force-pushed the move-getcontext branch 3 times, most recently from 6961ea4 to 8f054ec Compare July 2, 2025 23:23
Copy link
Contributor

@calixteman calixteman left a 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.

@timvandermeij timvandermeij changed the title Move getContext call to InternalRenderTask [api-minor] Move getContext call to InternalRenderTask ryzokuken ryzokukenMove getContext call to InternalRenderTask Jul 3, 2025
@timvandermeij timvandermeij changed the title [api-minor] Move getContext call to InternalRenderTask ryzokuken ryzokukenMove getContext call to InternalRenderTask [api-minor] Move getContext call to InternalRenderTask Jul 3, 2025
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.

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.
@timvandermeij
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/4e00c208abb783d/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/cc91921838b8808/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/4e00c208abb783d/output.txt

Total script time: 30.52 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/cc91921838b8808/output.txt

Total script time: 50.84 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@ryzokuken
Copy link
Collaborator Author

The failed integration tests are the flaky tests that also fail on master.

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 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.

@timvandermeij timvandermeij requested a review from calixteman July 8, 2025 17:25
Copy link
Contributor

@calixteman calixteman left a 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.

@calixteman calixteman merged commit 1b427a3 into mozilla:master Jul 8, 2025
9 checks passed
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.

6 participants