Create initial compositor frame sink for new renderers early [chromium/src : main]
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 3 comments![]( "Open in Gerrit")
Rakina Zata AmniWindows seems to be flaking on new test EarlyGpuChannelBrowserTest.EarlyFrameSinkUsed_InitiallyHidden,
Oh thanks, let's see if the unflaking works..
+dtapuska@ for third_party/blink review, PTAL :)
// If the widget is already shown, trigger frame sink creation. Use a
// PostTask to allow the window manager to finish attaching the Aura
// window to the display before Viz hooks up the CompositorFrameSink. IfRakina Zata AmniThis would expose us to Browser UI thread congestion. So if we can eliminate this now that would be good
Jonathan RossYeah ideally we avoid the PostTask here, but I couldn't figure out how to avoid these tab dragging failures otherwise: https://ci.chromium.org/ui/p/chromium/builders/try/linux-chromeos-rel/2799251. Do you know if there's another workaround?
Rakina Zata AmniStrange, I wouldn't expect that call to impact tab dragging. As ` CreateCompositorFrameSink` is not a sync mojom call. So timeouts are unexpected.
Jonathan RossFWIW I got this workaround from Gemini because I couldn't figure out why the test was failing. Here's what it says, does it make sense?
Why this fails on ChromeOS Tablet Mode
In ChromeOS tablet mode, snapping or floating a window is a heavy operation handled by Ash (specifically TabletModeWindowManager and SplitViewController). It involves:Changing the window's parent container in the Aura window tree.
Drastically changing the bounds.
Setting up window properties that tell Viz how to route hit testing and BeginFrames.
If you create the frame sink before this happens (by doing it synchronously instead of an async PostTask), Viz initializes the CompositorFrameSink with the initial (wrong) properties. When Ash immediately snaps the window right after, the sudden change in hierarchy and bounds can cause:Frame Eviction: The renderer might produce a frame for the old bounds which Viz rejects because Ash has already updated the expected surface ID to the new snapped bounds.
Missing BeginFrames: If the frame sink is created before the window is added to the correct snapped container, it might not be hooked up to the correct display's BeginFrameSource in time, causing the test to time out waiting for a frame.
Rakina Zata AmniIf ChromeOS tab dragging creates new CompositorFrameSinks then I hadn't known that.
Frame Eviction part of that explanation doesn't make sense. Viz might reject a frame, but that is a separate system. `viz::FrameEvictionManager` lives in the Browser process. It will "evict" the `viz::Surface` for a client when it becomes hidden. (After a limit of saved surfaces is hit.)
Jonathan RossOK after a few more Gemini consulations I think we can avoid the PostTask by doing two things:
- Checking if the window is attached when creating the widget (if it's not then we're in the middle of tab dragging and can skip the optimization)
- Handle re-parenting during drag: In tablet mode on ChromeOS, a window can be attached to a root window early but then gets re-parented during the drag operation. If the early-created frame sink persisted through this re-parenting, it caused synchronization issues and test timeouts. To fix this, the solution involved adding logic to detect when a tab-drag operation is in progress (using a window property like kTabDragInProgressKey) and explicitly destroying the frame sink in RenderWidgetHostViewAura::ParentHierarchyChanged if a drag is detected during re-parenting. This forces a fallback to the standard renderer-driven frame sink creation path, which is safe and handles the new hierarchy correctly.
Do those sound reasonable to you?
Rakina Zata AmniDisabling this during tab dragging sgtm.
It does sound like this is racy with re-parenting then. So we might need to consider a follow up to understand why this is failing
Jonathan RossHmm actually some of the tab dragging tests are still flaky after those fixes. I tried debugging but didn't really get anywhere. Since the Webium experiment is not currently enabled on ChromeOS, I think maybe it's better to just skip the optimization on ChromeOS at this point and investigate how to make this work later. WDYT?
Rakina Zata AmniIf we only needed this for ChromeOS, and it still fails. Then yeah we should limit the feature to non-CrOS for now. We could probably remove this if it wasn't needed for other platforms
Acknowledged
Related details
- Dave Tapuska
- Jonathan Ross
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 6 comments![]( "Open in Gerrit")
mojo::PendingReceiver<viz::mojom::CompositorFrameSink> initial_sink_receiver_;I'd prefer if this was wrapped in a struct like:
```
struct PendingFrameSinkChannels {
mojo::PendingRemote...
};
std::optional<PendingFrameSinkChannels> pending_frame_sink_channel_;
```
web_frame_widget->SetInitialFrameSink(Why another method, could we just roll this into InitializeCompositing?
virtual void SetInitialFrameSink(Why doesn't this exist for WebPagePopup?
mojo::PendingRemote<viz::mojom::CompositorFrameSink> initial_frame_sink_;Same idea, lets put this in a pending struct
Related details
- Jonathan Ross
- Rakina Zata Amni
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 7 comments![]( "Open in Gerrit")
Rakina Zata Amniwhy this removal?
Oops, bad rebase
mojo::PendingReceiver<viz::mojom::CompositorFrameSink> initial_sink_receiver_;I'd prefer if this was wrapped in a struct like:
```
struct PendingFrameSinkChannels {
mojo::PendingRemote...
};
std::optional<PendingFrameSinkChannels> pending_frame_sink_channel_;
```
Done
#if defined(USE_AURA)Rakina Zata AmniWhy this include?
Old change, removed
web_frame_widget->SetInitialFrameSink(Why another method, could we just roll this into InitializeCompositing?
Done
virtual void SetInitialFrameSink(Why doesn't this exist for WebPagePopup?
Now that it's combined with `InitializeCompositing()` this exists there too (although it does nothing)
mojo::PendingRemote<viz::mojom::CompositorFrameSink> initial_frame_sink_;Same idea, lets put this in a pending struct
Done
mojo::PendingReceiver<blink::mojom::blink::RenderInputRouterClient>(Rakina Zata AmniNit: `blink::mojom::RenderInputRouterClient`
Rakina Zata AmniI changed it to `mojom::blink::RenderInputRouterClient` since `blink::mojom::RenderInputRouterClient` wouldn't compile and Gemini seems to think it's not possible to use that, but let me know if it actually is possible
Done
Related details
- Dave Tapuska
- Jonathan Ross
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")
Looks like dtapuska@ is OOO so let me reassign to tkent@, and also add an IPC reviewer. PTAL :)
Related details
- Chromium IPC Reviews
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@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): toyo...@chromium.org
Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
- Takashi Toyoshima
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. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
2 comments
third_party/blink LGTM
"+services/viz/public/mojom/compositing/compositor_frame_sink.mojom-shared.h",This should be inserted at a sorted position.
Related details
- Dave Tapuska
- Jonathan Ross
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
"+services/viz/public/mojom/compositing/compositor_frame_sink.mojom-shared.h",This should be inserted at a sorted position.
- Dave Tapuska
- Jonathan Ross
- Takashi Toyoshima
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. |
Jonathan Ross (Gerrit)
Jonathan Ross added 5 comments![]( "Open in Gerrit")
// Expect that the initial channel was established successfully. Note that
// this might be established even before the navigation above, but there's no
// way to non-flakily wait for the histogram before we committed any
// navigation in the process.Further reason to not rely on histogram for testing
// The initial frame sink should be used on non-ChromeOS platforms.
// Wait for a frame to be submitted. This ensures the renderer has
// requested the frame sink.
RenderFrameSubmissionObserver observer(shell()->web_contents());
observer.WaitForAnyFrameSubmission();We should probably just make a non-ChromeOS test, that enables `features::kSendGPUChannelEarly` and validates that we successfully produce a frame
if (GetHostFrameSinkManager()->IsFrameSinkIdRegistered(GetFrameSinkId())) {Please fix this WARNING reported by autoreview issue finding: Is there a specific reason to drop the pipes if the ID is not registered yet? `CreateFrameSink` already handles buffering until the view is ready. If we reset here, the renderer (which already has its end of the pipes) will face a connection error and will have to fallback to the slow path anyway. It might be safer to just call `CreateFrameSink` and let it buffer.
Is there a specific reason to drop the pipes if the ID is not registered yet? `CreateFrameSink` already handles buffering until the view is ready. If we reset here, the renderer (which already has its end of the pipes) will face a connection error and will have to fallback to the slow path anyway. It might be safer to just call `CreateFrameSink` and let it buffer.
// TODO(crbug.com/ 496408117): Make this work on ChromeOS too.Please fix this WARNING reported by autoreview issue finding: NIT: Extra space in bug URL: 'crbug.com/ 496408117' -> 'crbug.com/496408117'
NIT: Extra space in bug URL: 'crbug.com/ 496408117' -> 'crbug.com/496408117'
<histogram name="GPU.EarlyInitialFrameSinkUsed" enum="Boolean"
expires_after="2027-04-24">UMAs for test validation are discouraged: https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#Don_t-Use-Histograms-to-Prove-Main-Logic-Correctness
Related details
- Rakina Zata Amni
- Takashi Toyoshima
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 5 comments![]( "Open in Gerrit")
// Expect that the initial channel was established successfully. Note that
// this might be established even before the navigation above, but there's no
// way to non-flakily wait for the histogram before we committed any
// navigation in the process.Further reason to not rely on histogram for testing
Hmm yeah I wonder if there are other nicer ways to test end-to-end including the renderer side without histograms for both the early GPU channel and frame sink. There shouldn't really be any observable behavior changing except the performance, which isn't really testable. I've dropped all the histogram testing now, let me know if I should add other kinds of tests.
// The initial frame sink should be used on non-ChromeOS platforms.
// Wait for a frame to be submitted. This ensures the renderer has
// requested the frame sink.
RenderFrameSubmissionObserver observer(shell()->web_contents());
observer.WaitForAnyFrameSubmission();We should probably just make a non-ChromeOS test, that enables `features::kSendGPUChannelEarly` and validates that we successfully produce a frame
Do you mean make this SpareRendererUsesEarlyChannel be non-ChromeOS-only, or something else? (there are other tests that already use RenderFrameSubmissionObserver too)
if (GetHostFrameSinkManager()->IsFrameSinkIdRegistered(GetFrameSinkId())) {Please fix this WARNING reported by autoreview issue finding: Is there a specific reason to drop the pipes if the ID is not registered yet? `CreateFrameSink` already handles buffering until the view is ready. If we reset here, the renderer (which already has its end of the pipes) will face a connection error and will have to fallback to the slow path anyway. It might be safer to just call `CreateFrameSink` and let it buffer.
Is there a specific reason to drop the pipes if the ID is not registered yet? `CreateFrameSink` already handles buffering until the view is ready. If we reset here, the renderer (which already has its end of the pipes) will face a connection error and will have to fallback to the slow path anyway. It might be safer to just call `CreateFrameSink` and let it buffer.
I think some tests were failing without the `IsFrameSinkIdRegistered()`, but I lost track of which ones. Let me run without the checks and see if they still fail.
// TODO(crbug.com/ 496408117): Make this work on ChromeOS too.Please fix this WARNING reported by autoreview issue finding: NIT: Extra space in bug URL: 'crbug.com/ 496408117' -> 'crbug.com/496408117'
NIT: Extra space in bug URL: 'crbug.com/ 496408117' -> 'crbug.com/496408117'
Done
<histogram name="GPU.EarlyInitialFrameSinkUsed" enum="Boolean"
expires_after="2027-04-24">UMAs for test validation are discouraged: https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#Don_t-Use-Histograms-to-Prove-Main-Logic-Correctness
- Jonathan Ross
- Takashi Toyoshima
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. |
Jonathan Ross (Gerrit)
Jonathan Ross added 2 comments![]( "Open in Gerrit")
// Expect that the initial channel was established successfully. Note that
// this might be established even before the navigation above, but there's no
// way to non-flakily wait for the histogram before we committed any
// navigation in the process.Rakina Zata AmniFurther reason to not rely on histogram for testing
Hmm yeah I wonder if there are other nicer ways to test end-to-end including the renderer side without histograms for both the early GPU channel and frame sink. There shouldn't really be any observable behavior changing except the performance, which isn't really testable. I've dropped all the histogram testing now, let me know if I should add other kinds of tests.
Yeah, any non-performance changes would result in existing tests failing to submit frames. We should be okay with the added ` EarlyFrameSinkUsed_InitiallyHidden` and `InitialGpuChannelBrowserTest` is enabling `kSendGPUChannelEarly`, for general coverage
// The initial frame sink should be used on non-ChromeOS platforms.
// Wait for a frame to be submitted. This ensures the renderer has
// requested the frame sink.
RenderFrameSubmissionObserver observer(shell()->web_contents());
observer.WaitForAnyFrameSubmission();Rakina Zata AmniWe should probably just make a non-ChromeOS test, that enables `features::kSendGPUChannelEarly` and validates that we successfully produce a frame
Do you mean make this SpareRendererUsesEarlyChannel be non-ChromeOS-only, or something else? (there are other tests that already use RenderFrameSubmissionObserver too)
I think the current state of `EarlyFrameSinkUsed_InitiallyHidden` should be sufficient, since `InitialGpuChannelBrowserTest` is enabling `kSendGPUChannelEarly`.
Related details
- Rakina Zata Amni
- Takashi Toyoshima
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
import "services/viz/public/mojom/compositing/compositor_frame_sink.mojom";dictionary order?
pending_remote<viz.mojom.CompositorFrameSink>? initial_frame_sink;Do you really need to make all these optional? Just struct level optional would work as these three seems to be set at once together?
Related details
- Rakina Zata Amni
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 6 comments![]( "Open in Gerrit")
InitialFrameSinkPipes initial_frame_sink_pipes_;std::optional<InitialFrameSinkPipes>
if (initial_frame_sink_pipes_.receiver.is_valid()) {initial_frame_sink_pipes_.has_value()
initial_frame_sink_pipes_.receiver.reset();initial_frame_sink_pipes_.reset();
auto initial_frame_sink_params = mojom::InitialFrameSinkParams::New();initial_frame_sink_pipes_.emplace();
initial_frame_sink_pipes_.receiver.reset();initial_frame_sink_pipes_.reset()
InitialFrameSinkPipes initial_frame_sink_pipes_;same deal here. (std::optional)... Why std::optional because it isn't valid all the time, and resetting it clears all the state.
Related details
- Kent Tamura
- Rakina Zata Amni
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 10 comments![]( "Open in Gerrit")
InitialFrameSinkPipes initial_frame_sink_pipes_;Rakina Zata Amnistd::optional<InitialFrameSinkPipes>
Done
if (initial_frame_sink_pipes_.receiver.is_valid()) {Rakina Zata Amniinitial_frame_sink_pipes_.has_value()
Done
if (GetHostFrameSinkManager()->IsFrameSinkIdRegistered(GetFrameSinkId())) {Please fix this WARNING reported by autoreview issue finding: Is there a specific reason to drop the pipes if the ID is not registered yet? `CreateFrameSink` already handles buffering until the view is ready. If we reset here, the renderer (which already has its end of the pipes) will face a connection error and will have to fallback to the slow path anyway. It might be safer to just call `CreateFrameSink` and let it buffer.
Is there a specific reason to drop the pipes if the ID is not registered yet? `CreateFrameSink` already handles buffering until the view is ready. If we reset here, the renderer (which already has its end of the pipes) will face a connection error and will have to fallback to the slow path anyway. It might be safer to just call `CreateFrameSink` and let it buffer.
I think some tests were failing without the `IsFrameSinkIdRegistered()`, but I lost track of which ones. Let me run without the checks and see if they still fail.
Looks like it's not needed after all, so keeping it removed :)
initial_frame_sink_pipes_.receiver.reset();Rakina Zata Amniinitial_frame_sink_pipes_.reset();
Done
auto initial_frame_sink_params = mojom::InitialFrameSinkParams::New();Rakina Zata Amniinitial_frame_sink_pipes_.emplace();
Done
initial_frame_sink_pipes_.receiver.reset();Rakina Zata Amniinitial_frame_sink_pipes_.reset()
Done
import "services/viz/public/mojom/compositing/compositor_frame_sink.mojom";Rakina Zata Amnidictionary order?
Done
Comments on where and how this is used
Done
pending_remote<viz.mojom.CompositorFrameSink>? initial_frame_sink;Do you really need to make all these optional? Just struct level optional would work as these three seems to be set at once together?
Right, removed the optional part of the members
same deal here. (std::optional)... Why std::optional because it isn't valid all the time, and resetting it clears all the state.
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
- Takashi Toyoshima
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 7 comments![]( "Open in Gerrit")
blink::mojom::RenderInputRouterClientInterfaceBase>mojom::RenderInputRouterClientInterfaceBase
Note the signature here will differ from the virtual definition. But this is meant to be that we use the sub-namespace blink ones on the impl files.
viz::mojom::CompositorFrameSinkClientInterfaceBase>Same..
CrossVariantMojoRemote<viz::mojom::CompositorFrameSinkInterfaceBase>viz::mojom::blink::
blink::mojom::RenderInputRouterClientInterfaceBase>Just mojom::RenderInputRouterClientInterfaceBase
viz::mojom::CompositorFrameSinkClientInterfaceBase>viz::mojom::blink::
CrossVariantMojoRemote<viz::mojom::CompositorFrameSinkInterfaceBase>This should be viz::mojom::blink::CompositorFrameSinkInterfaceBase
'third_party/blink/renderer/core/exported/web_page_popup_impl.cc',Hopefully with the viz::mojom::blink changes we can avoid everything bug web_widget.h for allowing the mojom types.
Related details
- Jonathan Ross
- Kent Tamura
- Rakina Zata Amni
- Takashi Toyoshima
Jonathan Ross (Gerrit)
Jonathan Ross added 3 comments![]( "Open in Gerrit")
if (!waiting_for_init_) {
blink_widget_->WasShown(view_->is_evicted(),
std::move(record_tab_switch_time_request));
} else {
// Delay the WasShown message until Init is called.
pending_show_params_.emplace(view_->is_evicted(),
std::move(record_tab_switch_time_request));
}Could `waiting_for_init_` cause an issue with binding the pipes in `CreateFrameSink`?
// If we created the frame sink pipes early but the widget was hidden,
// we deferred the actual creation until it becomes visible.
if (initial_frame_sink_pipes_.has_value()) {
CreateFrameSink(std::move(initial_frame_sink_pipes_->receiver),
std::move(initial_frame_sink_pipes_->client),
std::move(initial_frame_sink_pipes_->viz_rir_client));
initial_frame_sink_pipes_.reset();
}Should we have this above `SendScreenRects`? So that the pipes are given to the Renderer before it attempts to do any graphical work in response to the visual properties?
#endifNit: preference to document wha the pre-processor if for when more than a couple of lines away
`#endif // !BUILDFLAG(IS_CHROMEOS)`
Related details
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 10 comments![]( "Open in Gerrit")
if (!waiting_for_init_) {
blink_widget_->WasShown(view_->is_evicted(),
std::move(record_tab_switch_time_request));
} else {
// Delay the WasShown message until Init is called.
pending_show_params_.emplace(view_->is_evicted(),
std::move(record_tab_switch_time_request));
}Could `waiting_for_init_` cause an issue with binding the pipes in `CreateFrameSink`?
Hmm yeah I wonder. I think this deferral is more because we don't have the renderer-side objects yet right? So it makes sense to defer the WasShown until when we actually have the renderer-side objects. The early frame sink pipes themselves are only sent to the renderer when we create the widget on the renderer side so that seems fine without having to explicitly check for `waiting_for_init_`? Meanwhile the `CreateFrameSink()` call should be going to the GPU process and I think should work fine even if the renderer side is not connected yet, just like the early GPU channel?
// If we created the frame sink pipes early but the widget was hidden,
// we deferred the actual creation until it becomes visible.
if (initial_frame_sink_pipes_.has_value()) {
CreateFrameSink(std::move(initial_frame_sink_pipes_->receiver),
std::move(initial_frame_sink_pipes_->client),
std::move(initial_frame_sink_pipes_->viz_rir_client));
initial_frame_sink_pipes_.reset();
}Should we have this above `SendScreenRects`? So that the pipes are given to the Renderer before it attempts to do any graphical work in response to the visual properties?
Done
Nit: preference to document wha the pre-processor if for when more than a couple of lines away
`#endif // !BUILDFLAG(IS_CHROMEOS)`
Done
mojom::RenderInputRouterClientInterfaceBase
Note the signature here will differ from the virtual definition. But this is meant to be that we use the sub-namespace blink ones on the impl files.
Done
viz::mojom::CompositorFrameSinkClientInterfaceBase>Rakina Zata AmniSame..
Done
CrossVariantMojoRemote<viz::mojom::CompositorFrameSinkInterfaceBase>Rakina Zata Amniviz::mojom::blink::
Done
blink::mojom::RenderInputRouterClientInterfaceBase>Rakina Zata AmniJust mojom::RenderInputRouterClientInterfaceBase
Done
viz::mojom::CompositorFrameSinkClientInterfaceBase>Rakina Zata Amniviz::mojom::blink::
Done
CrossVariantMojoRemote<viz::mojom::CompositorFrameSinkInterfaceBase>This should be viz::mojom::blink::CompositorFrameSinkInterfaceBase
Done
'third_party/blink/renderer/core/exported/web_page_popup_impl.cc',Hopefully with the viz::mojom::blink changes we can avoid everything bug web_widget.h for allowing the mojom types.
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
- Takashi Toyoshima
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")
Friendly ping for reviews :)
Related details
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
struct InitialFrameSinkPipes {https://google.github.io/styleguide/cppguide.html#Declaration_Order
struct declaration should be located first before other method declarations in the same (private) scope. (though, already this file had an exception above...)
By the way, why don't you use the mojo struct directly here?
InitializeCompositing(screen_infos,
/*settings=*/nullptr, {}, {}, {});Not related to this change directly, but calling virtual method in a constructor causes a confusion. Even if a derived class implements InitializeCompositing, calling it from here would call the WebPagePopupImpl::InitializeCompositing always, not the derived class's implementation. That says, just a normal method is safe and provides the same functionality.
For the same reason, if the parent call this virtual method, this class's implementation would not be called. (because of the virtual table initialization order)
Just in case.
ditto. why we need a separate struct for the same structured objects?
Related details
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
third_party/blink LGTM
- Dave Tapuska
- Jonathan Ross
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 3 comments![]( "Open in Gerrit")
https://google.github.io/styleguide/cppguide.html#Declaration_Order
struct declaration should be located first before other method declarations in the same (private) scope. (though, already this file had an exception above...)
By the way, why don't you use the mojo struct directly here?
Moved to before the functions. These pipes are the opposite of what we sent via mojo so we need to use a separate struct.
InitializeCompositing(screen_infos,
/*settings=*/nullptr, {}, {}, {});Not related to this change directly, but calling virtual method in a constructor causes a confusion. Even if a derived class implements InitializeCompositing, calling it from here would call the WebPagePopupImpl::InitializeCompositing always, not the derived class's implementation. That says, just a normal method is safe and provides the same functionality.
For the same reason, if the parent call this virtual method, this class's implementation would not be called. (because of the virtual table initialization order)
Just in case.
Thanks, I agree that calling virtual methods in a constructor is an anti-pattern that can cause confusion. Currently this seems to work fine because WebPagePopupImpl is final and for this CL there's nothing actually changing for WebPagePopupImpl behavior too?
ditto. why we need a separate struct for the same structured objects?
This one can use the mojo struct, but I needed to move the struct to blink instead, which seems fine so I did that.
Related details
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 3 comments![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
3 comments
bool using_sync_compositing =nit: const bool is a bit nicer for compiler?
DCHECK_EQ(Prefer CHECK over DCHECK
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
Related details
- Dave Tapuska
- Jonathan Ross
- Kent Tamura
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Jonathan Ross
- Kent Tamura
- Rakina Zata Amni
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. |
Jonathan Ross (Gerrit)
Jonathan Ross voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jonathan Ross
| Code-Review | +1 |
3 comments
InitialFrameSinkPipes& operator=(InitialFrameSinkPipes&&) = default;Please fix this WARNING reported by autoreview issue finding: Since this struct has an explicit destructor and move assignment, it should also have a move constructor (e.g., `InitialFrameSinkPipes(InitialFrameSinkPipes&&) = default;`) to ensure it remains movable, especially since it is used within an `std::optional`.
Since this struct has an explicit destructor and move assignment, it should also have a move constructor (e.g., `InitialFrameSinkPipes(InitialFrameSinkPipes&&) = default;`) to ensure it remains movable, especially since it is used within an `std::optional`.
initial_frame_sink_pipes_.reset();Please fix this WARNING reported by autoreview issue finding: This reset is redundant because `CreateFrameSink` already calls `initial_frame_sink_pipes_.reset()` at the beginning of its implementation.
This reset is redundant because `CreateFrameSink` already calls `initial_frame_sink_pipes_.reset()` at the beginning of its implementation.
mojo::PendingRemote<viz::mojom::CompositorFrameSink> initial_frame_sink;Please fix this WARNING reported by autoreview issue finding: This logic for unpacking the initial frame sink pipes is identical to the one at line 1661. Please consider moving this to a small helper function within `render_frame_impl.cc` to avoid duplication.
This logic for unpacking the initial frame sink pipes is identical to the one at line 1661. Please consider moving this to a small helper function within `render_frame_impl.cc` to avoid duplication.
Related details
- Kent Tamura
- Rakina Zata Amni
Code-Coverage
Code-Owners
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 5 comments![]( "Open in Gerrit")
InitialFrameSinkPipes& operator=(InitialFrameSinkPipes&&) = default;Please fix this WARNING reported by autoreview issue finding: Since this struct has an explicit destructor and move assignment, it should also have a move constructor (e.g., `InitialFrameSinkPipes(InitialFrameSinkPipes&&) = default;`) to ensure it remains movable, especially since it is used within an `std::optional`.
Since this struct has an explicit destructor and move assignment, it should also have a move constructor (e.g., `InitialFrameSinkPipes(InitialFrameSinkPipes&&) = default;`) to ensure it remains movable, especially since it is used within an `std::optional`.
Done
initial_frame_sink_pipes_.reset();Please fix this WARNING reported by autoreview issue finding: This reset is redundant because `CreateFrameSink` already calls `initial_frame_sink_pipes_.reset()` at the beginning of its implementation.
This reset is redundant because `CreateFrameSink` already calls `initial_frame_sink_pipes_.reset()` at the beginning of its implementation.
Done
nit: const bool is a bit nicer for compiler?
Done
mojo::PendingRemote<viz::mojom::CompositorFrameSink> initial_frame_sink;Please fix this WARNING reported by autoreview issue finding: This logic for unpacking the initial frame sink pipes is identical to the one at line 1661. Please consider moving this to a small helper function within `render_frame_impl.cc` to avoid duplication.
This logic for unpacking the initial frame sink pipes is identical to the one at line 1661. Please consider moving this to a small helper function within `render_frame_impl.cc` to avoid duplication.
Done
Prefer CHECK over DCHECK
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
Done
Related details
- Kent Tamura
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 3 comments![]( "Open in Gerrit")
Rakina Zata AmniPTAL :) Not sure how to test the additions here though. Any suggestions?
Done
// In tablet mode, ConvertPointToScreen might fail and return 0,0.Rakina Zata AmniCan we file a bug for this?
(no longer neeeded)
if (!waiting_for_init_) {
blink_widget_->WasShown(view_->is_evicted(),
std::move(record_tab_switch_time_request));
} else {
// Delay the WasShown message until Init is called.
pending_show_params_.emplace(view_->is_evicted(),
std::move(record_tab_switch_time_request));
}Rakina Zata AmniCould `waiting_for_init_` cause an issue with binding the pipes in `CreateFrameSink`?
Hmm yeah I wonder. I think this deferral is more because we don't have the renderer-side objects yet right? So it makes sense to defer the WasShown until when we actually have the renderer-side objects. The early frame sink pipes themselves are only sent to the renderer when we create the widget on the renderer side so that seems fine without having to explicitly check for `waiting_for_init_`? Meanwhile the `CreateFrameSink()` call should be going to the GPU process and I think should work fine even if the renderer side is not connected yet, just like the early GPU channel?
Closing this, let me know if there are still concerns around this.
Related details
- Kent Tamura
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 Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Kent Tamura
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
102 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/gpu/initial_gpu_channel_browsertest.cc
Insertions: 13, Deletions: 27.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/browser/renderer_host/render_widget_host_impl.cc
Insertions: 6, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/platform/widget/widget_base.cc
Insertions: 6, Deletions: 7.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/browser/renderer_host/render_widget_host_impl.h
Insertions: 2, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/renderer/render_frame_impl.cc
Insertions: 20, Deletions: 18.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/platform/widget/widget_base.h
Insertions: 0, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Create initial compositor frame sink for new renderers early
We want to create compositor frame sink as soon as possible by
sending mojo pipes needed by the renderer for frame sink communication
during renderer initialization. This allows the renderer to get the
frame sink quickly without having the renderer request for it and
wait for a renderer -> browser roundtrip, which can be especially
slow when the browser main thread is busy, e.g. during startup.
This CL adds a new browser -> renderer SetInitialFrameSink path that
does just that by creating the message pipes and calling
CreateCompositorFrameSink as soon as possible on the browser process,
sending the renderer end of the pipes to SetInitialFrameSink. This
new path makes the renderer use the browser-sent initial frame
sink pipes instead of creating the pipes in the renderer and r
requesting the browser to connect it.
See doc: https://docs.google.com/document/d/1WumYcuGWjtS8EY6EWXRa1idV3MWbpLTHNWdAHsGiFjk/edit?resourcekey=0-ztQwyWhDmc7mFZTEVJZLHw&tab=t.0#heading=h.vczmpwfwb81x
- M components/viz/test/test_frame_sink_manager.cc
- M components/viz/test/test_frame_sink_manager.h
- M content/browser/gpu/initial_gpu_channel_browsertest.cc
- M content/browser/renderer_host/render_widget_host_impl.cc
- M content/browser/renderer_host/render_widget_host_impl.h
- M content/common/frame.mojom
- M content/renderer/render_frame_impl.cc
- M third_party/blink/public/mojom/widget/platform_widget.mojom
- M third_party/blink/public/web/DEPS
- M third_party/blink/public/web/web_widget.h
- M third_party/blink/renderer/core/exported/DEPS
- M third_party/blink/renderer/core/exported/web_page_popup_impl.cc
- M third_party/blink/renderer/core/exported/web_page_popup_impl.h
- M third_party/blink/renderer/core/frame/DEPS
- M third_party/blink/renderer/core/frame/frame_test_helpers.cc
- 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/platform/widget/widget_base.cc
- M third_party/blink/renderer/platform/widget/widget_base.h
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
- M tools/metrics/histograms/metadata/gpu/histograms.xml
Code-Review: +1 by Jonathan Ross, +1 by Dave Tapuska, +1 by Takashi Toyoshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |