Ozone/Wayland implement async window state changes [chromium/src : main]
Hidehiko Abe (Gerrit)
Hidehiko Abe added 6 comments![]( "Open in Gerrit")
My initial scan.
First of all, thank you for taking a look into the work!
Then, ... TBH, ...I'm wondering if we're moving towards to the same direction. Now, I replied some design questions below, and skipped detailed code reviews, because the strategy/implementation may need to be largely changed.
Could you take a look into my questions below? Also, if needed, please feel free to reach out to me over IM etc. or maybe VC for better iteration.
if (fullscreen) {I'm confused. For async model, we expect async-ness is handled in server side, not client side, so we should send the xdg API instantly.
if (IsSurfaceConfigured()) {so in the sense, this block should be removed entirely once async model is being made. We expect onWindowStateChanged is called as a reaction to the response from the server.
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {same as above.
void WaylandWindowTest::DoMinimizeTest(bool async_state) {May I ask you a bit help to make this code review more reliable by introducing a intermediate PS that just "moves" the code from TEST_* section below to here, so that "what was changed" conceptually can be highlighted by gerrity? Surely, the patch does not need to be compiled. I'd expect almost no-change, but some small parts may be updated, I think. Using tool is more reliable than diff by my eyes :-)
EXPECT_CALL(*xdg_surface->xdg_toplevel(), SetMaximized()).Times(2);I don't think we should call this twice...
Related details
- Patryk Chodur
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. |
Patryk Chodur (Gerrit)
Patryk Chodur added 4 comments![]( "Open in Gerrit")
if (fullscreen) {I'm confused. For async model, we expect async-ness is handled in server side, not client side, so we should send the xdg API instantly.
The same code is just called from [WaylandToplevelWindow::SetFullscreen](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_toplevel_window.cc;l=233;drc=00dbd87964f740a5c44bb0435e9e78ac2b5c8c67) -> [WaylandToplevelWindow::SetWindowState](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_toplevel_window.cc;l=862;drc=61757561ca2bbfee9b0d179dea1575a786749edc) -> [WaylandToplevelWindow::TriggerStateChanges](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_toplevel_window.cc;l=816-817;drc=61757561ca2bbfee9b0d179dea1575a786749edc)
if (IsSurfaceConfigured()) {so in the sense, this block should be removed entirely once async model is being made. We expect onWindowStateChanged is called as a reaction to the response from the server.
This can be cleared out, thanks for pointing that out. I fixed it in the new revision - xdg_toplevel_->SetMinimized() is used directly only with the feature flag disabled. If it is enabled, we just call `SetWindowState`.
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {same as above.
The `RequestState` is called so that all the variables related to the state are set properly. We call the OnWindowStateChanged only from configure event though.
EXPECT_CALL(*xdg_surface->xdg_toplevel(), SetMaximized()).Times(2);I don't think we should call this twice...
It is called from Restore() when exiting Fullscreen to Maximized. The whole test flow is
kNormal -> Maximize() -> kMaximized -> SetFullscreen() -> kFullScreen -> Restore() -> kMaximized
Related details
- Hidehiko Abe
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. |
Patryk Chodur (Gerrit)
Patryk Chodur added 1 comment![]( "Open in Gerrit")
void WaylandWindowTest::DoMinimizeTest(bool async_state) {May I ask you a bit help to make this code review more reliable by introducing a intermediate PS that just "moves" the code from TEST_* section below to here, so that "what was changed" conceptually can be highlighted by gerrity? Surely, the patch does not need to be compiled. I'd expect almost no-change, but some small parts may be updated, I think. Using tool is more reliable than diff by my eyes :-)
Done
Related details
- Hidehiko Abe
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. |