views(chromeos): fix restore

There are two paths to unminimize/restoring a window:
. Implicitly by calling Show()/Activate(). In this scenario the expectation
  is the Widget returns to it's pre-minimized state.
  DesktopWindowTreeHostMus does *not* cache the pre-minimized state, only
  the server knows it. While waiting for the server to change the show state
  it's important IsMinimized() returns false. To deal with this an observer is
  added to wait for the show state to change.
. By calling Restore(). Restore sets the state to normal, circumventing
  the pre-minimized state in the server. This mirrors what NativeWidgetAura
  does.

BUG=883523
TEST=covered by tests

Change-Id: I79a89a419032f125d905a98a01fe906c7ca82efd
Reviewed-on: https://chromium-review.googlesource.com/c/1299913
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603522}

chrome/browser/devtools/devtools_sanity_interactive_browsertest.cc

@@ -65,7 +65,7 @@ class CheckWaiter {
 
 class DevToolsManagerDelegateTest : public InProcessBrowserTest {
  public:
-  void SendCommand(std::string state) {
+  void SendCommand(const std::string& state) {
     auto window_bounds =
         protocol::Browser::Bounds::Create().SetWindowState(state).Build();
     BrowserHandler handler(nullptr);

testing/buildbot/filters/chromeos.single_process_mash.interactive_ui_tests.filter

@@ -8,9 +8,6 @@
 # ChromeOS.
 -BrowserKeyEventsTest.AccessKeys
 
-# misc.
--DevToolsManagerDelegateTest.ShowMinimizedWindow
-
 # Broken tests related to accessibility. crbug.com/888750 and crbug.com/889093
 -GuestSpokenFeedbackTest.FocusToolbar
 -SelectToSpeakTest.ActivatesWithTapOnSelectToSpeakTray

ui/views/mus/desktop_window_tree_host_mus.cc

@@ -237,6 +237,36 @@ void OnMoveLoopEnd(bool* out_success,
 
 }  // namespace
 
+// See description in RestoreToPreminimizedState() for details.
+class DesktopWindowTreeHostMus::RestoreWindowObserver
+    : public aura::WindowObserver {
+ public:
+  explicit RestoreWindowObserver(DesktopWindowTreeHostMus* host) : host_(host) {
+    host->window()->AddObserver(this);
+  }
+  ~RestoreWindowObserver() override { host_->window()->RemoveObserver(this); }
+
+  // aura::WindowObserver:
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old) override {
+    if (key == aura::client::kShowStateKey) {
+      host_->restore_window_observer_.reset();
+      // WARNING: this has been deleted.
+    }
+  }
+  void OnWindowDestroying(aura::Window* window) override {
+    // This is owned by DesktopWindowTreeHostMus, which should be destroyed
+    // before the Window.
+    NOTREACHED();
+  }
+
+ private:
+  DesktopWindowTreeHostMus* host_;
+
+  DISALLOW_COPY_AND_ASSIGN(RestoreWindowObserver);
+};
+
 DesktopWindowTreeHostMus::DesktopWindowTreeHostMus(
     aura::WindowTreeHostMusInitParams init_params,
     internal::NativeWidgetDelegate* native_widget_delegate,
@@ -320,6 +350,12 @@ bool DesktopWindowTreeHostMus::ShouldSendClientAreaToServer() const {
   return type == WIP::TYPE_WINDOW || type == WIP::TYPE_PANEL;
 }
 
+void DesktopWindowTreeHostMus::RestoreToPreminimizedState() {
+  DCHECK(IsMinimized());
+  restore_window_observer_ = std::make_unique<RestoreWindowObserver>(this);
+  window()->Show();
+}
+
 void DesktopWindowTreeHostMus::Init(const Widget::InitParams& params) {
   const bool translucent =
       MusClient::ShouldMakeWidgetWindowsTranslucent(params);
@@ -490,6 +526,11 @@ void DesktopWindowTreeHostMus::Show(ui::WindowShowState show_state,
                                     const gfx::Rect& restore_bounds) {
   native_widget_delegate_->OnNativeWidgetVisibilityChanging(true);
 
+  // NOTE: this code is called from Widget::Show() (no args). Widget::Show()
+  // supplies ui::SHOW_STATE_DEFAULT as the |show_state| after the first call.
+  // If SHOW_STATE_DEFAULT is supplied, and the Window is currently minimized,
+  // the window should be restored to its preminimized state.
+
   if (show_state == ui::SHOW_STATE_MAXIMIZED && !restore_bounds.IsEmpty()) {
     window()->SetProperty(aura::client::kRestoreBoundsKey,
                           new gfx::Rect(restore_bounds));
@@ -497,6 +538,9 @@ void DesktopWindowTreeHostMus::Show(ui::WindowShowState show_state,
   if (show_state == ui::SHOW_STATE_MAXIMIZED ||
       show_state == ui::SHOW_STATE_FULLSCREEN) {
     window()->SetProperty(aura::client::kShowStateKey, show_state);
+    restore_window_observer_.reset();
+  } else if (show_state == ui::SHOW_STATE_DEFAULT && IsMinimized()) {
+    RestoreToPreminimizedState();
   }
   // DesktopWindowTreeHostMus is unique in that it calls window()->Show() here.
   // All other implementations call window()->Show() from the constructor. This
@@ -515,8 +559,10 @@ void DesktopWindowTreeHostMus::Show(ui::WindowShowState show_state,
   native_widget_delegate_->OnNativeWidgetVisibilityChanged(true);
 
   if (native_widget_delegate_->CanActivate()) {
-    if (show_state != ui::SHOW_STATE_INACTIVE)
+    if (show_state != ui::SHOW_STATE_INACTIVE &&
+        show_state != ui::SHOW_STATE_MINIMIZED) {
       Activate();
+    }
 
     // SetInitialFocus() should be always be called, even for
     // SHOW_STATE_INACTIVE. If the window has to stay inactive, the method will
@@ -586,7 +632,13 @@ void DesktopWindowTreeHostMus::GetWindowPlacement(
     ui::WindowShowState* show_state) const {
   // Implementation matches that of NativeWidgetAura.
   *bounds = GetRestoredBounds();
-  *show_state = window()->GetProperty(aura::client::kShowStateKey);
+  if (IsWaitingForRestoreToComplete()) {
+    // The real state is not known, use ui::SHOW_STATE_NORMAL to avoid saving
+    // the minimized state.
+    *show_state = ui::SHOW_STATE_NORMAL;
+  } else {
+    *show_state = window()->GetProperty(aura::client::kShowStateKey);
+  }
 }
 
 gfx::Rect DesktopWindowTreeHostMus::GetWindowBoundsInScreen() const {
@@ -632,6 +684,10 @@ void DesktopWindowTreeHostMus::Activate() {
   if (!IsVisible())
     return;
 
+  // Activate() is expected to restore a minimized window.
+  if (IsMinimized())
+    RestoreToPreminimizedState();
+
   // This should result in OnActiveFocusClientChanged() being called, which
   // triggers a call to DesktopNativeWidgetAura::HandleActivationChanged(),
   // which focuses the right window.
@@ -671,6 +727,14 @@ void DesktopWindowTreeHostMus::Maximize() {
 
 void DesktopWindowTreeHostMus::Minimize() {
   window()->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);
+
+  // When minimized, this should no longer be active.
+  if (IsFocusClientInstalledOnFocusSynchronizer()) {
+    MusClient::Get()
+        ->window_tree_client()
+        ->focus_synchronizer()
+        ->SetActiveFocusClient(nullptr, nullptr);
+  }
 }
 
 void DesktopWindowTreeHostMus::Restore() {
@@ -684,7 +748,8 @@ bool DesktopWindowTreeHostMus::IsMaximized() const {
 
 bool DesktopWindowTreeHostMus::IsMinimized() const {
   return window()->GetProperty(aura::client::kShowStateKey) ==
-         ui::SHOW_STATE_MINIMIZED;
+             ui::SHOW_STATE_MINIMIZED &&
+         !IsWaitingForRestoreToComplete();
 }
 
 bool DesktopWindowTreeHostMus::HasCapture() const {

ui/views/mus/desktop_window_tree_host_mus.h

@@ -48,6 +48,8 @@ class VIEWS_MUS_EXPORT DesktopWindowTreeHostMus
   }
 
  private:
+  class RestoreWindowObserver;
+
   void SendClientAreaToServer();
 
   // Returns true if the FocusClient associated with our window is installed on
@@ -62,6 +64,24 @@ class VIEWS_MUS_EXPORT DesktopWindowTreeHostMus
   // Returns true if the client area should be set on this.
   bool ShouldSendClientAreaToServer() const;
 
+  bool IsWaitingForRestoreToComplete() const {
+    return restore_window_observer_.get() != nullptr;
+  }
+
+  // Restores the window to its pre-minimized state. There are two paths to
+  // unminimizing/restoring a window:
+  // . Implicitly by calling Show()/Activate(). In this scenario the expectation
+  //   is the Widget returns to its pre-minimized state.
+  //   DesktopWindowTreeHostMus does *not* cache the pre-minimized state, only
+  //   the server knows it.
+  // . By calling Restore(). Restore sets the state to normal, circumventing
+  //   the pre-minimized state in the server. This mirrors what NativeWidgetAura
+  //   does.
+  // This function handles the first case. As DesktopWindowTreeHostMus doesn't
+  // know the new state, an observer is added that tracks when the show state
+  // changes. While waiting, IsMinimized() returns false.
+  void RestoreToPreminimizedState();
+
   // DesktopWindowTreeHost:
   void Init(const Widget::InitParams& params) override;
   void OnNativeWidgetCreated(const Widget::InitParams& params) override;
@@ -172,6 +192,9 @@ class VIEWS_MUS_EXPORT DesktopWindowTreeHostMus
 
   ScopedObserver<views::View, views::ViewObserver> observed_frame_{this};
 
+  // See description in RestoreToPreminimizedState() for details.
+  std::unique_ptr<RestoreWindowObserver> restore_window_observer_;
+
   // Used so that Close() isn't immediate.
   base::WeakPtrFactory<DesktopWindowTreeHostMus> close_widget_factory_;
 

ui/views/mus/desktop_window_tree_host_mus_unittest.cc

@@ -540,4 +540,38 @@ TEST_F(DesktopWindowTreeHostMusTest,
   EXPECT_TRUE(observer.got_root_window_hidden());
 }
 
+TEST_F(DesktopWindowTreeHostMusTest, MinimizeActivate) {
+  std::unique_ptr<Widget> widget(CreateWidget());
+  widget->Show();
+  EXPECT_TRUE(widget->IsActive());
+
+  widget->Minimize();
+  EXPECT_FALSE(widget->IsActive());
+  EXPECT_TRUE(widget->IsMinimized());
+
+  // Activate() should restore the window.
+  widget->Activate();
+  EXPECT_TRUE(widget->IsActive());
+  EXPECT_FALSE(widget->IsMinimized());
+}
+
+TEST_F(DesktopWindowTreeHostMusTest, MaximizeMinimizeRestore) {
+  std::unique_ptr<Widget> widget(CreateWidget());
+  widget->Show();
+  EXPECT_TRUE(widget->IsActive());
+
+  widget->Maximize();
+  widget->Minimize();
+  EXPECT_FALSE(widget->IsActive());
+  EXPECT_TRUE(widget->IsMinimized());
+  EXPECT_FALSE(widget->IsMaximized());
+
+  widget->Restore();
+  // Restore() *always* sets the state to normal, not the pre-minimized state.
+  // This mirrors the logic in NativeWidgetAura. See
+  // DesktopWindowTreeHostMus::RestoreToPreminimizedState() for details.
+  EXPECT_FALSE(widget->IsMinimized());
+  EXPECT_FALSE(widget->IsMaximized());
+}
+
 }  // namespace views