mus: makes showing/hiding WindowTreeHost's window same as Widget

This patch makes showing/hiding the WindowTreeHost's window behave the
same as if Widget Show/Hide was called. This is necessary for transient
windows to work. In particular it's expected that hiding the transient
parent hides transient children. TransientWindowManager does that by way
of calling show/hide directly on the aura window. I could have added the
ability to inject function to accomplish that, but I know I've run into
other code expecting show/hide on the aura window to map to showing/hiding
the widget.

BUG=883371
TEST=covered by tests

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

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

@@ -33,9 +33,6 @@
 # Failing on ASAN bot. crbug.com/882631
 -ExtensionWebRequestApiTest.WebRequestTypes
 
-# Crashing flakily. https://crbug.com/883371
--ExtensionDisabledGlobalErrorTest.UninstallWhilePromptBeingShown
-
 # Flaky on ASAN bot. crbug.com/884216
 -KeyboardOverlayUIBrowserTest.*
 

ui/views/mus/desktop_window_tree_host_mus.cc

@@ -278,34 +278,45 @@ class ScopedTouchTransferController : public ui::GestureRecognizerObserver {
 
 }  // namespace
 
-// See description in RestoreToPreminimizedState() for details.
-class DesktopWindowTreeHostMus::RestoreWindowObserver
+// WindowObserver installed on DesktopWindowTreeHostMus::window(). Mostly
+// forwards interesting events to DesktopWindowTreeHostMus. To avoid having
+// DesktopWindowTreeHostMus be a WindowObserver on two windows (which is mildly
+// error prone), this helper class is used.
+class DesktopWindowTreeHostMus::WindowTreeHostWindowObserver
     : public aura::WindowObserver {
  public:
-  explicit RestoreWindowObserver(DesktopWindowTreeHostMus* host) : host_(host) {
+  explicit WindowTreeHostWindowObserver(DesktopWindowTreeHostMus* host)
+      : host_(host) {
     host->window()->AddObserver(this);
   }
-  ~RestoreWindowObserver() override { host_->window()->RemoveObserver(this); }
+  ~WindowTreeHostWindowObserver() override {
+    host_->window()->RemoveObserver(this);
+  }
+
+  void set_is_waiting_for_restore(bool value) {
+    is_waiting_for_restore_ = value;
+  }
+  bool is_waiting_for_restore() const { return is_waiting_for_restore_; }
 
   // aura::WindowObserver:
+  void OnWindowVisibilityChanged(aura::Window* window, bool visible) override {
+    if (window == host_->window())
+      host_->OnWindowTreeHostWindowVisibilityChanged(visible);
+  }
   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();
+    if (key == aura::client::kShowStateKey)
+      is_waiting_for_restore_ = false;
   }
 
  private:
   DesktopWindowTreeHostMus* host_;
 
-  DISALLOW_COPY_AND_ASSIGN(RestoreWindowObserver);
+  // True while waiting for the show state to change.
+  bool is_waiting_for_restore_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(WindowTreeHostWindowObserver);
 };
 
 DesktopWindowTreeHostMus::DesktopWindowTreeHostMus(
@@ -325,11 +336,16 @@ DesktopWindowTreeHostMus::DesktopWindowTreeHostMus(
   // Widget. This call enables that.
   NativeWidgetAura::RegisterNativeWidgetForWindow(desktop_native_widget_aura,
                                                   window());
+
+  window_tree_host_window_observer_ =
+      std::make_unique<WindowTreeHostWindowObserver>(this);
   // TODO: use display id and bounds if available, likely need to pass in
   // InitParams for that.
 }
 
 DesktopWindowTreeHostMus::~DesktopWindowTreeHostMus() {
+  window_tree_host_window_observer_.reset();
+
   // The cursor-client can be accessed during WindowTreeHostMus tear-down. So
   // the cursor-client needs to be unset on the root-window before
   // |cursor_manager_| is destroyed.
@@ -386,6 +402,10 @@ void DesktopWindowTreeHostMus::SetBoundsInDIP(const gfx::Rect& bounds_in_dip) {
   SetBoundsInPixels(rect, viz::LocalSurfaceIdAllocation());
 }
 
+bool DesktopWindowTreeHostMus::IsWaitingForRestoreToComplete() const {
+  return window_tree_host_window_observer_->is_waiting_for_restore();
+}
+
 bool DesktopWindowTreeHostMus::ShouldSendClientAreaToServer() const {
   if (!auto_update_client_area_)
     return false;
@@ -397,10 +417,26 @@ bool DesktopWindowTreeHostMus::ShouldSendClientAreaToServer() const {
 
 void DesktopWindowTreeHostMus::RestoreToPreminimizedState() {
   DCHECK(IsMinimized());
-  restore_window_observer_ = std::make_unique<RestoreWindowObserver>(this);
+  window_tree_host_window_observer_->set_is_waiting_for_restore(true);
+  base::AutoReset<bool> setter(&is_updating_window_visibility_, true);
   window()->Show();
 }
 
+void DesktopWindowTreeHostMus::OnWindowTreeHostWindowVisibilityChanged(
+    bool visible) {
+  if (is_updating_window_visibility_)
+    return;
+
+  // Call Show()/Hide() so that the visibility state is mirrored correctly. This
+  // function makes it so that calling Show()/Hide() on window() is the same as
+  // calling Show()/Hide() on the Widget.
+  base::AutoReset<bool> setter(&is_updating_window_visibility_, true);
+  if (visible)
+    Show(ui::SHOW_STATE_INACTIVE, gfx::Rect());
+  else
+    Hide();
+}
+
 void DesktopWindowTreeHostMus::Init(const Widget::InitParams& params) {
   const bool translucent =
       MusClient::ShouldMakeWidgetWindowsTranslucent(params);
@@ -575,7 +611,11 @@ aura::WindowTreeHost* DesktopWindowTreeHostMus::AsWindowTreeHost() {
 
 void DesktopWindowTreeHostMus::Show(ui::WindowShowState show_state,
                                     const gfx::Rect& restore_bounds) {
-  native_widget_delegate_->OnNativeWidgetVisibilityChanging(true);
+  // Only notify if the visibility is really changing.
+  const bool notify_visibility_change =
+      is_updating_window_visibility_ || !IsVisible();
+  if (notify_visibility_change)
+    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.
@@ -589,7 +629,7 @@ 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();
+    window_tree_host_window_observer_->set_is_waiting_for_restore(false);
   } else if (show_state == ui::SHOW_STATE_DEFAULT && IsMinimized()) {
     RestoreToPreminimizedState();
   } else if (show_state == ui::SHOW_STATE_MINIMIZED && !IsMinimized()) {
@@ -600,7 +640,10 @@ void DesktopWindowTreeHostMus::Show(ui::WindowShowState show_state,
   // is necessary as window()'s visibility is mirrored in the server, on other
   // platforms it's the visibility of the AcceleratedWidget that matters and
   // dictates what is actually drawn on screen.
-  window()->Show();
+  {
+    base::AutoReset<bool> setter(&is_updating_window_visibility_, true);
+    window()->Show();
+  }
   if (compositor())
     compositor()->SetVisible(true);
 
@@ -609,7 +652,8 @@ void DesktopWindowTreeHostMus::Show(ui::WindowShowState show_state,
   // otherwise focus goes to window().
   content_window()->Show();
 
-  native_widget_delegate_->OnNativeWidgetVisibilityChanged(true);
+  if (notify_visibility_change)
+    native_widget_delegate_->OnNativeWidgetVisibilityChanged(true);
 
   if (native_widget_delegate_->CanActivate()) {
     if (show_state != ui::SHOW_STATE_INACTIVE &&
@@ -1019,8 +1063,17 @@ void DesktopWindowTreeHostMus::ShowImpl() {
 }
 
 void DesktopWindowTreeHostMus::HideImpl() {
+  // If |is_updating_window_visibility_| is true, this is being called in
+  // response to window()'s visibility changing, in which case we need to
+  // continue on to complete processing of the hide.
+  if (!IsVisible() && !is_updating_window_visibility_)
+    return;
+
   native_widget_delegate_->OnNativeWidgetVisibilityChanging(false);
-  WindowTreeHostMus::HideImpl();
+  {
+    base::AutoReset<bool> setter(&is_updating_window_visibility_, true);
+    WindowTreeHostMus::HideImpl();
+  }
   native_widget_delegate_->OnNativeWidgetVisibilityChanged(false);
 
   // When hiding we can't possibly be active any more. Reset the FocusClient,

ui/views/mus/desktop_window_tree_host_mus.h

@@ -48,7 +48,7 @@ class VIEWS_MUS_EXPORT DesktopWindowTreeHostMus
   }
 
  private:
-  class RestoreWindowObserver;
+  class WindowTreeHostWindowObserver;
 
   void SendClientAreaToServer();
 
@@ -64,9 +64,7 @@ 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;
-  }
+  bool IsWaitingForRestoreToComplete() const;
 
   // Restores the window to its pre-minimized state. There are two paths to
   // unminimizing/restoring a window:
@@ -82,6 +80,10 @@ class VIEWS_MUS_EXPORT DesktopWindowTreeHostMus
   // changes. While waiting, IsMinimized() returns false.
   void RestoreToPreminimizedState();
 
+  // Called when window()'s visibility changes to |visible|. This is called from
+  // WindowObserver::OnWindowVisibilityChanged().
+  void OnWindowTreeHostWindowVisibilityChanged(bool visible);
+
   // DesktopWindowTreeHost:
   void Init(const Widget::InitParams& params) override;
   void OnNativeWidgetCreated(const Widget::InitParams& params) override;
@@ -193,8 +195,13 @@ class VIEWS_MUS_EXPORT DesktopWindowTreeHostMus
   // server.
   ScopedObserver<views::View, views::ViewObserver> observed_client_view_{this};
 
-  // See description in RestoreToPreminimizedState() for details.
-  std::unique_ptr<RestoreWindowObserver> restore_window_observer_;
+  // If true, |this| is changing the visibility of window(), or is processing
+  // a change in the visibility of window().
+  bool is_updating_window_visibility_ = false;
+
+  // aura::WindowObserver on window().
+  std::unique_ptr<WindowTreeHostWindowObserver>
+      window_tree_host_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

@@ -38,6 +38,7 @@
 #include "ui/views/widget/widget_delegate.h"
 #include "ui/views/widget/widget_observer.h"
 #include "ui/wm/core/shadow_types.h"
+#include "ui/wm/core/transient_window_manager.h"
 
 namespace views {
 
@@ -777,4 +778,101 @@ TEST_F(DesktopWindowTreeHostMusTest, ShowWindowFromServerDoesntActivate) {
   EXPECT_FALSE(widget->IsActive());
 }
 
+// Used to track the number of times OnWidgetVisibilityChanged() is called.
+class WidgetVisibilityObserver : public WidgetObserver {
+ public:
+  WidgetVisibilityObserver() = default;
+  ~WidgetVisibilityObserver() override = default;
+
+  int get_and_clear_change_count() {
+    int result = change_count_;
+    change_count_ = 0;
+    return result;
+  }
+
+  // WidgetObserver:
+  void OnWidgetVisibilityChanged(Widget* widget, bool visible) override {
+    change_count_++;
+  }
+
+ private:
+  int change_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(WidgetVisibilityObserver);
+};
+
+TEST_F(DesktopWindowTreeHostMusTest,
+       TogglingVisibilityOfWindowTreeWindowTriggersWidgetNotification) {
+  std::unique_ptr<Widget> widget(CreateWidget());
+  widget->Show();
+
+  WidgetVisibilityObserver observer;
+  widget->AddObserver(&observer);
+
+  widget->Show();
+  EXPECT_EQ(0, observer.get_and_clear_change_count());
+  EXPECT_TRUE(widget->IsVisible());
+  EXPECT_TRUE(widget->GetNativeWindow()->GetHost()->compositor()->IsVisible());
+  EXPECT_TRUE(widget->GetNativeWindow()->GetRootWindow()->IsVisible());
+
+  widget->Hide();
+  EXPECT_EQ(1, observer.get_and_clear_change_count());
+  EXPECT_FALSE(widget->IsVisible());
+  EXPECT_FALSE(widget->GetNativeWindow()->GetHost()->compositor()->IsVisible());
+  EXPECT_FALSE(widget->GetNativeWindow()->GetRootWindow()->IsVisible());
+
+  // Changing the visibility of the WindowTreeHost Window should notify the
+  // observer.
+  widget->GetNativeWindow()->GetHost()->window()->Show();
+  EXPECT_EQ(1, observer.get_and_clear_change_count());
+  EXPECT_TRUE(widget->IsVisible());
+  EXPECT_TRUE(widget->GetNativeWindow()->GetHost()->compositor()->IsVisible());
+  EXPECT_TRUE(widget->GetNativeWindow()->GetRootWindow()->IsVisible());
+
+  widget->GetNativeWindow()->GetHost()->window()->Hide();
+  EXPECT_EQ(1, observer.get_and_clear_change_count());
+  EXPECT_FALSE(widget->IsVisible());
+  EXPECT_FALSE(widget->GetNativeWindow()->GetHost()->compositor()->IsVisible());
+  EXPECT_FALSE(widget->GetNativeWindow()->GetRootWindow()->IsVisible());
+
+  widget->RemoveObserver(&observer);
+}
+
+TEST_F(DesktopWindowTreeHostMusTest, TransientChildMatchesParentVisibility) {
+  std::unique_ptr<Widget> widget(CreateWidget());
+  widget->Show();
+
+  std::unique_ptr<Widget> transient_child =
+      CreateWidget(nullptr, widget->GetNativeWindow());
+  transient_child->Show();
+
+  WidgetVisibilityObserver observer;
+  transient_child->AddObserver(&observer);
+
+  // Hiding the parent should also hide the transient child.
+  widget->Hide();
+  EXPECT_FALSE(transient_child->IsVisible());
+  EXPECT_EQ(1, observer.get_and_clear_change_count());
+  EXPECT_FALSE(
+      transient_child->GetNativeWindow()->GetHost()->compositor()->IsVisible());
+  EXPECT_FALSE(
+      transient_child->GetNativeWindow()->GetRootWindow()->IsVisible());
+
+  // set_parent_controls_visibility(true) makes it so showing the parent also
+  // shows the child.
+  wm::TransientWindowManager::GetOrCreate(
+      transient_child->GetNativeWindow()->GetRootWindow())
+      ->set_parent_controls_visibility(true);
+  // With set_parent_controls_visibility() true, showing the parent should also
+  // show the transient child.
+  widget->Show();
+  EXPECT_TRUE(transient_child->IsVisible());
+  EXPECT_EQ(1, observer.get_and_clear_change_count());
+  EXPECT_TRUE(
+      transient_child->GetNativeWindow()->GetHost()->compositor()->IsVisible());
+  EXPECT_TRUE(transient_child->GetNativeWindow()->GetRootWindow()->IsVisible());
+
+  transient_child->RemoveObserver(&observer);
+}
+
 }  // namespace views