OopAsh: make browser frame respond to changes in tablet mode.

Tested via --ash-debug-shortcuts and Ctrl+Alt+Shift+T

Bug: 854704,760811
Change-Id: Ia3aadae89fcd50d95a84fdf7d275e97bc2e9e2e9
Reviewed-on: https://chromium-review.googlesource.com/1152230
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580021}

ash/frame/custom_frame_view_ash.cc

@@ -63,12 +63,13 @@ class CustomFrameViewAshWindowStateDelegate : public wm::WindowStateDelegate,
     // immersive fullscreen via the "Restore" window control.
     // TODO(pkotwicz): This is a hack. Remove ASAP. http://crbug.com/319048
     window_state_ = window_state;
-    window_state_->AddObserver(this);
     window_state_->window()->AddObserver(this);
 
     if (!enable_immersive)
       return;
 
+    window_state_->AddObserver(this);
+
     Shell::Get()->tablet_mode_controller()->AddObserver(this);
 
     immersive_fullscreen_controller_ =

ash/public/interfaces/shell_test_api.mojom

@@ -8,4 +8,7 @@ interface ShellTestApi {
   // Returns true if a system modal window is open (e.g. the Wi-Fi network
   // password dialog).
   IsSystemModalWindowOpen() => (bool visible);
+
+  // Enables or disables the tablet mode window manager.
+  EnableTabletModeWindowManager(bool enable);
 };

ash/shell_test_api.cc

@@ -10,6 +10,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/tablet_mode/tablet_mode_controller.h"
 #include "components/prefs/testing_pref_service.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 
@@ -71,4 +72,8 @@ void ShellTestApi::IsSystemModalWindowOpen(IsSystemModalWindowOpenCallback cb) {
   std::move(cb).Run(Shell::IsSystemModalWindowOpen());
 }
 
+void ShellTestApi::EnableTabletModeWindowManager(bool enable) {
+  shell_->tablet_mode_controller()->EnableTabletModeWindowManager(enable);
+}
+
 }  // namespace ash

ash/shell_test_api.h

