Move getContext call to InternalRenderTask #20016

ryzokuken · 2025-06-17T13:00:27Z

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.

web/base_pdf_page_view.js

src/display/api.js

calixteman · 2025-06-23T15:25:18Z

/botio test

moz-tools-bot · 2025-06-23T15:25:21Z

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 · 2025-06-23T15:25:21Z

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 · 2025-06-23T15:53:33Z

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 · 2025-06-23T16:18:04Z

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

timvandermeij · 2025-06-17T19:46:13Z

src/display/api.js

@@ -1404,7 +1404,7 @@ class PDFPageProxy {
   *   resolved when the page finishes rendering.
   */
  render({
-    canvasContext,
+    canvas,


Isn't this an API-breaking change? If so, this patch should either be labeled as [api-minor] with a working fallback and deprecation warning, or as [api-major] if no sensible fallback can be used here.

It was indeed, which is also why we had all the test breakage. I think with my latest update now some of the test changes can indeed be reverted if you prefer I do that. That said, I made a change that would allow the change to not be breaking (set the default value of canvas to canvasContext.canvas and continue to accept canvasContext).

calixteman · 2025-06-25T15:47:51Z

/botio test

moz-tools-bot · 2025-06-25T15:47:54Z

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 · 2025-06-25T15:47:54Z

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 · 2025-06-25T16:18:31Z

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 · 2025-06-25T16:46:12Z

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

calixteman

LGTM. Thank you.

timvandermeij

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.

timvandermeij · 2025-06-30T19:49:15Z

src/display/api.js

- * @property {CanvasRenderingContext2D} canvasContext - A 2D context of a DOM
- *   Canvas object.
+ * @property {CanvasRenderingContext2D} canvasContext - An optional
+ *   CanvasRenderingContext2D for backwards compatibility.


We should indicate the replacement option here so that users know what to migrate towards. I would suggest "canvasContext - Deprecated 2D context of a DOM Canvas object for backwards compatibility; use the canvas parameter instead."

timvandermeij · 2025-06-30T19:51:19Z

src/display/api.js

+ * @property {CanvasRenderingContext2D} canvasContext - An optional
+ *   CanvasRenderingContext2D for backwards compatibility.
+ * @property {HTMLCanvasElement} canvas - A DOM Canvas object the default value
+ *   is the canvas associated with the canvasContext paramter above.


"canvas - A DOM Canvas object. The default value is the canvas associated with the canvasContext parameter if no value is provided explicitly." (fixes the paramter typo and makes it a bit easier to read)

timvandermeij · 2025-06-30T19:53:08Z

src/display/api.js

@@ -3131,7 +3146,8 @@ class InternalRenderTask {
    this._continueBound = this._continue.bind(this);
    this._scheduleNextBound = this._scheduleNext.bind(this);
    this._nextBound = this._next.bind(this);
-    this._canvas = params.canvasContext.canvas;
+    this._canvas = params.canvas;
+    this.enableHWA = enableHWA;


Can we make this.enableHWA private here (so this._enableHWA) too to limit the API surface, or is it used in the public API somewhere?

timvandermeij · 2025-06-30T19:55:24Z

test/unit/api_spec.js

@@ -4939,7 +4939,7 @@ Caron Broadcasting, Inc., an Ohio corporation (“Lessee”).`)
        viewport.height
      );
      const renderTask = page.render({
-        canvasContext: canvasAndCtx.context,


Let's add a new test that deliberately uses the legacy canvasContext to keep coverage for it until it's eventually removed, to avoid regressions in that code path.

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 · 2025-06-30T23:55:29Z

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 self-assigned this Jun 17, 2025

ryzokuken force-pushed the move-getcontext branch 3 times, most recently from 2027870 to f89b738 Compare June 17, 2025 13:14

nicolo-ribaudo reviewed Jun 17, 2025

View reviewed changes

web/base_pdf_page_view.js Show resolved Hide resolved

ryzokuken force-pushed the move-getcontext branch from f89b738 to 680988d Compare June 17, 2025 16:31

timvandermeij added core viewer labels Jun 17, 2025

GV222 approved these changes Jun 18, 2025

View reviewed changes

calixteman reviewed Jun 20, 2025

View reviewed changes

src/display/api.js Outdated Show resolved Hide resolved

timvandermeij reviewed Jun 24, 2025

View reviewed changes

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

calixteman approved these changes Jun 25, 2025

View reviewed changes

calixteman requested a review from timvandermeij June 25, 2025 17:58

timvandermeij reviewed Jun 30, 2025

View reviewed changes

ryzokuken force-pushed the move-getcontext branch from 755099d to 0dd8b9c Compare June 30, 2025 23:54

Move getContext call to InternalRenderTask #20016

Are you sure you want to change the base?

Move getContext call to InternalRenderTask #20016

Conversation

ryzokuken commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calixteman commented Jun 23, 2025

Uh oh!

moz-tools-bot commented Jun 23, 2025

From: Bot.io (Linux m4)

Received

Uh oh!

moz-tools-bot commented Jun 23, 2025

From: Bot.io (Windows)

Received

Uh oh!

moz-tools-bot commented Jun 23, 2025

From: Bot.io (Linux m4)

Failed

Uh oh!

moz-tools-bot commented Jun 23, 2025

From: Bot.io (Windows)

Failed

Uh oh!

timvandermeij Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryzokuken Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

calixteman commented Jun 25, 2025

Uh oh!

moz-tools-bot commented Jun 25, 2025

From: Bot.io (Windows)

Received

Uh oh!

moz-tools-bot commented Jun 25, 2025

From: Bot.io (Linux m4)

Received

Uh oh!

moz-tools-bot commented Jun 25, 2025

From: Bot.io (Linux m4)

Failed

Uh oh!

moz-tools-bot commented Jun 25, 2025

From: Bot.io (Windows)

Failed

Uh oh!

calixteman left a comment

Choose a reason for hiding this comment

Uh oh!

timvandermeij left a comment

Choose a reason for hiding this comment

Uh oh!

timvandermeij Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timvandermeij Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timvandermeij Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

timvandermeij Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

ryzokuken commented Jun 30, 2025

Uh oh!

Uh oh!

ryzokuken commented Jun 17, 2025 •

edited

Loading

timvandermeij Jun 30, 2025 •

edited

Loading

timvandermeij Jun 30, 2025 •

edited

Loading