Fix and enable ash browser test

BrowserNonClientFrameViewAshTest.HeaderVisibilityInOverviewAndSplitview

This test was disabled due to flake (classic Ash), which I can't repro
now. But while it was disabled, it quit passing because some updates
to the test broke it, and because the home launcher now means that
tablet mode affects the visibility of caption buttons. This fixes the
test for all configurations including multi process Mash.

Bug: 818170
Change-Id: Ia0a029f52872b84f6ce6d37c1e433719c243eb56
Reviewed-on: https://chromium-review.googlesource.com/c/1327529Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609831}

ash/public/interfaces/shell_test_api.mojom

@@ -16,11 +16,14 @@ interface ShellTestApi {
   // controller. In tablet mode, enables the virtual keyboard.
   EnableVirtualKeyboard() => ();
 
-  // Tells the SplitViewController to snap the given window to the left.
-  // The client name is used to find the client's WindowTree.
-  SnapWindowInSplitView(string client_name, uint64 window_id) => ();
+  // Tells the SplitViewController to snap the given window to the left or
+  // right. The client name is used to find the client's WindowTree.
+  SnapWindowInSplitView(string client_name, uint64 window_id, bool left) => ();
 
   // Fullscreens the active window, as if the user had pressed the hardware
   // fullscreen button.
   ToggleFullscreen() => ();
+
+  // Enters or exits overview mode.
+  ToggleOverviewMode() => ();
 };

ash/shell_test_api.cc

@@ -11,6 +11,7 @@
 #include "ash/shell.h"
 #include "ash/system/power/backlights_forced_off_setter.h"
 #include "ash/system/power/power_button_controller.h"
+#include "ash/wm/overview/window_selector_controller.h"
 #include "ash/wm/splitview/split_view_controller.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/ws/window_service_owner.h"
@@ -88,6 +89,7 @@ void ShellTestApi::EnableVirtualKeyboard(EnableVirtualKeyboardCallback cb) {
 
 void ShellTestApi::SnapWindowInSplitView(const std::string& client_name,
                                          ws::Id window_id,
+                                         bool left,
                                          SnapWindowInSplitViewCallback cb) {
   auto* window_service = shell_->window_service_owner()->window_service();
   aura::Window* window = nullptr;
@@ -98,8 +100,9 @@ void ShellTestApi::SnapWindowInSplitView(const std::string& client_name,
     }
   }
   DCHECK(window);
-  shell_->split_view_controller()->SnapWindow(window,
-                                              ash::SplitViewController::LEFT);
+  shell_->split_view_controller()->SnapWindow(
+      window,
+      left ? ash::SplitViewController::LEFT : ash::SplitViewController::RIGHT);
   shell_->split_view_controller()->FlushForTesting();
   std::move(cb).Run();
 }
@@ -109,4 +112,9 @@ void ShellTestApi::ToggleFullscreen(ToggleFullscreenCallback cb) {
   std::move(cb).Run();
 }
 
+void ShellTestApi::ToggleOverviewMode(ToggleOverviewModeCallback cb) {
+  shell_->window_selector_controller()->ToggleOverview();
+  std::move(cb).Run();
+}
+
 }  // namespace ash

ash/shell_test_api.h