@@ -52,6 +52,7 @@ class ShellTestApi : public mojom::ShellTestApi {
 
   // mojom::ShellTestApi:
   void IsSystemModalWindowOpen(IsSystemModalWindowOpenCallback cb) override;
+  void EnableTabletModeWindowManager(bool enable) override;
 
  private:
   Shell* shell_;  // not owned

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

@@ -139,6 +139,9 @@ BrowserNonClientFrameViewAsh::~BrowserNonClientFrameViewAsh() {
   browser_view()->browser()->command_controller()->RemoveCommandObserver(
       IDC_BACK, this);
 
+  if (TabletModeClient::Get())
+    TabletModeClient::Get()->RemoveObserver(this);
+
   // As with Init(), some of this may need porting to Mash.
   if (IsMash())
     return;
@@ -150,8 +153,6 @@ BrowserNonClientFrameViewAsh::~BrowserNonClientFrameViewAsh() {
 
   window_observer_.RemoveAll();
 
-  if (TabletModeClient::Get())
-    TabletModeClient::Get()->RemoveObserver(this);
   ash::Shell::Get()->RemoveShellObserver(this);
 }
 
@@ -194,6 +195,10 @@ void BrowserNonClientFrameViewAsh::Init() {
   if (browser->profile()->IsOffTheRecord())
     window->SetProperty(ash::kBlockedForAssistantSnapshotKey, true);
 
+  // TabletModeClient may not be initialized during unit tests.
+  if (TabletModeClient::Get())
+    TabletModeClient::Get()->AddObserver(this);
+
   // TODO(estade): how much of the rest of this needs porting to Mash?
   if (IsMash()) {
     window->SetProperty(ash::kFrameTextColorKey, GetTitleColor());
@@ -209,10 +214,6 @@ void BrowserNonClientFrameViewAsh::Init() {
 
   frame_header_ = CreateFrameHeader();
 
-  // TabletModeClient may not be initialized during unit tests.
-  if (TabletModeClient::Get())
-    TabletModeClient::Get()->AddObserver(this);
-
   browser_view()->immersive_mode_controller()->AddObserver(this);
 
   UpdateFrameColors();
@@ -691,19 +692,18 @@ void BrowserNonClientFrameViewAsh::OnOverviewModeEnded() {
 
 void BrowserNonClientFrameViewAsh::OnTabletModeToggled(bool enabled) {
   // TODO(estade): handle in Mash?
-  if (IsMash())
-    return;
+  if (!IsMash()) {
+    if (!enabled && browser_view()->immersive_mode_controller()->IsRevealed()) {
+      // Before updating the caption buttons state below (which triggers a
+      // relayout), we want to move the caption buttons from the
+      // TopContainerView back to this view.
+      OnImmersiveRevealEnded();
+    }
 
-  if (!enabled && browser_view()->immersive_mode_controller()->IsRevealed()) {
-    // Before updating the caption buttons state below (which triggers a
-    // relayout), we want to move the caption buttons from the TopContainerView
-    // back to this view.
-    OnImmersiveRevealEnded();
+    caption_button_container_->SetVisible(ShouldShowCaptionButtons());
+    caption_button_container_->UpdateCaptionButtonState(true /*=animate*/);
   }
 
-  caption_button_container_->SetVisible(ShouldShowCaptionButtons());
-  caption_button_container_->UpdateCaptionButtonState(true /*=animate*/);
-
   if (enabled) {
     // Enter immersive mode if the feature is enabled and the widget is not
     // already in fullscreen mode. Popups that are not activated but not

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

@@ -170,10 +170,6 @@ void ImmersiveModeControllerAsh::OnFindBarVisibleBoundsChanged(
 }
 
 bool ImmersiveModeControllerAsh::ShouldStayImmersiveAfterExitingFullscreen() {
-  // TODO(crbug.com/760811): Support tablet mode in mash.
-  if (!features::IsAshInBrowserProcess())
-    return false;
-
   return !browser_view_->IsBrowserTypeNormal() &&
          TabletModeClient::Get()->tablet_mode_enabled();
 }
@@ -188,11 +184,8 @@ void ImmersiveModeControllerAsh::OnWidgetActivationChanged(
   if (browser_view_->IsBrowserTypeNormal())
     return;
 
-  // TODO(crbug.com/760811): Support tablet mode in mash.
-  if (!features::IsAshInBrowserProcess() ||
-      !TabletModeClient::Get()->tablet_mode_enabled()) {
+  if (!TabletModeClient::Get()->tablet_mode_enabled())
     return;
-  }
 
   // Enable immersive mode if the widget is activated. Do not disable immersive
   // mode if the widget deactivates, but is not minimized.

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

@@ -8,6 +8,8 @@
 #include "ash/frame/caption_buttons/frame_caption_button_container_view.h"
 #include "ash/public/cpp/config.h"
 #include "ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.h"
+#include "ash/public/interfaces/constants.mojom.h"
+#include "ash/public/interfaces/shell_test_api.mojom.h"
 #include "ash/shell.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/macros.h"
@@ -15,6 +17,7 @@
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/ssl/cert_verifier_browser_test.h"
+#include "chrome/browser/ui/ash/tablet_mode_client.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller_test.h"
@@ -30,11 +33,62 @@
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/service_manager_connection.h"
 #include "net/cert/mock_cert_verifier.h"
+#include "services/service_manager/public/cpp/connector.h"
 #include "ui/aura/client/aura_constants.h"
+#include "ui/aura/test/mus/change_completion_waiter.h"
 #include "ui/base/ui_base_features.h"
 #include "ui/views/animation/test/ink_drop_host_view_test_api.h"
 
+namespace {
+
+// A helper used to wait for an expected change to the tablet mode state.
+class TestTabletModeClientObserver : public TabletModeClientObserver {
+ public:
+  explicit TestTabletModeClientObserver(bool target_state)
+      : target_state_(target_state) {
+    TabletModeClient::Get()->AddObserver(this);
+  }
+
+  ~TestTabletModeClientObserver() override {
+    TabletModeClient::Get()->RemoveObserver(this);
+  }
+
+  void OnTabletModeToggled(bool enabled) override {
+    if (enabled == target_state_)
+      run_loop_.Quit();
+  }
+
+  base::RunLoop* run_loop() { return &run_loop_; }
+
+ private:
+  const bool target_state_;
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestTabletModeClientObserver);
+};
+
+// Enables or disables the tablet mode and waits to until the change has made
+// its way back into Chrome (from Ash). Should only be called to toggle the
+// current mode.
+void SetAndWaitForTabletMode(bool enabled) {
+  ASSERT_NE(enabled, TabletModeClient::Get()->tablet_mode_enabled());
+
+  ash::mojom::ShellTestApiPtr shell_test_api;
+  content::ServiceManagerConnection::GetForProcess()
+      ->GetConnector()
+      ->BindInterface(ash::mojom::kServiceName, &shell_test_api);
+  shell_test_api->EnableTabletModeWindowManager(enabled);
+
+  TestTabletModeClientObserver observer(enabled);
+  observer.run_loop()->Run();
+
+  ASSERT_EQ(enabled, TabletModeClient::Get()->tablet_mode_enabled());
+}
+
+}  // namespace
+
 class ImmersiveModeControllerAshHostedAppBrowserTest
     : public extensions::ExtensionBrowserTest {
  public:
@@ -221,39 +275,34 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
   LaunchAppBrowser();
   ASSERT_FALSE(controller()->IsEnabled());
 
-  aura::Window* window =
-      browser_view()->frame()->GetFrameView()->frame()->GetNativeWindow();
+  aura::Window* aura_window = browser_view()->frame()->GetNativeWindow();
   // Verify that after entering tablet mode, immersive mode is enabled, and the
   // the associated window's top inset is 0 (the top of the window is not
   // visible).
-  ash::TabletModeController* tablet_mode_controller =
-      ash::Shell::Get()->tablet_mode_controller();
-  tablet_mode_controller->EnableTabletModeWindowManager(true);
-  tablet_mode_controller->FlushForTesting();
+  ASSERT_NO_FATAL_FAILURE(SetAndWaitForTabletMode(true));
   EXPECT_TRUE(controller()->IsEnabled());
-  EXPECT_EQ(0, window->GetProperty(aura::client::kTopViewInset));
+  EXPECT_EQ(0, aura_window->GetProperty(aura::client::kTopViewInset));
 
   // Verify that after minimizing, immersive mode is disabled.
   browser()->window()->Minimize();
+  aura::test::WaitForAllChangesToComplete();
+  EXPECT_TRUE(browser()->window()->IsMinimized());
   EXPECT_FALSE(controller()->IsEnabled());
 
   // Verify that after showing the browser, immersive mode is reenabled.
   browser()->window()->Show();
-  tablet_mode_controller->FlushForTesting();
   EXPECT_TRUE(controller()->IsEnabled());
 
   // Verify that immersive mode remains if fullscreen is toggled while in tablet
   // mode.
   ToggleFullscreen();
   EXPECT_TRUE(controller()->IsEnabled());
-  tablet_mode_controller->EnableTabletModeWindowManager(false);
-  tablet_mode_controller->FlushForTesting();
+  ASSERT_NO_FATAL_FAILURE(SetAndWaitForTabletMode(false));
   EXPECT_TRUE(controller()->IsEnabled());
 
   // Verify that immersive mode remains if the browser was fullscreened when
   // entering tablet mode.
-  tablet_mode_controller->EnableTabletModeWindowManager(true);
-  tablet_mode_controller->FlushForTesting();
+  ASSERT_NO_FATAL_FAILURE(SetAndWaitForTabletMode(true));
   EXPECT_TRUE(controller()->IsEnabled());
 
   // Verify that if the browser is not fullscreened, upon exiting tablet mode,
@@ -261,20 +310,27 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
   // greater than 0 (the top of the window is visible).
   ToggleFullscreen();
   EXPECT_TRUE(controller()->IsEnabled());
-  tablet_mode_controller->EnableTabletModeWindowManager(false);
-  tablet_mode_controller->FlushForTesting();
+  ASSERT_NO_FATAL_FAILURE(SetAndWaitForTabletMode(false));
   EXPECT_FALSE(controller()->IsEnabled());
-  EXPECT_GT(window->GetProperty(aura::client::kTopViewInset), 0);
+
+  // TODO(estade): make kTopviewInset work in OopAsh.
+  if (features::IsAshInBrowserProcess())
+    EXPECT_GT(aura_window->GetProperty(aura::client::kTopViewInset), 0);
 }
 
 // Verify that the frame layout is as expected when using immersive mode in
 // tablet mode.
 IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
                        FrameLayoutToggleTabletMode) {
+  // For OopAsh, the layout is handled in Ash and tested by
+  // FrameCaptionButtonContainerViewTest.
+  // TODO(estade): remove this test when OopAsh is default.
+  if (!features::IsAshInBrowserProcess())
+    return;
+
   LaunchAppBrowser();
   ASSERT_FALSE(controller()->IsEnabled());
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser());
-  ASSERT_TRUE(features::IsAshInBrowserProcess());
   BrowserNonClientFrameViewAsh* frame_view =
       static_cast<BrowserNonClientFrameViewAsh*>(
           browser_view->GetWidget()->non_client_view()->frame_view());
@@ -286,10 +342,7 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
   EXPECT_TRUE(frame_test_api.size_button()->visible());
 
   // Verify the size button is hidden in tablet mode.
-  ash::TabletModeController* tablet_mode_controller =
-      ash::Shell::Get()->tablet_mode_controller();
-  tablet_mode_controller->EnableTabletModeWindowManager(true);
-  tablet_mode_controller->FlushForTesting();
+  SetAndWaitForTabletMode(true);
   frame_test_api.EndAnimations();
 
   EXPECT_FALSE(frame_test_api.size_button()->visible());
@@ -298,8 +351,7 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
 
   // Verify the size button is visible in clamshell mode, and that it does not
   // cover the other two buttons.
-  tablet_mode_controller->EnableTabletModeWindowManager(false);
-  tablet_mode_controller->FlushForTesting();
+  SetAndWaitForTabletMode(false);
   frame_test_api.EndAnimations();
 
   EXPECT_TRUE(frame_test_api.size_button()->visible());
@@ -316,10 +368,7 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
 IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
                        FrameLayoutStartInTabletMode) {
   // Start in tablet mode
-  ash::TabletModeController* tablet_mode_controller =
-      ash::Shell::Get()->tablet_mode_controller();
-  tablet_mode_controller->EnableTabletModeWindowManager(true);
-  tablet_mode_controller->FlushForTesting();
+  SetAndWaitForTabletMode(true);
 
   BrowserNonClientFrameViewAsh* frame_view = nullptr;
   {
@@ -330,7 +379,6 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
     LaunchAppBrowser(false);
     BrowserView* browser_view =
         BrowserView::GetBrowserViewForBrowser(browser());
-    ASSERT_TRUE(features::IsAshInBrowserProcess());
     frame_view = static_cast<BrowserNonClientFrameViewAsh*>(
         browser_view->GetWidget()->non_client_view()->frame_view());
 
@@ -341,8 +389,6 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerAshHostedAppBrowserTest,
 
   // Verify the size button is visible in clamshell mode, and that it does not
   // cover the other two buttons.
-  tablet_mode_controller->EnableTabletModeWindowManager(false);
-  tablet_mode_controller->FlushForTesting();
-
+  SetAndWaitForTabletMode(false);
   VerifyButtonsInImmersiveMode(frame_view);
 }

testing/buildbot/filters/mash.browser_tests.filter

@@ -54,7 +54,6 @@
 # Fix immersive fullscreen mode in mash. https://crbug.com/844748.
 -ImmersiveModeBrowserViewTest.TestCaptionButtonsReceiveEventsInAppImmersiveMode*
 -ImmersiveModeBrowserViewTest.TestCaptionButtonsReceiveEventsInBrowserImmersiveMode*
--ImmersiveModeControllerAshHostedAppBrowserTest.*
 
 # Null immersive_fullscreen_controller_.
 -ChromeNativeAppWindowViewsAuraAshBrowserTest.*