SurfaceEmbed: Propagate render throttling status to embedded WebContents [chromium/src : main]
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 3 comments![]( "Open in Gerrit")
!frame_tree_->delegate()->GetOuterDelegateFrameTreeNodeId().is_null();By adding `!frame_tree_->delegate()->GetOuterDelegateFrameTreeNodeId().is_null()` to the `is_guest_view` condition, all inner WebContents (including SurfaceEmbed and Portals) will be classified as `kGuestView`.
Because the check for `is_guest_view` comes before `delegate_->IsEmbeddedViaSurfaceEmbed()`, the new `kSurfaceEmbed` widget type will actually never be reached.
Was this change to `is_guest_view` intended? If not, reverting this specific addition should allow `kSurfaceEmbed` to work correctly.
connector->UpdateRenderThrottlingStatus(true, false, false);Can we add parameter name labels to `UpdateRenderThrottlingStatus`?
if (rvh->frame_tree()->is_primary()) {Prerender frame trees will return `false` for `is_primary()`, which means they will skip this block and incorrectly return `true` (embedded). Since prerendered pages are top-level frames, this will incorrectly apply embedded layer tree settings to them (which can affect scrollbars, backgrounds, etc.).
Instead of checking `is_primary()`, it's safer to explicitly allow fenced frames, since guest and surface embeds are already handled above:
```cpp
if (owner_delegate->IsRenderViewHost()) {
auto* rvh = static_cast<RenderViewHostImpl*>(owner_delegate);
return rvh->frame_tree()->is_fenced_frame();
}
```
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(SurfaceEmbedConnectorImplBrowserTest,
ThrottlingPropagation) {should we also be adding a test to surface_embed_plugin_browsertest.cc?
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
!frame_tree_->delegate()->GetOuterDelegateFrameTreeNodeId().is_null();By adding `!frame_tree_->delegate()->GetOuterDelegateFrameTreeNodeId().is_null()` to the `is_guest_view` condition, all inner WebContents (including SurfaceEmbed and Portals) will be classified as `kGuestView`.
Because the check for `is_guest_view` comes before `delegate_->IsEmbeddedViaSurfaceEmbed()`, the new `kSurfaceEmbed` widget type will actually never be reached.
Was this change to `is_guest_view` intended? If not, reverting this specific addition should allow `kSurfaceEmbed` to work correctly.
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
if (rvh->frame_tree()->is_primary()) {Prerender frame trees will return `false` for `is_primary()`, which means they will skip this block and incorrectly return `true` (embedded). Since prerendered pages are top-level frames, this will incorrectly apply embedded layer tree settings to them (which can affect scrollbars, backgrounds, etc.).
Instead of checking `is_primary()`, it's safer to explicitly allow fenced frames, since guest and surface embeds are already handled above:
```cpp
if (owner_delegate->IsRenderViewHost()) {
auto* rvh = static_cast<RenderViewHostImpl*>(owner_delegate);
return rvh->frame_tree()->is_fenced_frame();
}
```
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 2 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(SurfaceEmbedConnectorImplBrowserTest,
ThrottlingPropagation) {Cammie Smith Barnesshould we also be adding a test to surface_embed_plugin_browsertest.cc?
Done
connector->UpdateRenderThrottlingStatus(true, false, false);Cammie Smith BarnesCan we add parameter name labels to `UpdateRenderThrottlingStatus`?
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 2 comments![]( "Open in Gerrit")
visual_properties.is_embedded = delegate_->IsWidgetEmbedded(this);If this is a popup widget (`is_frame_widget == false`), `WebContentsImpl::IsWidgetEmbedded` will still return `true` because popups don't have an `owner_delegate` and fall through to its default `return true`.
Consider changing this to:
```cpp
visual_properties.is_embedded = is_frame_widget && delegate_->IsWidgetEmbedded(this);
```
or handle popups explicitly in `WebContentsImpl::IsWidgetEmbedded` to prevent them from being incorrectly marked as embedded.
return true;Popup widgets (which do not have an `owner_delegate`) will fall through to here and return `true`, incorrectly marking them as embedded. You might want to handle popups explicitly (e.g., by ensuring this is only called/true for frame widgets) or handle this in `RenderWidgetHostImpl::SynchronizeVisualProperties`.
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 3 comments![]( "Open in Gerrit")
// Popup widget should not be embedded.
RenderWidgetHostImpl* popup_rwh = RenderWidgetHostFactory::CreateSelfOwned(
&contents()->GetPrimaryFrameTree(), contents(),
contents()
->GetPrimaryMainFrame()
->GetSiteInstance()
->group()
->GetSafeRef(),
contents()->GetPrimaryMainFrame()->GetProcess()->GetNextRoutingID(),
contents()->IsHidden(), contents()->GetPrimaryMainFrame()->GetGlobalId());
EXPECT_FALSE(contents()->IsWidgetEmbedded(popup_rwh));
popup_rwh->ShutdownAndDestroyWidget(true);is this addition to the test in the correct file? what do popups have to do with the prerender host? I think we should move these newly added lines to a standalone test in a file that is more relevant to popup or widget handling.
visual_properties.is_embedded = delegate_->IsWidgetEmbedded(this);If this is a popup widget (`is_frame_widget == false`), `WebContentsImpl::IsWidgetEmbedded` will still return `true` because popups don't have an `owner_delegate` and fall through to its default `return true`.
Consider changing this to:
```cpp
visual_properties.is_embedded = is_frame_widget && delegate_->IsWidgetEmbedded(this);
```
or handle popups explicitly in `WebContentsImpl::IsWidgetEmbedded` to prevent them from being incorrectly marked as embedded.
Done
Popup widgets (which do not have an `owner_delegate`) will fall through to here and return `true`, incorrectly marking them as embedded. You might want to handle popups explicitly (e.g., by ensuring this is only called/true for frame widgets) or handle this in `RenderWidgetHostImpl::SynchronizeVisualProperties`.
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
// Popup widget should not be embedded.
RenderWidgetHostImpl* popup_rwh = RenderWidgetHostFactory::CreateSelfOwned(
&contents()->GetPrimaryFrameTree(), contents(),
contents()
->GetPrimaryMainFrame()
->GetSiteInstance()
->group()
->GetSafeRef(),
contents()->GetPrimaryMainFrame()->GetProcess()->GetNextRoutingID(),
contents()->IsHidden(), contents()->GetPrimaryMainFrame()->GetGlobalId());
EXPECT_FALSE(contents()->IsWidgetEmbedded(popup_rwh));
popup_rwh->ShutdownAndDestroyWidget(true);is this addition to the test in the correct file? what do popups have to do with the prerender host? I think we should move these newly added lines to a standalone test in a file that is more relevant to popup or widget handling.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
// Toggles render throttling on the child WebContents.**API-T14-01**: Please explicitly document the behavioral semantics of consecutive IPC calls here. The term "Toggles" implies that successive calls invert the current state. However, since the method accepts explicit boolean parameters (`is_throttled`, etc.), it actually replaces the existing state.
Please update the comment to unambiguously clarify that this method updates or replaces the throttling status to match the provided parameters, rather than toggling it.
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
// Toggles render throttling on the child WebContents.**API-T14-01**: Please explicitly document the behavioral semantics of consecutive IPC calls here. The term "Toggles" implies that successive calls invert the current state. However, since the method accepts explicit boolean parameters (`is_throttled`, etc.), it actually replaces the existing state.
Please update the comment to unambiguously clarify that this method updates or replaces the throttling status to match the provided parameters, rather than toggling it.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
bool is_throttled, bool subtree_throttled, bool display_locked);Consider grouping these sequential boolean parameters (`is_throttled`, `subtree_throttled`, `display_locked`) into a single strongly-typed struct (e.g., `RenderThrottlingStatus`). In adherence with the spirit of Rule 6 (IPC Type Safety) and to improve code clarity, structured Mojom primitives are preferred over multiple independent boolean flags to prevent "boolean blindness" and accidental parameter swapping across IPC boundaries.
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
bool is_throttled, bool subtree_throttled, bool display_locked);Consider grouping these sequential boolean parameters (`is_throttled`, `subtree_throttled`, `display_locked`) into a single strongly-typed struct (e.g., `RenderThrottlingStatus`). In adherence with the spirit of Rule 6 (IPC Type Safety) and to improve code clarity, structured Mojom primitives are preferred over multiple independent boolean flags to prevent "boolean blindness" and accidental parameter swapping across IPC boundaries.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
kere...@chromium.org is from context(googleclient/chrome/chromium_gwsq/components/surface_embed/config.gwsq)
Related details
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
Thanks for the CL!
Looks like there're CQ failures and merge conflicts. Let me know if you want my review now or if I should wait for your fix.
Related details
- Cammie Smith Barnes
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
Thanks for the CL!
Looks like there're CQ failures and merge conflicts. Let me know if you want my review now or if I should wait for your fix.
Fixed the merge conflict and CQ failures, so it's ready for review. Thanks!
Related details
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 13 comments![]( "Open in Gerrit")
Thank you Cammie! The direction overall looks good but the complexity is a bit overwhelming. I leave some comments.
It is worth mentioning the existing OOPIF throttling mechanism and this CL builds on top of that, e.g.,
```
This CL extends the existing OOPIF throttling pipeline to support
throttling embedded main frame (GuestView and SurfaceEmbed). The
throttling state is passed through FrameConnector ->
RenderWidgetHostViewChildFrame -> blink::FrameWidget when
RWHVChildFrame::UpdateRenderThrottlingStatus() is called.
```
This occurred because WebContentsImpl::IsWidgetEmbedded() returned falseThis description sounds like `IsWidgetEmbedded` exists today, which is not true. Maybe reword? I also think that maybe we don't need this method (I leave a separate comment for that).
- Adding `UpdateRenderThrottlingStatus` to `mojom::SurfaceEmbedHost` toCan we mention that `UpdateRenderThrottlingStatus` is a new virtual method in `blink::WebPlugin` and also describe the source of the throttling status (i.e., it originates from the parent LocalFrameView)?
// Represents the render throttling status of an embedded frame.
struct RenderThrottlingStatus {
bool is_throttled;
bool subtree_throttled;
bool display_locked;
};This is the same set of booleans used by [RemoteFrame](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/frame/remote_frame.mojom;l=191;drc=0a5e9fb6b61cd0e736d3166b99326d33b9ccea66) and [FrameWidget](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/page/widget.mojom;l=160;drc=c0f65cfcd9968553a6d669588ea8dec10d241627). I think we can make it a shared struct (in a follow up).
For now, can we reference `RemoteFrame::UpdateRenderThrottlingStatus` in a comment? (that the same set of parameters are used).
// provided parameters. Successive calls replace the existing state rather
// than toggling it.What does "toggle" mean? This function signature looks stateless to me (i.e. it does not depend on the current throttling state of the child side), and so "toggle" seems redundant. Maybe delete the whole sentence?
UpdateRenderThrottlingStatus(RenderThrottlingStatus status);About naming, maybe `OnEmbedElementThrottlingStatusChanged`, which hints that `SurfaceEmbedHost` will decide how the state is applied to the child WebContents? For example, it may reject throttling a child WebContents when the WebContents is being screen-captured.
} else if (delegate_->IsEmbeddedViaSurfaceEmbed()) {What will break without this change? If it breaks `CHECK`s, can you point me to them?
Currently, an surface-embedded Blink frame does not know about itself being embeded. This line will change that (via [CreateFrameParams::is_for_nested_main_frame](https://source.chromium.org/chromium/chromium/src/+/main:content/common/frame.mojom;l=361;drc=60df9044934a2841875904f21e9673afe970da1e) set to true due to [type != kTopLevel](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=4598;drc=03b18568967f29eb7d198a513c9be98291a5ce6c)). We want to make a surface-embedded frame behave like a top-level frame as much as possible, so I am curious if we can avoid this change.
bool is_frame_widget() const { return !self_owned_; }We don't actually need this. See my comment in `web_contents_impl.cc`.
is_frame_widget && delegate_->IsWidgetEmbedded(this);Can we replace this with `GetView()->IsRenderWidgetHostViewChildFrame()`, so that we don't need the new `IsWidgetEmbedded`?
virtual bool IsRenderViewHost() const;Can we avoid adding this virtual method? The RenderView concept is being removed (see comment above this class).
ThrottlingPropagation) {I think this test does not really test state propagation? e.g., the state is set then retrieved from connector. No IPC is involved. I think we need better test coverage.
if (!render_widget_host->is_frame_widget()) {This is equivalent to `render_widget_host->GetView()->GetWidgetType() == WidgetType::kFrame` (no need for the new `is_frame_widget` getter).
Related details
- Cammie Smith Barnes
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 2 comments![]( "Open in Gerrit")
We don't actually need this. See my comment in `web_contents_impl.cc`.
Done
This is equivalent to `render_widget_host->GetView()->GetWidgetType() == WidgetType::kFrame` (no need for the new `is_frame_widget` getter).
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 2 comments![]( "Open in Gerrit")
This occurred because WebContentsImpl::IsWidgetEmbedded() returned falseCammie Smith BarnesThis description sounds like `IsWidgetEmbedded` exists today, which is not true. Maybe reword? I also think that maybe we don't need this method (I leave a separate comment for that).
Done
Can we replace this with `GetView()->IsRenderWidgetHostViewChildFrame()`, so that we don't need the new `IsWidgetEmbedded`?
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 2 comments![]( "Open in Gerrit")
It is worth mentioning the existing OOPIF throttling mechanism and this CL builds on top of that, e.g.,
```
This CL extends the existing OOPIF throttling pipeline to support
throttling embedded main frame (GuestView and SurfaceEmbed). The
throttling state is passed through FrameConnector ->
RenderWidgetHostViewChildFrame -> blink::FrameWidget when
RWHVChildFrame::UpdateRenderThrottlingStatus() is called.
```
Done
- Adding `UpdateRenderThrottlingStatus` to `mojom::SurfaceEmbedHost` toCammie Smith BarnesCan we mention that `UpdateRenderThrottlingStatus` is a new virtual method in `blink::WebPlugin` and also describe the source of the throttling status (i.e., it originates from the parent LocalFrameView)?
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 2 comments![]( "Open in Gerrit")
// Represents the render throttling status of an embedded frame.
struct RenderThrottlingStatus {
bool is_throttled;
bool subtree_throttled;
bool display_locked;
};This is the same set of booleans used by [RemoteFrame](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/frame/remote_frame.mojom;l=191;drc=0a5e9fb6b61cd0e736d3166b99326d33b9ccea66) and [FrameWidget](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/page/widget.mojom;l=160;drc=c0f65cfcd9968553a6d669588ea8dec10d241627). I think we can make it a shared struct (in a follow up).
For now, can we reference `RemoteFrame::UpdateRenderThrottlingStatus` in a comment? (that the same set of parameters are used).
Done
// provided parameters. Successive calls replace the existing state rather
// than toggling it.What does "toggle" mean? This function signature looks stateless to me (i.e. it does not depend on the current throttling state of the child side), and so "toggle" seems redundant. Maybe delete the whole sentence?
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
UpdateRenderThrottlingStatus(RenderThrottlingStatus status);About naming, maybe `OnEmbedElementThrottlingStatusChanged`, which hints that `SurfaceEmbedHost` will decide how the state is applied to the child WebContents? For example, it may reject throttling a child WebContents when the WebContents is being screen-captured.
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
virtual bool IsRenderViewHost() const;Cammie Smith BarnesCan we avoid adding this virtual method? The RenderView concept is being removed (see comment above this class).
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
} else if (delegate_->IsEmbeddedViaSurfaceEmbed()) {What will break without this change? If it breaks `CHECK`s, can you point me to them?
Currently, an surface-embedded Blink frame does not know about itself being embeded. This line will change that (via [CreateFrameParams::is_for_nested_main_frame](https://source.chromium.org/chromium/chromium/src/+/main:content/common/frame.mojom;l=361;drc=60df9044934a2841875904f21e9673afe970da1e) set to true due to [type != kTopLevel](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=4598;drc=03b18568967f29eb7d198a513c9be98291a5ce6c)). We want to make a surface-embedded frame behave like a top-level frame as much as possible, so I am curious if we can avoid this change.
Done
Related details
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. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
I think this test does not really test state propagation? e.g., the state is set then retrieved from connector. No IPC is involved. I think we need better test coverage.
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu voted and added 5 comments![]( "Open in Gerrit")
Votes added by Keren Zhu
| Code-Review | +1 |
5 comments
context.rwhvcf = static_cast<RenderWidgetHostViewChildFrame*>(
static_cast<WebContentsImpl*>(context.child_web_contents.get())
->GetRenderWidgetHostView());
ASSERT_TRUE(context.rwhvcf);
connector->SetView(context.rwhvcf, /*allow_paint_holding=*/false);
RenderWidgetHostImpl* child_rwh = context.rwhvcf->host();
ASSERT_TRUE(child_rwh);
mojo::AssociatedRemote<blink::mojom::FrameWidgetHost> blink_frame_widget_host;
auto blink_frame_widget_host_receiver =
blink_frame_widget_host.BindNewEndpointAndPassDedicatedReceiver();
mojo::AssociatedRemote<blink::mojom::FrameWidget> blink_frame_widget;
auto blink_frame_widget_receiver =
blink_frame_widget.BindNewEndpointAndPassDedicatedReceiver();
child_rwh->BindFrameWidgetInterfaces(
std::move(blink_frame_widget_host_receiver), blink_frame_widget.Unbind());
FakeFrameWidget fake_frame_widget(std::move(blink_frame_widget_receiver));This test setup looks magical. Thanks for figuring out a way to test it.
if (auto* rwh = inner_web_contents_impl->GetPrimaryMainFrame()
->GetRenderWidgetHost()) {
rwh->SynchronizeVisualProperties();
}q: (here and around L3714) this code affects only GuestView / GuestContents, not surface embed. Are they added for completeness?
#include "content/browser/renderer_host/render_widget_host_factory.h"This can be removed?
DCHECK(ForSubframe() || widget_base_->is_embedded());nit: `widget_base_->is_embedded()` will be true for subframe, so this can be
```suggestion
DCHECK(widget_base_->is_embedded());
```
Related details
- Cammie Smith Barnes
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. |