@@ -57,8 +57,10 @@ class ShellTestApi : public mojom::ShellTestApi {
   void EnableVirtualKeyboard(EnableVirtualKeyboardCallback cb) override;
   void SnapWindowInSplitView(const std::string& client_name,
                              ws::Id window_id,
+                             bool left,
                              SnapWindowInSplitViewCallback cb) override;
   void ToggleFullscreen(ToggleFullscreenCallback cb) override;
+  void ToggleOverviewMode(ToggleOverviewModeCallback cb) override;
 
  private:
   Shell* shell_;  // not owned

chrome/browser/ui/ash/system_tray_client.cc

@@ -6,7 +6,7 @@
 
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/public/interfaces/constants.mojom.h"
-#include "ash/shell.h"
+#include "ash/shell.h"  // mash-ok
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

@@ -19,9 +19,9 @@
 #include "ash/public/cpp/window_properties.h"
 #include "ash/public/interfaces/constants.mojom.h"
 #include "ash/public/interfaces/shell_test_api.mojom.h"
-#include "ash/shell.h"
-#include "ash/wm/overview/window_selector_controller.h"
-#include "ash/wm/splitview/split_view_controller.h"
+#include "ash/shell.h"                                   // mash-ok
+#include "ash/wm/overview/window_selector_controller.h"  // mash-ok
+#include "ash/wm/splitview/split_view_controller.h"      // mash-ok
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"  // mash-ok
 #include "base/run_loop.h"
 #include "base/scoped_observer.h"
@@ -132,6 +132,20 @@ void ExitFullscreenModeAndWait(BrowserView* browser_view) {
   waiter.Wait();
 }
 
+void ToggleOverview() {
+  if (features::IsUsingWindowService()) {
+    ash::mojom::ShellTestApiPtr shell_test_api;
+    content::ServiceManagerConnection::GetForProcess()
+        ->GetConnector()
+        ->BindInterface(ash::mojom::kServiceName, &shell_test_api);
+    base::RunLoop run_loop;
+    shell_test_api->ToggleOverviewMode(run_loop.QuitClosure());
+    run_loop.Run();
+  } else {
+    ash::Shell::Get()->window_selector_controller()->ToggleOverview();
+  }
+}
+
 BrowserNonClientFrameViewAsh* GetFrameViewAsh(BrowserView* browser_view) {
   // We know we're using Ash, so static cast.
   auto* frame_view = static_cast<BrowserNonClientFrameViewAsh*>(
@@ -1140,9 +1154,8 @@ IN_PROC_BROWSER_TEST_P(BrowserNonClientFrameViewAshTest, TopViewInset) {
   EXPECT_EQ(0, window->GetProperty(aura::client::kTopViewInset));
 }
 
-// Disabled due to high flake rate; https://crbug.com/818170.
 IN_PROC_BROWSER_TEST_P(BrowserNonClientFrameViewAshTest,
-                       DISABLED_HeaderVisibilityInOverviewAndSplitview) {
+                       HeaderVisibilityInOverviewAndSplitview) {
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser());
   Widget* widget = browser_view->GetWidget();
   BrowserNonClientFrameViewAsh* frame_view = GetFrameViewAsh(browser_view);
@@ -1154,10 +1167,9 @@ IN_PROC_BROWSER_TEST_P(BrowserNonClientFrameViewAshTest,
 
   // Test that the header is invisible for the browser window in overview mode
   // and visible when not in overview mode.
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey, true);
+  ToggleOverview();
   EXPECT_FALSE(frame_view->caption_button_container_->visible());
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey,
-                                            false);
+  ToggleOverview();
   EXPECT_TRUE(frame_view->caption_button_container_->visible());
 
   // Create another browser window.
@@ -1169,61 +1181,96 @@ IN_PROC_BROWSER_TEST_P(BrowserNonClientFrameViewAshTest,
   AddBlankTabAndShow(browser2);
   BrowserView* browser_view2 = BrowserView::GetBrowserViewForBrowser(browser2);
   Widget* widget2 = browser_view2->GetWidget();
-  BrowserNonClientFrameViewAsh* frame_view2 =
-      static_cast<BrowserNonClientFrameViewAsh*>(
-          widget2->non_client_view()->frame_view());
+  BrowserNonClientFrameViewAsh* frame_view2 = GetFrameViewAsh(browser_view2);
   widget2->GetNativeWindow()->SetProperty(
       aura::client::kResizeBehaviorKey,
       ws::mojom::kResizeBehaviorCanMaximize |
           ws::mojom::kResizeBehaviorCanResize);
 
-  // Test that when one browser window is snapped, the header is visible for the
-  // snapped browser window, but invisible for the browser window still in
-  // overview mode.
-  ASSERT_NO_FATAL_FAILURE(test::SetAndWaitForTabletMode(true));
-  ash::Shell* shell = ash::Shell::Get();
-  ash::SplitViewController* split_view_controller =
-      shell->split_view_controller();
-  split_view_controller->BindRequest(
-      mojo::MakeRequest(&frame_view->split_view_controller_));
-  split_view_controller->BindRequest(
-      mojo::MakeRequest(&frame_view2->split_view_controller_));
-  split_view_controller->AddObserver(
-      frame_view->CreateInterfacePtrForTesting());
-  split_view_controller->AddObserver(
-      frame_view2->CreateInterfacePtrForTesting());
-  frame_view->split_view_controller_.FlushForTesting();
-  frame_view2->split_view_controller_.FlushForTesting();
-
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey, true);
-  frame_view2->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey,
-                                             true);
-  split_view_controller->SnapWindow(widget->GetNativeWindow(),
-                                    ash::SplitViewController::LEFT);
-  frame_view->split_view_controller_.FlushForTesting();
-  frame_view2->split_view_controller_.FlushForTesting();
-  EXPECT_TRUE(frame_view->caption_button_container_->visible());
-  EXPECT_FALSE(frame_view2->caption_button_container_->visible());
+  if (features::IsUsingWindowService()) {
+    // Test that when one browser window is snapped, the header is visible for
+    // the snapped browser window, but invisible for the browser window still in
+    // overview mode.
+    ToggleOverview();
 
-  // When both browser windows are snapped, the headers are both visible.
-  split_view_controller->SnapWindow(widget2->GetNativeWindow(),
-                                    ash::SplitViewController::RIGHT);
-  frame_view->split_view_controller_.FlushForTesting();
-  frame_view2->split_view_controller_.FlushForTesting();
-  EXPECT_TRUE(frame_view->caption_button_container_->visible());
-  EXPECT_TRUE(frame_view2->caption_button_container_->visible());
-
-  // Toggle overview mode while splitview mode is active. Test that the header
-  // is visible for the snapped browser window but not for the other browser
-  // window in overview mode.
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey,
-                                            false);
-  frame_view2->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey,
-                                             true);
-  frame_view->split_view_controller_.FlushForTesting();
-  frame_view2->split_view_controller_.FlushForTesting();
-  EXPECT_TRUE(frame_view->caption_button_container_->visible());
-  EXPECT_FALSE(frame_view2->caption_button_container_->visible());
+    ash::mojom::ShellTestApiPtr shell_test_api;
+    content::ServiceManagerConnection::GetForProcess()
+        ->GetConnector()
+        ->BindInterface(ash::mojom::kServiceName, &shell_test_api);
+
+    {
+      base::RunLoop run_loop;
+      shell_test_api->SnapWindowInSplitView(content::mojom::kBrowserServiceName,
+                                            frame_view->GetServerWindowId(),
+                                            true, run_loop.QuitClosure());
+      run_loop.Run();
+    }
+
+    EXPECT_TRUE(frame_view->caption_button_container_->visible());
+    EXPECT_FALSE(frame_view2->caption_button_container_->visible());
+
+    // When both browser windows are snapped, the headers are both visible.
+    {
+      base::RunLoop run_loop;
+      shell_test_api->SnapWindowInSplitView(content::mojom::kBrowserServiceName,
+                                            frame_view2->GetServerWindowId(),
+                                            false, run_loop.QuitClosure());
+      run_loop.Run();
+    }
+
+    EXPECT_TRUE(frame_view->caption_button_container_->visible());
+    EXPECT_TRUE(frame_view2->caption_button_container_->visible());
+
+    // Toggle overview mode while splitview mode is active. Test that the header
+    // is visible for the snapped browser window but not for the other browser
+    // window in overview mode.
+    ToggleOverview();
+    EXPECT_TRUE(frame_view->caption_button_container_->visible());
+    EXPECT_FALSE(frame_view2->caption_button_container_->visible());
+  } else {
+    // Test that when one browser window is snapped, the header is visible for
+    // the snapped browser window, but invisible for the browser window still in
+    // overview mode.
+    ash::Shell* shell = ash::Shell::Get();
+    ash::SplitViewController* split_view_controller =
+        shell->split_view_controller();
+    split_view_controller->BindRequest(
+        mojo::MakeRequest(&frame_view->split_view_controller_));
+    split_view_controller->BindRequest(
+        mojo::MakeRequest(&frame_view2->split_view_controller_));
+    split_view_controller->AddObserver(
+        frame_view->CreateInterfacePtrForTesting());
+    split_view_controller->AddObserver(
+        frame_view2->CreateInterfacePtrForTesting());
+    frame_view->split_view_controller_.FlushForTesting();
+    frame_view2->split_view_controller_.FlushForTesting();
+
+    ToggleOverview();
+    split_view_controller->SnapWindow(widget->GetNativeWindow(),
+                                      ash::SplitViewController::LEFT);
+    frame_view->split_view_controller_.FlushForTesting();
+    frame_view2->split_view_controller_.FlushForTesting();
+    EXPECT_TRUE(frame_view->caption_button_container_->visible());
+    EXPECT_FALSE(frame_view2->caption_button_container_->visible());
+
+    // When both browser windows are snapped, the headers are both visible.
+    split_view_controller->SnapWindow(widget2->GetNativeWindow(),
+                                      ash::SplitViewController::RIGHT);
+    frame_view->split_view_controller_.FlushForTesting();
+    frame_view2->split_view_controller_.FlushForTesting();
+    EXPECT_TRUE(frame_view->caption_button_container_->visible());
+    EXPECT_TRUE(frame_view2->caption_button_container_->visible());
+
+    // Toggle overview mode while splitview mode is active. Test that the header
+    // is visible for the snapped browser window but not for the other browser
+    // window in overview mode.
+    ToggleOverview();
+    frame_view->split_view_controller_.FlushForTesting();
+    frame_view2->split_view_controller_.FlushForTesting();
+
+    EXPECT_TRUE(frame_view->caption_button_container_->visible());
+    EXPECT_FALSE(frame_view2->caption_button_container_->visible());
+  }
 }
 
 // Regression test for https://crbug.com/879851.
