[TreesInViz] Propagate screenshot_destination_token to Viz [chromium/src : main]
0 views
Skip to first unread message
Zhenyao Mo (Gerrit)
May 7, 2026, 6:56:03 PM (3 days ago) May 7
to Tzarial, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Tzarial
Zhenyao Mo added 1 comment![]( "Open in Gerrit")
File services/viz/public/mojom/compositing/layer_context.mojom
Line 239, Patchset 1 (Latest):
blink.mojom.SameDocNavigationScreenshotDestinationToken?Zhenyao Mo . unresolved
why not mojo/public/mojom/base/unguessable_token.mojom directly?
Related details
Attention is currently required from:
- Tzarial
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. |
Tzarial (Gerrit)
May 7, 2026, 7:41:01 PM (3 days ago) May 7
to Zhenyao Mo, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Zhenyao Mo
Tzarial added 1 comment![]( "Open in Gerrit")
File services/viz/public/mojom/compositing/layer_context.mojom
Line 239, Patchset 1 (Latest):
blink.mojom.SameDocNavigationScreenshotDestinationToken?Zhenyao Mo . unresolved
why not mojo/public/mojom/base/unguessable_token.mojom directly?
Tzarial
A few reasons! This ensures you have a compiler error if you pass in the wrong kind of ID, it makes the interface communicate an explicit contract, and this makes the name consistent with the type that consumes it.
Related details
Attention is currently required from:
- Zhenyao Mo
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. |
Tzarial (Gerrit)
May 7, 2026, 8:37:47 PM (3 days ago) May 7
to Zhenyao Mo, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Zhenyao Mo
Tzarial added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Zhenyao Mo
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. |
Zhenyao Mo (Gerrit)
May 8, 2026, 11:06:37 AM (2 days ago) May 8
to Tzarial, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Tzarial
Zhenyao Mo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zhenyao Mo
| Code-Review | +1 |
2 comments
Patchset-level comments
Zhenyao Mo . resolved
LGTM
Thanks for the quick fix
Once this lands, let me kick off another CQ job for turning on TreesInViz on android bots
File services/viz/public/mojom/compositing/layer_context.mojom
Line 239, Patchset 1 (Latest):
blink.mojom.SameDocNavigationScreenshotDestinationToken?Zhenyao Mo . resolved
Tzarialwhy not mojo/public/mojom/base/unguessable_token.mojom directly?
A few reasons! This ensures you have a compiler error if you pass in the wrong kind of ID, it makes the interface communicate an explicit contract, and this makes the name consistent with the type that consumes it.
Attention is currently required from:
- Tzarial
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
May 8, 2026, 11:20:40 AM (2 days ago) May 8
to Tzarial, Chromium IPC Reviews, Giovanni Ortuno Urquidi, Zhenyao Mo, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Giovanni Ortuno Urquidi and Tzarial
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ort...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
Attention is currently required from:
- Giovanni Ortuno Urquidi
- Tzarial
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
May 8, 2026, 12:33:40 PM (2 days ago) May 8
to Tzarial, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Zhenyao Mo, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Tzarial
Giovanni Ortuno Urquidi voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Tzarial
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhenyao Mo (Gerrit)
May 8, 2026, 12:48:09 PM (2 days ago) May 8
to Tzarial, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Tzarial
Zhenyao Mo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zhenyao Mo
| Code-Review | +0 |
1 comment
File components/viz/service/layers/layer_context_impl.cc
Line 2049, Patchset 1 (Latest):
layers.SetScreenshotDestinationToken(Zhenyao Mo . unresolved
I think I know why this doesn't work. CompositorFrame's metadata holds a screenshot_destination, but it came from LayerTreeHostImpl, not from LayerTreeImpl. So you need to set it to the host_impl_ instead.
Related details
Attention is currently required from:
- Tzarial
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. |
Tzarial (Gerrit)
May 8, 2026, 1:43:30 PM (2 days ago) May 8
to Giovanni Ortuno Urquidi, Chromium IPC Reviews, Zhenyao Mo, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Giovanni Ortuno Urquidi, Tzarial and Zhenyao Mo
Tzarial voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tzarial
| Commit-Queue | +2 |
1 comment
File components/viz/service/layers/layer_context_impl.cc
Line 2049, Patchset 1:
layers.SetScreenshotDestinationToken(Zhenyao Mo . unresolved
I think I know why this doesn't work. CompositorFrame's metadata holds a screenshot_destination, but it came from LayerTreeHostImpl, not from LayerTreeImpl. So you need to set it to the host_impl_ instead.
Attention is currently required from:
- Giovanni Ortuno Urquidi
- Tzarial
- Zhenyao Mo
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. |
Zhenyao Mo (Gerrit)
May 8, 2026, 2:02:20 PM (2 days ago) May 8
to Tzarial, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Giovanni Ortuno Urquidi and Tzarial
Zhenyao Mo added 1 comment![]( "Open in Gerrit")
File cc/mojo_embedder/viz_layer_context.cc
Line 1546, Patchset 3 (Latest):
if (auto token = tree.TakeScreenshotDestinationToken(); !token.is_empty()) {Zhenyao Mo . unresolved
I think you also need to take it from the LayerTreeHostImpl because the LayerTreeImpl's token is already taken.
Related details
Attention is currently required from:
- Giovanni Ortuno Urquidi
- Tzarial
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. |
Tzarial (Gerrit)
May 8, 2026, 2:12:06 PM (2 days ago) May 8
to Giovanni Ortuno Urquidi, Chromium IPC Reviews, Zhenyao Mo, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Giovanni Ortuno Urquidi and Zhenyao Mo
Tzarial added 1 comment![]( "Open in Gerrit")
File cc/mojo_embedder/viz_layer_context.cc
Line 1546, Patchset 3:
if (auto token = tree.TakeScreenshotDestinationToken(); !token.is_empty()) {Zhenyao Mo . resolved
I think you also need to take it from the LayerTreeHostImpl because the LayerTreeImpl's token is already taken.
Attention is currently required from:
- Giovanni Ortuno Urquidi
- Zhenyao Mo
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. |
Giovanni Ortuno Urquidi (Gerrit)
May 8, 2026, 2:22:31 PM (2 days ago) May 8
to Tzarial, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Zhenyao Mo, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Tzarial and Zhenyao Mo
Giovanni Ortuno Urquidi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Ortuno Urquidi
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Tzarial
- Zhenyao Mo
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. |
Zhenyao Mo (Gerrit)
May 8, 2026, 11:04:28 PM (2 days ago) May 8
to Tzarial, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Giovanni Ortuno Urquidi and Tzarial
Zhenyao Mo voted and added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Giovanni Ortuno Urquidi
- Tzarial
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. |
Tzarial (Gerrit)
May 9, 2026, 12:25:05 AM (yesterday) May 9
to Zhenyao Mo, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Chromium LUCI CQ, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Attention needed from Giovanni Ortuno Urquidi
Tzarial added 1 comment![]( "Open in Gerrit")
File components/viz/service/layers/layer_context_impl.cc
Line 2049, Patchset 1:
layers.SetScreenshotDestinationToken(Zhenyao Mo . resolved
I think I know why this doesn't work. CompositorFrame's metadata holds a screenshot_destination, but it came from LayerTreeHostImpl, not from LayerTreeImpl. So you need to set it to the host_impl_ instead.
Tzarial
Done, sent your CL to the trybots
Tzarial
Done
Related details
Attention is currently required from:
- Giovanni Ortuno Urquidi
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)
10:11 AM (3 hours ago) 10:11 AM
to Tzarial, Zhenyao Mo, Giovanni Ortuno Urquidi, Chromium IPC Reviews, chromium...@chromium.org, cc-...@chromium.org, ipc-securi...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[TreesInViz] Propagate screenshot_destination_token to Viz
When TreesInViz is enabled, same-document navigation screenshots fail
because the `screenshot_destination_token` is not propagated from the
client side to the Viz-side `LayerTreeHostImpl`.
Normally, `CommitComplete()` transfers this token from `LayerTreeImpl`
to `LayerTreeHostImpl`. In TreesInViz mode, updates are applied directly
to the active tree via `LayerContextImpl`, bypassing `CommitComplete()`.
As a result, the generated compositor frame metadata lacked the
destination token, meaning the copy output request was never triggered
and screenshot tests timed out.
This CL: 1. Adds `screenshot_destination` to
`viz::mojom::LayerTreeUpdate`. 2. Populates it in
`VizLayerContext::UpdateDisplayTreeFrom`. 3. Adds
`SetScreenshotDestinationToken` to `LayerTreeHostImpl`. 4. Consumes the
token in `LayerContextImpl::DoUpdateDisplayTree` and
sets it on the `LayerTreeHostImpl`.
This fixes timeouts in BackForwardTransitionAnimationManagerBrowserTest
subframe transitions on Android when TreesInViz is enabled.
Bug: 510879620
Change-Id: I10a45ba39ecf17ae3b72260b0117c0a1e59c93e7
Commit-Queue: Zhenyao Mo <z...@chromium.org>
Reviewed-by: Zhenyao Mo <z...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1628267}
Files:
- M cc/mojo_embedder/viz_layer_context.cc
- M cc/trees/layer_tree_host_impl.cc
- M cc/trees/layer_tree_host_impl.h
- M components/viz/service/layers/layer_context_impl.cc
- M services/viz/public/mojom/compositing/layer_context.mojom
Change size: S
Delta: 5 files changed, 31 insertions(+), 1 deletion(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Zhenyao Mo
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages