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. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 4 comments![]( "Open in Gerrit")
int64_t fullscreen_display_id = display::kInvalidDisplayId);nit: to avoid accident, could you pass the value always?
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {I'm confused. If I understand the consensus on the design doc correctly, we should keep this, because we just want to make this call to xdg_toplevel_ invocation, and do nothing about state management below.
Or, am i misunderstanding the proposal somehow?
SetWindowState(PlatformWindowState::kMaximized);same. We should just call xdg_toplevel_ call here only?
void WaylandToplevelWindow::Minimize() {same here, too.
Ok to keep remembering the minimization request for handling on response from the compositor.
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 1 comment![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {I'm confused. If I understand the consensus on the design doc correctly, we should keep this, because we just want to make this call to xdg_toplevel_ invocation, and do nothing about state management below.
Or, am i misunderstanding the proposal somehow?
I modified the `SetWindowState` and `TriggerStateChanges` to just check whether the desired state is active and call the `xdg_toplevel_` functions. If you prefer, I can inline them here to make it clearer that we don't want any additional logic.
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. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 1 comment![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {Patryk ChodurI'm confused. If I understand the consensus on the design doc correctly, we should keep this, because we just want to make this call to xdg_toplevel_ invocation, and do nothing about state management below.
Or, am i misunderstanding the proposal somehow?
I modified the `SetWindowState` and `TriggerStateChanges` to just check whether the desired state is active and call the `xdg_toplevel_` functions. If you prefer, I can inline them here to make it clearer that we don't want any additional logic.
One of the keys of the redesign is to let compositor decide the state, and the decision is async operation. So, if Chrome wants to make a fullscreen, it should send the request to the compositor always, IIUC?
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 1 comment![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {Patryk ChodurI'm confused. If I understand the consensus on the design doc correctly, we should keep this, because we just want to make this call to xdg_toplevel_ invocation, and do nothing about state management below.
Or, am i misunderstanding the proposal somehow?
Hidehiko AbeI modified the `SetWindowState` and `TriggerStateChanges` to just check whether the desired state is active and call the `xdg_toplevel_` functions. If you prefer, I can inline them here to make it clearer that we don't want any additional logic.
One of the keys of the redesign is to let compositor decide the state, and the decision is async operation. So, if Chrome wants to make a fullscreen, it should send the request to the compositor always, IIUC?
Right, but the functions `Window::SetFullscreen`, `Window::Maximize`, `Window::Minimize` and `Window::Restore` (declared [here](https://source.chromium.org/chromium/chromium/src/+/main:ui/platform_window/platform_window.h;l=85-88;drc=d503f89588de845f4903558934b7513747b1be95)), which `WaylandToplevelWindow::SetFullscreen`, `WaylandToplevelWindow::Maximize`, `WaylandToplevelWindow::Minimize` and `WaylandToplevelWindow::Restore` override, are defined not as the direct compositor counterparts, but rather as browser-level operations focused on getting to the desired window state.
When the `Window::Maximize` is called, the browser needs to decide what Wayland/XDG Shell operations to call. When the window is minimized prior to maximizing, it'll unminimize the window before maximizing. When the window is in fullscreen mode, it'll close the fullscreen before maximizing. This translation is necessary at some level, as, for example, the exact Restore operation is implemented differently on each platform, but the high level behavior is the same on every platform.
This still enables the compositor to be the source of truth and decide the state. With the changes in this CL we decide which XDG Shell functions to call based on the stored window state, but read the window state and act on any changes directly from the Configure event. Prior to changes in this CL, we called the XDG Shell functions and just assumed the browser switched to the desired state without waiting for any Configure event. We also did some logic (like storing what state should the restore operation switch to) in the request function call, which meant we wouldn't process any of that logic for compositor-initiated changes.
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 voted and added 8 comments![]( "Open in Gerrit")
Votes added by Patryk Chodur
| Commit-Queue | +1 |
8 comments
int64_t fullscreen_display_id_ = display::kInvalidDisplayId;This was cleared on every Configure call, so it essentially acted like a function variable, but stored globally.
int64_t fullscreen_display_id = display::kInvalidDisplayId);nit: to avoid accident, could you pass the value always?
Done. Should we also have explicit parameter for `SetWindowState`?
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {Patryk ChodurI'm confused. If I understand the consensus on the design doc correctly, we should keep this, because we just want to make this call to xdg_toplevel_ invocation, and do nothing about state management below.
Or, am i misunderstanding the proposal somehow?
Hidehiko AbeI modified the `SetWindowState` and `TriggerStateChanges` to just check whether the desired state is active and call the `xdg_toplevel_` functions. If you prefer, I can inline them here to make it clearer that we don't want any additional logic.
Patryk ChodurOne of the keys of the redesign is to let compositor decide the state, and the decision is async operation. So, if Chrome wants to make a fullscreen, it should send the request to the compositor always, IIUC?
Right, but the functions `Window::SetFullscreen`, `Window::Maximize`, `Window::Minimize` and `Window::Restore` (declared [here](https://source.chromium.org/chromium/chromium/src/+/main:ui/platform_window/platform_window.h;l=85-88;drc=d503f89588de845f4903558934b7513747b1be95)), which `WaylandToplevelWindow::SetFullscreen`, `WaylandToplevelWindow::Maximize`, `WaylandToplevelWindow::Minimize` and `WaylandToplevelWindow::Restore` override, are defined not as the direct compositor counterparts, but rather as browser-level operations focused on getting to the desired window state.
When the `Window::Maximize` is called, the browser needs to decide what Wayland/XDG Shell operations to call. When the window is minimized prior to maximizing, it'll unminimize the window before maximizing. When the window is in fullscreen mode, it'll close the fullscreen before maximizing. This translation is necessary at some level, as, for example, the exact Restore operation is implemented differently on each platform, but the high level behavior is the same on every platform.
This still enables the compositor to be the source of truth and decide the state. With the changes in this CL we decide which XDG Shell functions to call based on the stored window state, but read the window state and act on any changes directly from the Configure event. Prior to changes in this CL, we called the XDG Shell functions and just assumed the browser switched to the desired state without waiting for any Configure event. We also did some logic (like storing what state should the restore operation switch to) in the request function call, which meant we wouldn't process any of that logic for compositor-initiated changes.
I changed the implementation to call the XdgToplevel functions from `WaylandToplevelWindow::SetFullscreen`, `WaylandToplevelWindow::Maximize`, `WaylandToplevelWindow::Minimize` and `WaylandToplevelWindow::Restore`, but keeping the current logic for evaluating which `XdgToplevel` functions to call.
if (fullscreen) {Patryk ChodurI'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)
Not relevant anymore, it's not implemented directly in the method.
same. We should just call xdg_toplevel_ call here only?
Now we call it directly.
same here, too.
Ok to keep remembering the minimization request for handling on response from the compositor.
Done
Patryk Chodurso 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`.
Not relevant anymore - the implementation was changed to a one that calls the `XdgToplevel` functions immediately.
if (base::FeatureList::IsEnabled(features::kAsyncFullscreenWindowState)) {Patryk Chodursame 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.
Changed the implementation, it's not relevant anymore.
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")
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
Resolving for reasons described above. The Maximize is called first on `Maximize()`, and then on `Restore()` to make sure we're in the maximized state.
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 3 comments![]( "Open in Gerrit")
if (!xdg_toplevel_) {
return;
}nit: this was done at L274.
float new_ui_scale = connection_->window_manager()->DetermineUiScale();
if (state.window_scale == new_scale && state.ui_scale == new_ui_scale) {
return;
}clarification: is this related to async work...? Could you elaborate details in the commit message?
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. |