@@ -1257,7 +1304,6 @@ IN_PROC_BROWSER_TEST_P(BrowserNonClientFrameViewAshTest,
   params.initial_show_state = ui::SHOW_STATE_DEFAULT;
   Browser* browser = new Browser(params);
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
-  BrowserNonClientFrameViewAsh* frame_view = GetFrameViewAsh(browser_view);
   ImmersiveModeController* immersive_mode_controller =
       browser_view->immersive_mode_controller();
   aura::Window* window = browser->window()->GetNativeWindow();
@@ -1287,7 +1333,7 @@ IN_PROC_BROWSER_TEST_P(BrowserNonClientFrameViewAshTest,
 
   // The kTopViewInset is the same as in overview mode.
   const int inset_normal = window->GetProperty(aura::client::kTopViewInset);
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey, true);
+  ToggleOverview();
   const int inset_in_overview_mode =
       window->GetProperty(aura::client::kTopViewInset);
   EXPECT_EQ(inset_normal, inset_in_overview_mode);
@@ -1356,10 +1402,9 @@ IN_PROC_BROWSER_TEST_P(HomeLauncherBrowserNonClientFrameViewAshTest,
   ASSERT_NO_FATAL_FAILURE(test::SetAndWaitForTabletMode(true));
   EXPECT_FALSE(frame_view->caption_button_container_->visible());
 
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey, true);
+  ToggleOverview();
   EXPECT_FALSE(frame_view->caption_button_container_->visible());
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey,
-                                            false);
+  ToggleOverview();
   EXPECT_FALSE(frame_view->caption_button_container_->visible());
 
   ASSERT_NO_FATAL_FAILURE(test::SetAndWaitForTabletMode(false));
