x11: Fix regression in _NET_WM_STATE_FULLSCREEN changes handling

crrev.com/c/1769450 removed X11-specific DWTHX11::{Set,Is}Fullscreen()
overrides in favor of DWTHPlatform versions, making chrome to enter/exit
browser fullscreen mode both when it is requested explicitly by the user (e.g:
Using F11 keybinding) as well as when it was requested by the other process,
e.g: custom WM keybinding to toggle _NET_WM_STATE_FULLSCREEN window state.
Such functionality change can be considered a regression as it violate EWMH
guidelines, which states:

> When a window-manager tells a window to become 'fullscreen', it is not
> telling the window to make a sub-selection of its contents fullscreen, but
> instead optimize the whole application for fullscreen usage. Window
> decorations (e.g. borders) should be hidden, but the functionalily of the
> application should not change.

This CL restores the previous behavior and test expectations. Additionally this
fixes an issue in window bounds tracking while it is in browser fullscreen mode
(but the X11 window is not in fullscreen state). Such issue was already present
before this regression has been introduced.

Bug: 1035772
Change-Id: Ief8fba7df2c5d2466da038c2b9edf1d928261c8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1989049Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Nick Yamane <nickdiego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#731721}

ui/platform_window/x11/x11_window.cc

@@ -248,13 +248,14 @@ void X11Window::ToggleFullscreen() {
   // about state changes asynchronously, which leads to a wrong return value in
   // DesktopWindowTreeHostPlatform::IsFullscreen, for example, and media
   // files can never be set to fullscreen. Wayland does the same.
+  auto new_state = PlatformWindowState::kNormal;
   if (fullscreen)
-    state_ = PlatformWindowState::kFullScreen;
+    new_state = PlatformWindowState::kFullScreen;
   else if (IsMaximized())
-    state_ = PlatformWindowState::kMaximized;
-  else
-    state_ = PlatformWindowState::kNormal;
+    new_state = PlatformWindowState::kMaximized;
 
+  bool was_fullscreen = IsFullscreen();
+  state_ = new_state;
   SetFullscreen(fullscreen);
 
   if (unmaximize_and_remaximize)
@@ -273,7 +274,15 @@ void X11Window::ToggleFullscreen() {
     SetRestoredBoundsInPixels(bounds_in_pixels);
     bounds_in_pixels = display.bounds();
   } else {
-    bounds_in_pixels = GetRestoredBoundsInPixels();
+    // Exiting "browser fullscreen mode", but the X11 window is not necessarily
+    // in fullscreen state (e.g: a WM keybinding might have been used to toggle
+    // fullscreen state). So check whether the window is in fullscreen state
+    // before trying to restore its bounds (saved before entering in browser
+    // fullscreen mode).
+    if (was_fullscreen)
+      bounds_in_pixels = GetRestoredBoundsInPixels();
+    else
+      SetRestoredBoundsInPixels({});
   }
   // Do not go through SetBounds as long as it adjusts bounds and sets them to X
   // Server. Instead, we just store the bounds and notify the client that the
@@ -525,21 +534,43 @@ void X11Window::OnXWindowCreated() {
 }
 
 void X11Window::OnXWindowStateChanged() {
-  // Propagate the window state information to the client. Note that the order
-  // of checks is important here, because window can have several properties
-  // at the same time.
-  PlatformWindowState old_state = state_;
-  if (IsMinimized()) {
-    state_ = PlatformWindowState::kMinimized;
-  } else if (IsFullscreen()) {
-    state_ = PlatformWindowState::kFullScreen;
-  } else if (IsMaximized()) {
-    state_ = PlatformWindowState::kMaximized;
-  } else {
-    state_ = PlatformWindowState::kNormal;
-  }
+  // Determine the new window state information to be propagated to the client.
+  // Note that the order of checks is important here, because window can have
+  // several properties at the same time.
+  auto new_state = PlatformWindowState::kNormal;
+  if (IsMinimized())
+    new_state = PlatformWindowState::kMinimized;
+  else if (IsFullscreen())
+    new_state = PlatformWindowState::kFullScreen;
+  else if (IsMaximized())
+    new_state = PlatformWindowState::kMaximized;
+
+  // fullscreen state is set syschronously at ToggleFullscreen() and must be
+  // kept and propagated to the client only when explicitly requested by upper
+  // layers, as it means we are in "browser fullscreen mode" (where
+  // decorations, omnibar, buttons, etc are hidden), which is different from
+  // the case where the request comes from the window manager (or any other
+  // process), handled by this method. In this case, we follow EWMH guidelines:
+  // Optimize the whole application for fullscreen usage. Window decorations
+  // (e.g. borders) should be hidden, but the functionalily of the application
+  // should not change. Further details:
+  // https://specifications.freedesktop.org/wm-spec/wm-spec-1.3.html
+  bool browser_fullscreen_mode = state_ == PlatformWindowState::kFullScreen;
+  bool window_fullscreen_mode = new_state == PlatformWindowState::kFullScreen;
+  // So, we ignore fullscreen state transitions in 2 cases:
+  // 1. If |new_state| is kFullScreen but |state_| is not, which means the
+  // fullscreen request is coming from an external process. So the browser
+  // window must occupies the entire screen but not transitioning to browser
+  // fullscreen mode.
+  // 2. if |state_| is kFullScreen but |new_state| is not, we have been
+  // requested to exit fullscreen by other process (e.g: via WM keybinding),
+  // in this case we must keep on "browser fullscreen mode" bug the platform
+  // window gets back to its previous state (e.g: unmaximized, tiled in TWMs,
+  // etc).
+  if (window_fullscreen_mode != browser_fullscreen_mode)
+    return;
 
-  if (restored_bounds_in_pixels_.IsEmpty()) {
+  if (GetRestoredBoundsInPixels().IsEmpty()) {
     if (IsMaximized()) {
       // The request that we become maximized originated from a different
       // process. |bounds_in_pixels_| already contains our maximized bounds. Do
@@ -554,8 +585,10 @@ void X11Window::OnXWindowStateChanged() {
     SetRestoredBoundsInPixels(gfx::Rect());
   }
 
-  if (old_state != state_)
+  if (new_state != state_) {
+    state_ = new_state;
     platform_window_delegate_->OnWindowStateChanged(state_);
+  }
 }
 
 void X11Window::OnXWindowDamageEvent(const gfx::Rect& damage_rect) {

ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc

@@ -320,6 +320,8 @@ TEST_F(DesktopWindowTreeHostX11Test, WindowManagerTogglesFullscreen) {
   if (!ui::WmSupportsHint(gfx::GetAtom("_NET_WM_STATE_FULLSCREEN")))
     return;
 
+  Display* display = gfx::GetXDisplay();
+
   std::unique_ptr<Widget> widget = CreateWidget(new ShapedWidgetDelegate());
   auto* non_client_view = static_cast<ShapedNonClientFrameView*>(
       widget->non_client_view()->frame_view());
@@ -343,10 +345,8 @@ TEST_F(DesktopWindowTreeHostX11Test, WindowManagerTogglesFullscreen) {
   EXPECT_FALSE(non_client_view->GetAndResetLayoutRequest());
 
   // Emulate the window manager exiting fullscreen via a window manager
-  // accelerator key. It should affect the widget's fullscreen state.
+  // accelerator key.
   {
-    Display* display = gfx::GetXDisplay();
-
     XEvent xclient;
     memset(&xclient, 0, sizeof(xclient));
     xclient.type = ClientMessage;
@@ -364,6 +364,30 @@ TEST_F(DesktopWindowTreeHostX11Test, WindowManagerTogglesFullscreen) {
     WMStateWaiter waiter(xid, "_NET_WM_STATE_FULLSCREEN", false);
     waiter.Wait();
   }
+  // Ensure it continues in browser fullscreen mode and bounds are restored to
+  // |initial_bounds|.
+  EXPECT_TRUE(widget->IsFullscreen());
+  EXPECT_EQ(initial_bounds.ToString(),
+            widget->GetWindowBoundsInScreen().ToString());
+
+  // Emulate window resize (through X11 configure events) while in browser
+  // fullscreen mode and ensure bounds are tracked correctly.
+  initial_bounds.set_size({400, 400});
+  {
+    XWindowChanges changes = {0};
+    changes.width = initial_bounds.width();
+    changes.height = initial_bounds.height();
+    XConfigureWindow(display, xid, CWHeight | CWWidth, &changes);
+    // Ensure that the task which is posted when a window is resized is run.
+    base::RunLoop().RunUntilIdle();
+  }
+  EXPECT_TRUE(widget->IsFullscreen());
+  EXPECT_EQ(initial_bounds.ToString(),
+            widget->GetWindowBoundsInScreen().ToString());
+
+  // Calling Widget::SetFullscreen(false) should clear the widget's fullscreen
+  // state and clean things up.
+  widget->SetFullscreen(false);
   EXPECT_FALSE(widget->IsFullscreen());
   EXPECT_EQ(initial_bounds.ToString(),
             widget->GetWindowBoundsInScreen().ToString());