overview: Fix dragging an ARC window from one overview grid to another

When you drag an ARC window from one overview grid and drop into another
overview grid, it shall neither cause a crash nor cause overview to end.

Test: manual
Bug: 961170
Change-Id: Iac895e4f896cf652dc2d522f3f1b8a4c1765416b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2088507Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Avery Musbach <amusbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747481}

ash/wm/overview/overview_session.cc

@@ -51,7 +51,6 @@
 #include "ui/events/event.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/core/coordinate_conversion.h"
-#include "ui/wm/core/window_util.h"
 
 namespace ash {
 
@@ -894,42 +893,6 @@ void OverviewSession::OnDisplayMetricsChanged(const display::Display& display,
   RefreshNoWindowsWidgetBounds(/*animate=*/false);
 }
 
-void OverviewSession::OnWindowHierarchyChanged(
-    const HierarchyChangeParams& params) {
-  if (ignore_window_hierarchy_changes_)
-    return;
-
-  // Only care about newly added children of |observed_windows_|.
-  if (!observed_windows_.count(params.receiver) ||
-      !observed_windows_.count(params.new_parent)) {
-    return;
-  }
-
-  // Removing a desk while in overview mode results in reparenting the windows
-  // of that desk to the associated container of another desk. This is a window
-  // hierarchy change that shouldn't result in exiting overview mode.
-  if (DesksController::Get()->AreDesksBeingModified())
-    return;
-
-  aura::Window* new_window = params.target;
-  WindowState* state = WindowState::Get(new_window);
-  if (!state->IsUserPositionable() || state->IsPip())
-    return;
-
-  // If the new window is added when splitscreen is active, do nothing.
-  // SplitViewController will do the right thing to snap the window or end
-  // overview mode.
-  if (SplitViewController::Get(new_window)->InSplitViewMode())
-    return;
-
-  if (IsSwitchableContainer(new_window->parent()) &&
-      !::wm::GetTransientParent(new_window)) {
-    // The new window is in one of the switchable containers, abort overview.
-    EndOverview();
-    return;
-  }
-}
-
 void OverviewSession::OnWindowDestroying(aura::Window* window) {
   Shell::Get()
       ->accessibility_controller()

ash/wm/overview/overview_session.h

@@ -326,7 +326,6 @@ class ASH_EXPORT OverviewSession : public display::DisplayObserver,
                                uint32_t metrics) override;
 
   // aura::WindowObserver:
-  void OnWindowHierarchyChanged(const HierarchyChangeParams& params) override;
   void OnWindowDestroying(aura::Window* window) override;
 
   // ShelObserver:
@@ -344,11 +343,6 @@ class ASH_EXPORT OverviewSession : public display::DisplayObserver,
 
   OverviewDelegate* delegate() { return delegate_; }
 
-  void set_ignore_window_hierarchy_changes(
-      bool ignore_window_hierarchy_changes) {
-    ignore_window_hierarchy_changes_ = ignore_window_hierarchy_changes;
-  }
-
   bool is_shutting_down() const { return is_shutting_down_; }
   void set_is_shutting_down(bool is_shutting_down) {
     is_shutting_down_ = is_shutting_down;
@@ -431,10 +425,6 @@ class ASH_EXPORT OverviewSession : public display::DisplayObserver,
   // initially true until this is initialized.
   bool ignore_activations_ = true;
 
-  // True when performing operations that may cause window hierarchy changes.
-  // Used to prevent handling the resulting expected window hierarchy change.
-  bool ignore_window_hierarchy_changes_ = false;
-
   // True when overview mode is exiting.
   bool is_shutting_down_ = false;
 

ash/wm/overview/overview_window_drag_controller.cc

@@ -29,6 +29,7 @@
 #include "ash/wm/window_util.h"
 #include "base/numerics/ranges.h"
 #include "ui/aura/window.h"
+#include "ui/aura/window_observer.h"
 #include "ui/display/display.h"
 #include "ui/wm/core/coordinate_conversion.h"
 
@@ -125,6 +126,65 @@ class AtScopeExitRunner {
   DISALLOW_COPY_AND_ASSIGN(AtScopeExitRunner);
 };
 
+// Helps with handling the workflow where you drag an overview item from one
+// grid and drop into another grid. The challenge is that if the item represents
+// an ARC window, that window will be moved to the target root asynchronously.
+// |OverviewItemMoveHelper| observes the window until it moves to the target
+// root. Then |OverviewItemMoveHelper| self destructs and adds a new item to
+// represent the window on the target root.
+class OverviewItemMoveHelper : public aura::WindowObserver {
+ public:
+  // |target_item_bounds| is the bounds of the dragged overview item when the
+  // drag ends. |target_item_bounds| is used to put the new item where the old
+  // item ended, so it looks like it is the same item. Then the item is animated
+  // from there to its proper position in the grid.
+  OverviewItemMoveHelper(aura::Window* window,
+                         const gfx::RectF& target_item_bounds)
+      : window_(window), target_item_bounds_(target_item_bounds) {
+    window->AddObserver(this);
+  }
+  OverviewItemMoveHelper(const OverviewItemMoveHelper&) = delete;
+  OverviewItemMoveHelper& operator=(const OverviewItemMoveHelper&) = delete;
+  ~OverviewItemMoveHelper() override {
+    OverviewController* overview_controller =
+        Shell::Get()->overview_controller();
+    if (overview_controller->InOverviewSession()) {
+      overview_controller->overview_session()->PositionWindows(
+          /*animate=*/true);
+    }
+  }
+
+  // aura::WindowObserver:
+  void OnWindowDestroyed(aura::Window* window) override {
+    DCHECK_EQ(window_, window);
+    delete this;
+  }
+  void OnWindowAddedToRootWindow(aura::Window* window) override {
+    DCHECK_EQ(window_, window);
+    window->RemoveObserver(this);
+    OverviewController* overview_controller =
+        Shell::Get()->overview_controller();
+    if (overview_controller->InOverviewSession()) {
+      OverviewGrid* target_grid =
+          overview_controller->overview_session()->GetGridWithRootWindow(
+              window->GetRootWindow());
+      // Add |window| to |target_grid| with reposition=false and restack=false,
+      // because soon we will handle both repositioning and restacking anyway.
+      target_grid->AddItemInMruOrder(window, /*reposition=*/false,
+                                     /*animate=*/false, /*restack=*/false);
+      OverviewItem* item = target_grid->GetOverviewItemContaining(window);
+      item->SetBounds(target_item_bounds_, OVERVIEW_ANIMATION_NONE);
+      item->set_should_restack_on_animation_end(true);
+      // The destructor will call |OverviewSession::PositionWindows|.
+    }
+    delete this;
+  }
+
+ private:
+  aura::Window* const window_;
+  const gfx::RectF target_item_bounds_;
+};
+
 }  // namespace
 
 OverviewWindowDragController::OverviewWindowDragController(
@@ -581,36 +641,25 @@ OverviewWindowDragController::CompleteNormalDrag(
       target_root != item_->root_window()) {
     // Get the window and bounds from |item_| before removing it from its grid.
     aura::Window* window = item_->GetWindow();
-    const gfx::RectF bounds = item_->target_bounds();
-    // Remove |item_| from its grid, with reposition=false because soon we will
-    // call |OverviewSession::PositionWindows| anyway.
+    const gfx::RectF target_item_bounds = item_->target_bounds();
+    // Remove |item_| from its grid. Leave the repositioning to the
+    // |OverviewItemMoveHelper|.
     item_->overview_grid()->RemoveItem(item_, /*item_destroying=*/false,
                                        /*reposition=*/false);
     item_ = nullptr;
-    // Use |OverviewSession::set_ignore_window_hierarchy_changes| to prevent
-    // |OverviewSession::OnWindowHierarchyChanged| from ending overview as we
-    // move |window| to |target_root|.
-    overview_session_->set_ignore_window_hierarchy_changes(true);
+    // The |OverviewItemMoveHelper| will self destruct when we move |window| to
+    // |target_root|.
+    new OverviewItemMoveHelper(window, target_item_bounds);
+    // Move |window| to |target_root|. The |OverviewItemMoveHelper| will take
+    // care of the rest.
     window_util::MoveWindowToDisplay(window,
                                      display::Screen::GetScreen()
                                          ->GetDisplayNearestWindow(target_root)
                                          .id());
-    overview_session_->set_ignore_window_hierarchy_changes(false);
-
-    OverviewGrid* target_grid =
-        overview_session_->GetGridWithRootWindow(target_root);
-    // Add |window| to |target_grid| with reposition=false and restack=false,
-    // because soon we will handle both repositioning and restacking anyway.
-    target_grid->AddItemInMruOrder(window, /*reposition=*/false,
-                                   /*animate=*/false, /*restack=*/false);
-    item_ = target_grid->GetOverviewItemContaining(window);
-    // Put the new item where the old item ended, so it looks like it is the
-    // same item. The following call to |OverviewSession::PositionWindows| will
-    // animate it from there.
-    item_->SetBounds(bounds, OVERVIEW_ANIMATION_NONE);
+  } else {
+    item_->set_should_restack_on_animation_end(true);
+    overview_session_->PositionWindows(/*animate=*/true);
   }
-  item_->set_should_restack_on_animation_end(true);
-  overview_session_->PositionWindows(/*animate=*/true);
   return DragResult::kDropIntoOverview;
 }
 

ash/wm/splitview/split_view_controller.cc

@@ -384,17 +384,10 @@ void SplitViewController::SnapWindow(aura::Window* window,
   // will run into problems because |window| will be on the wrong overview grid.
   RemoveSnappingWindowFromOverviewIfApplicable(overview_session, window);
   if (root_window_ != window->GetRootWindow()) {
-    // Use |OverviewSession::set_ignore_window_hierarchy_changes| to prevent
-    // |OverviewSession::OnWindowHierarchyChanged| from ending overview as we
-    // move |window| to |root_window_|.
-    if (overview_session)
-      overview_session->set_ignore_window_hierarchy_changes(true);
     window_util::MoveWindowToDisplay(window,
                                      display::Screen::GetScreen()
                                          ->GetDisplayNearestWindow(root_window_)
                                          .id());
-    if (overview_session)
-      overview_session->set_ignore_window_hierarchy_changes(false);
   }
 
   bool do_divider_spawn_animation = false;