@@ -1377,6 +1422,7 @@ IN_PROC_BROWSER_TEST_P(HomeLauncherBrowserNonClientFrameViewAshTest,
   params.initial_show_state = ui::SHOW_STATE_DEFAULT;
   Browser* browser = new Browser(params);
   ASSERT_TRUE(browser->is_app());
+  browser->window()->Show();
 
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
   BrowserNonClientFrameViewAsh* frame_view = GetFrameViewAsh(browser_view);
@@ -1387,10 +1433,9 @@ IN_PROC_BROWSER_TEST_P(HomeLauncherBrowserNonClientFrameViewAshTest,
   EXPECT_TRUE(frame_view->caption_button_container_->visible());
 
   // However, overview mode does.
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey, true);
+  ToggleOverview();
   EXPECT_FALSE(frame_view->caption_button_container_->visible());
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey,
-                                            false);
+  ToggleOverview();
   EXPECT_TRUE(frame_view->caption_button_container_->visible());
 
   ASSERT_NO_FATAL_FAILURE(test::SetAndWaitForTabletMode(false));
@@ -1428,7 +1473,7 @@ IN_PROC_BROWSER_TEST_P(NonHomeLauncherBrowserNonClientFrameViewAshTest,
   // mode.
   EXPECT_EQ(expected_height, frame_view->frame_header_->GetHeaderHeight());
 
-  frame_view->GetFrameWindow()->SetProperty(ash::kIsShowingInOverviewKey, true);
+  ToggleOverview();
 
   if (features::IsUsingWindowService()) {
     ash::mojom::ShellTestApiPtr shell_test_api;
@@ -1437,7 +1482,7 @@ IN_PROC_BROWSER_TEST_P(NonHomeLauncherBrowserNonClientFrameViewAshTest,
         ->BindInterface(ash::mojom::kServiceName, &shell_test_api);
     base::RunLoop run_loop;
     shell_test_api->SnapWindowInSplitView(content::mojom::kBrowserServiceName,
-                                          frame_view->GetServerWindowId(),
+                                          frame_view->GetServerWindowId(), true,
                                           run_loop.QuitClosure());
     run_loop.Run();
   } else {