Move placeholder_client out of the CanvasResourceDispatcher [chromium/src : main]
0 views
Skip to first unread message
Vasiliy Telezhnikov (Gerrit)
Jun 12, 2026, 10:47:12 AMJun 12
to Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Colin Blundell
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Colin Blundell
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Jun 12, 2026, 12:17:52 PMJun 12
to Vasiliy Telezhnikov, Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Vasiliy Telezhnikov
Colin Blundell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
Patchset-level comments
Colin Blundell . resolved
Thanks!
File third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc
Line 580, Patchset 2 (Latest):
std::move(dispatcher_task_runner),Colin Blundell . unresolved
musing for potential followup: is it a bit odd for the placeholder to have these ties to the dispatcher now (here and just below)?
Related details
Attention is currently required from:
- Vasiliy Telezhnikov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Jun 12, 2026, 12:31:43 PMJun 12
to Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Vasiliy Telezhnikov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Commit-Queue | +2 |
2 comments
Patchset-level comments
File third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc
musing for potential followup: is it a bit odd for the placeholder to have these ties to the dispatcher now (here and just below)?
Vasiliy Telezhnikov
So there are two separate things:
- Task runner, which is just "this thread task runner". I plan to move creation of the placeholder client to GetContext (it's where we become thread bound and canvas can't be moved across threads anymore), then we'd just use `GetTaskRunner(TaskType::kInternalDefault);`
- Animation state. This is a bit tricky, maybe we should change logic a bit for clarity. Essentially it's placeholder canvas forwards [page visibility signal](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;drc=5971f5f9d8aa5434519791361e63cdac46520baa;l=1742). This information really belongs to placeholder<=>canvas communication, but it does affect how CRD needs to work (should it request BeginFrame from viz or should it run timer, because viz won't send anything), so we need to tell dispatcher something.
I see two options to clarify this:
- Send visibility/presence of capture from placeholder, compute animation state here. It would require sending two fields instead of one (and so we'd need to store [deferred value](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/offscreen_canvas_placeholder.cc;drc=ea1f27a398caf47ad1459dc6c10d65871c947721;l=81).
- Instead of having lambda here, have a real function, so it's a bit more obvious what we're doing.
Orthogonal to this we should move [this](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc;drc=1adcb18ff079091fcb79278f0941ac5bc9227752;l=260) out of CRD.
I'll take a look once this settles down.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jun 12, 2026, 12:46:57 PMJun 12
to Vasiliy Telezhnikov, Colin Blundell, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Commit message:
Move placeholder_client out of the CanvasResourceDispatcher
It's really part of the OffscreenCanvas itself, so move it there. No
functional changes, the life time matches CRD.
In a follow-up we should just create it once when we become thread
bound.
Change-Id: I0cc255e34753ec4d7e7fb90d6392a953f5348d84
Reviewed-by: Colin Blundell <blun...@chromium.org>
Commit-Queue: Vasiliy Telezhnikov <vas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1646039}
Files:
- M third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc
- M third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.h
- M third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.h
Change size: M
Delta: 4 files changed, 35 insertions(+), 33 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Colin Blundell
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Jun 15, 2026, 5:19:56 AMJun 15
to Chromium LUCI CQ, Vasiliy Telezhnikov, Colin Blundell, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Colin Blundell added 2 comments![]( "Open in Gerrit")
Patchset-level comments
Colin Blundell . resolved
Thanks!
File third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc
Line 580, Patchset 2:
std::move(dispatcher_task_runner),Colin Blundell . resolved
Vasiliy Telezhnikovmusing for potential followup: is it a bit odd for the placeholder to have these ties to the dispatcher now (here and just below)?
So there are two separate things:
- Task runner, which is just "this thread task runner". I plan to move creation of the placeholder client to GetContext (it's where we become thread bound and canvas can't be moved across threads anymore), then we'd just use `GetTaskRunner(TaskType::kInternalDefault);`
- Animation state. This is a bit tricky, maybe we should change logic a bit for clarity. Essentially it's placeholder canvas forwards [page visibility signal](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;drc=5971f5f9d8aa5434519791361e63cdac46520baa;l=1742). This information really belongs to placeholder<=>canvas communication, but it does affect how CRD needs to work (should it request BeginFrame from viz or should it run timer, because viz won't send anything), so we need to tell dispatcher something.
I see two options to clarify this:
- Send visibility/presence of capture from placeholder, compute animation state here. It would require sending two fields instead of one (and so we'd need to store [deferred value](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/offscreen_canvas_placeholder.cc;drc=ea1f27a398caf47ad1459dc6c10d65871c947721;l=81).
- Instead of having lambda here, have a real function, so it's a bit more obvious what we're doing.
Orthogonal to this we should move [this](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc;drc=1adcb18ff079091fcb79278f0941ac5bc9227752;l=260) out of CRD.
I'll take a look once this settles down.
Colin Blundell
SGTM, thanks!
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages