Unbounded Element: Support unbounded element from iframes [chromium/src : main]
Vladimir Levin (Gerrit)
Vladimir Levin added 2 comments![]( "Open in Gerrit")
bool Document::HasActiveUnboundedElements() const {It's a little weird to me that a document essentially says "yes" to having unbounded elements if some other document in the same local frame tree has an unbounded element.
Can you remind me where this is actually used?
if (widget) {I feel like you can just nest that in the if
Related details
- Mason Freed
- Philip Rogers
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. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
bool Document::HasActiveUnboundedElements() const {It's a little weird to me that a document essentially says "yes" to having unbounded elements if some other document in the same local frame tree has an unbounded element.
Can you remind me where this is actually used?
Yeah, this is one area where a public API will need more thought, e.g. allowing sub-frames to have permission to do this. But currently unbounded elements are one-per-top-level-frame, and that's being more hard-coded with this patch.
`HasActiveUnboundedElements` is used roughly one place right now, here:
That needs to know "is there any unbounded window right now? If so, I need to look for hit test results outside clips".
if (widget) {I feel like you can just nest that in the if
Before I just go do that, you mean like this?
```
if (active && widget) {
widget->IncrementActiveUnboundedElementCount();
} else if (!active && widget) {
widget->DecrementActiveUnboundedElementCount();
}
```
Because if so, I really prefer the existing thing...
Related details
- Philip Rogers
- Vladimir Levin
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Anything I can do to help with the review here?
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
unbounded_surface_state_ =Are there issues with supporting multiple simultaneous unbounded elements? Here we overwrite unbounded_surface_state_, but the old one is still active. This can now happen with this patch due to unbounded elements being created from different iframes which have different execution contexts.
Related details
- Mason Freed
- Vladimir Levin
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Are there issues with supporting multiple simultaneous unbounded elements? Here we overwrite unbounded_surface_state_, but the old one is still active. This can now happen with this patch due to unbounded elements being created from different iframes which have different execution contexts.
Yeah, that's currently possible. It's been on my to-do list to lock down, but I just hadn't gotten to it yet. Because of this comment, I've implemented protections against multiple unbounded elements. See patchset 13+.
Related details
- Philip Rogers
- Vladimir Levin
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
+rakina for content/browser/*
(I would have tagged jonross, but he's ooo for some time.)
Related details
- Philip Rogers
- Rakina Zata Amni
- Vladimir Levin
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. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
- Mason Freed
- Rakina Zata Amni
- Vladimir Levin
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. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
parent_view_->GetWeakPtr(), this));Eventhough the RWHVB owns `UnboundedSurfaceWindowAura`, it can already be destroyed by the sync deletion from `RequestUnboundedSurface` right? Then this is unsafe. Are there IDs that you can use that are not raw ptrs? You can use WeakPtrs I guess but differentiate the case where you pass nullptr from the start vs when the intended unbounded surface is already destroyed.
Related details
- Mason Freed
- Vladimir Levin
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +1 |
1 comment
(pre-LGTM assuming the comment gets addressed)
Related details
- Mason Freed
- Vladimir Levin
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. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
2 comments
Done, but that change cleared your +1's unfortunately.
parent_view_->GetWeakPtr(), this));Mason FreedEventhough the RWHVB owns `UnboundedSurfaceWindowAura`, it can already be destroyed by the sync deletion from `RequestUnboundedSurface` right? Then this is unsafe. Are there IDs that you can use that are not raw ptrs? You can use WeakPtrs I guess but differentiate the case where you pass nullptr from the start vs when the intended unbounded surface is already destroyed.
Done - I switched to WeakPtr. For the one sync delete path, I now just pass the active window's weak pointer, and if it's nullptr, then we just clear the pointer.
Related details
- Philip Rogers
- Rakina Zata Amni
- Vladimir Levin
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. |
Mason Freed (Gerrit)
Mason Freed voted Auto-Submit+0![]( "Open in Gerrit")
| Auto-Submit | +0 |
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
vmpstr@ I'm going to ack these two comments. Still happy to do something about both of them, but I'd like to get this landed, so that'd be via a followup. Hopefully ok! Just let me know.
Mason FreedIt's a little weird to me that a document essentially says "yes" to having unbounded elements if some other document in the same local frame tree has an unbounded element.
Can you remind me where this is actually used?
Yeah, this is one area where a public API will need more thought, e.g. allowing sub-frames to have permission to do this. But currently unbounded elements are one-per-top-level-frame, and that's being more hard-coded with this patch.
`HasActiveUnboundedElements` is used roughly one place right now, here:
That needs to know "is there any unbounded window right now? If so, I need to look for hit test results outside clips".
Acknowledged
Mason FreedI feel like you can just nest that in the if
Before I just go do that, you mean like this?
```
if (active && widget) {
widget->IncrementActiveUnboundedElementCount();
} else if (!active && widget) {
widget->DecrementActiveUnboundedElementCount();
}
```Because if so, I really prefer the existing thing...
Acknowledged
Related details
- Philip Rogers
- Rakina Zata Amni
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
- Mason Freed
- Philip Rogers
- Vladimir Levin
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)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Unbounded Element: Support unbounded element from iframes
This patch enables the Unbounded Element API to work correctly when the
unbounded element resides inside an iframe. Active unbounded element
tracking is moved from Document (TopDocument) to WebFrameWidgetImpl,
ensuring correct tracking across local subframes within a local root
frame tree. Additionally, GetOrCreateUnboundedSurfaceState is updated
to accept and use the subframe element's execution context instead of
assuming the local root's, and binds Mojo pipes using the subframe's
task runner.
To correctly position the unbounded surface relative to the top-level
viewport, unbounded element bounds are converted to viewport
coordinates during showUnboundedElement() and PrePaintTreeWalk.
Finally, to avoid losing the first submitted frame (which was causing
the newly-added web_test to fail), a forced commit and redraw via
SetNeedsCommitWithForcedRedraw is triggered when allocating a new
surface or updating the local surface ID on the LayerTreeHost.
This patch also adds code to ensure only one unbounded element can
be shown from one top level window. If `showUnboundedElement()` is called while another one is already open, it will be closed first.
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/renderer_host/render_widget_host_view_base.cc
- M content/browser/renderer_host/render_widget_host_view_base.h
- M content/browser/renderer_host/unbounded_element_browsertest.cc
- M content/browser/renderer_host/unbounded_surface_window.h
- M content/browser/renderer_host/unbounded_surface_window_aura.cc
- M content/browser/renderer_host/unbounded_surface_window_aura.h
- M content/browser/renderer_host/unbounded_surface_window_mac.h
- M content/browser/renderer_host/unbounded_surface_window_mac.mm
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/dom/document.h
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.h
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
- M third_party/blink/web_tests/TestExpectations
- A third_party/blink/web_tests/wpt_internal/html/rendering/unbounded-element/iframe-appearance-ref.html
- A third_party/blink/web_tests/wpt_internal/html/rendering/unbounded-element/iframe-appearance.html
Code-Review: +1 by Rakina Zata Amni
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |