[Reland] Desks: Fix MRU and stacking order of windows of removed desks

The windows of removed desks should be demoted to be LRU across
all desks. That's why they're appended at the end of the overview
grid list. However, their relative order should be maintained.

This CL makes sure the MRU order is updated to match the above
expectation, as well as the windows' stacking order (that is
windows of the removed desk should be stacked at the bottom
of the target desk's container's children).

BUG=1048271
TEST=Expanded existing tests.

Change-Id: I400b2ac3ce2b5de1f1d8defaff7c4d155e4fab29
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2076670
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745202}

ash/wm/desks/desk.cc

@@ -19,8 +19,10 @@
 #include "ash/wm/workspace/backdrop_controller.h"
 #include "ash/wm/workspace/workspace_layout_manager.h"
 #include "ash/wm/workspace_controller.h"
+#include "base/containers/adapters.h"
 #include "base/stl_util.h"
 #include "ui/aura/client/aura_constants.h"
+#include "ui/aura/window_tracker.h"
 #include "ui/wm/core/window_util.h"
 
 namespace ash {
@@ -303,19 +305,34 @@ void Desk::MoveWindowsToDesk(Desk* target_desk) {
 
     // Moving windows will change the hierarchy and hence |windows_|, and has to
     // be done without changing the relative z-order. So we make a copy of all
-    // the top-level windows on all the containers of this desk.
-    std::vector<aura::Window*> windows_to_move;
-    windows_to_move.reserve(windows_.size());
+    // the top-level windows on all the containers of this desk, such that
+    // windows in each container are copied from top-most (z-order) to
+    // bottom-most.
+    // Note that moving windows out of the container and restacking them
+    // differently may trigger events that lead to destroying a window on the
+    // list. For example moving the top-most window which has a backdrop will
+    // cause the backdrop to be destroyed. Therefore observe such events using
+    // an |aura::WindowTracker|.
+    aura::WindowTracker windows_to_move;
     for (aura::Window* root : Shell::GetAllRootWindows()) {
       const aura::Window* container = GetDeskContainerForRoot(root);
-      const auto& children = container->children();
-      std::copy(children.begin(), children.end(),
-                std::back_inserter(windows_to_move));
+      for (auto* window : base::Reversed(container->children()))
+        windows_to_move.Add(window);
     }
 
-    for (auto* window : windows_to_move) {
-      if (CanMoveWindowOutOfDeskContainer(window))
-        MoveWindowToDeskInternal(window, target_desk);
+    auto* mru_tracker = Shell::Get()->mru_window_tracker();
+    while (!windows_to_move.windows().empty()) {
+      auto* window = windows_to_move.Pop();
+      if (!CanMoveWindowOutOfDeskContainer(window))
+        continue;
+
+      // Note that windows that belong to the same container in
+      // |windows_to_move| are sorted from top-most to bottom-most, hence
+      // calling |StackChildAtBottom()| on each in this order will maintain that
+      // same order in the |target_desk|'s container.
+      MoveWindowToDeskInternal(window, target_desk);
+      window->parent()->StackChildAtBottom(window);
+      mru_tracker->OnWindowMovedOutFromRemovingDesk(window);
     }
   }
 

ash/wm/desks/desk.h

@@ -98,7 +98,13 @@ class ASH_EXPORT Desk {
   // on this desk will be deactivated.
   void Deactivate(bool update_window_activation);
 
-  // Moves all the windows on this desk to |target_desk|.
+  // In preparation for removing this desk, moves all the windows on this desk
+  // to |target_desk| such that they become last in MRU order across all desks,
+  // and they will be stacked at the bottom among the children of
+  // |target_desk|'s container.
+  // Note that from a UX stand point, removing a desk is viewed as the user is
+  // now done with this desk, and therefore its windows are demoted and
+  // deprioritized.
   void MoveWindowsToDesk(Desk* target_desk);
 
   // Moves a single |window| from this desk to |target_desk|. |window| must

ash/wm/desks/desks_controller.cc

@@ -19,6 +19,7 @@
 #include "ash/wm/desks/root_window_desk_switch_animator.h"
 #include "ash/wm/mru_window_tracker.h"
 #include "ash/wm/overview/overview_controller.h"
+#include "ash/wm/overview/overview_grid.h"
 #include "ash/wm/overview/overview_item.h"
 #include "ash/wm/splitview/split_view_controller.h"
 #include "ash/wm/splitview/split_view_utils.h"
@@ -59,10 +60,8 @@ constexpr char kDeskRemovalSmoothnessHistogramName[] =
 
 // Appends the given |windows| to the end of the currently active overview mode
 // session such that the most-recently used window is added first. If
-// |should_animate| is true, the windows will animate to their positions in the
-// overview grid.
-void AppendWindowsToOverview(const std::vector<aura::Window*>& windows,
-                             bool should_animate) {
+// The windows will animate to their positions in the overview grid.
+void AppendWindowsToOverview(const std::vector<aura::Window*>& windows) {
   DCHECK(Shell::Get()->overview_controller()->InOverviewSession());
 
   auto* overview_session =
@@ -74,20 +73,19 @@ void AppendWindowsToOverview(const std::vector<aura::Window*>& windows,
       continue;
     }
 
-    overview_session->AppendItem(window, /*reposition=*/true, should_animate);
+    overview_session->AppendItem(window, /*reposition=*/true, /*animate=*/true);
   }
 }
 
-// Removes the given |windows| from the currently active overview mode session.
-void RemoveWindowsFromOverview(const base::flat_set<aura::Window*>& windows) {
+// Removes all the items that currently exist in overview.
+void RemoveAllWindowsFromOverview() {
   DCHECK(Shell::Get()->overview_controller()->InOverviewSession());
 
   auto* overview_session =
       Shell::Get()->overview_controller()->overview_session();
-  for (auto* window : windows) {
-    auto* item = overview_session->GetOverviewItemForWindow(window);
-    if (item)
-      overview_session->RemoveItem(item);
+  for (const auto& grid : overview_session->grid_list()) {
+    while (!grid->empty())
+      overview_session->RemoveItem(grid->window_list()[0].get());
   }
 }
 
@@ -812,11 +810,13 @@ void DesksController::RemoveDeskInternal(const Desk* desk,
     removed_desk->MoveWindowsToDesk(active_desk_);
 
     // If overview mode is active, we add the windows of the removed desk to the
-    // overview grid in the order of their MRU. Note that this can only be done
-    // after the windows have moved to the active desk above, so that building
+    // overview grid in the order of the new MRU (which changes after removing a
+    // desk by making the windows of the removed desk as the least recently used
+    // across all desks). Note that this can only be done after the windows have
+    // moved to the active desk in `MoveWindowsToDesk()` above, so that building
     // the window MRU list should contain those windows.
     if (in_overview)
-      AppendWindowsToOverview(removed_desk_windows, /*should_animate=*/true);
+      AppendWindowsToOverview(removed_desk_windows);
   } else {
     Desk* target_desk = nullptr;
     if (iter_after == desks_.begin()) {
@@ -841,13 +841,13 @@ void DesksController::RemoveDeskInternal(const Desk* desk,
           ->EndSplitView(SplitViewController::EndReason::kDesksChange);
     }
 
-    // The removed desk is the active desk, so temporarily remove its windows
-    // from the overview grid which will result in removing the
+    // The removed desk is still the active desk, so temporarily remove its
+    // windows from the overview grid which will result in removing the
     // "OverviewModeLabel" widgets created by overview mode for these windows.
     // This way the removed desk tracks only real windows, which are now ready
     // to be moved to the target desk.
     if (in_overview)
-      RemoveWindowsFromOverview(removed_desk_windows);
+      RemoveAllWindowsFromOverview();
 
     // If overview mode is active, change desk activation without changing
     // window activation. Activation should remain on the dummy
@@ -859,9 +859,9 @@ void DesksController::RemoveDeskInternal(const Desk* desk,
     DCHECK_EQ(in_overview, overview_controller->InOverviewSession());
 
     // Now that the windows from the removed and target desks merged, add them
-    // all without animation to the grid in the order of their MRU.
+    // all to the grid in the order of the new MRU.
     if (in_overview)
-      AppendWindowsToOverview(target_desk->windows(), /*should_animate=*/false);
+      AppendWindowsToOverview(target_desk->windows());
   }
 
   // It's OK now to refresh the mini_views of *only* the active desk, and only

ash/wm/desks/desks_unittests.cc

@@ -913,26 +913,32 @@ TEST_F(DesksTest, RemoveInactiveDeskFromOverview) {
   NewDesk();
   ASSERT_EQ(4u, controller->desks().size());
 
-  // Create two windows on desk_1.
+  // Create 3 windows on desk_1.
   auto win0 = CreateAppWindow(gfx::Rect(0, 0, 250, 100));
   auto win1 = CreateAppWindow(gfx::Rect(50, 50, 200, 200));
+  auto win2 = CreateAppWindow(gfx::Rect(50, 50, 200, 200));
   wm::ActivateWindow(win0.get());
   EXPECT_EQ(win0.get(), window_util::GetActiveWindow());
 
-  // Active desk_4 and enter overview mode. Expect that the grid is currently
-  // empty.
+  auto* mru_tracker = Shell::Get()->mru_window_tracker();
+  EXPECT_EQ(std::vector<aura::Window*>({win0.get(), win2.get(), win1.get()}),
+            mru_tracker->BuildMruWindowList(DesksMruType::kActiveDesk));
+
+  // Active desk_4 and enter overview mode, and add a single window.
   Desk* desk_4 = controller->desks()[3].get();
   ActivateDesk(desk_4);
   auto* overview_controller = Shell::Get()->overview_controller();
+  auto win3 = CreateAppWindow(gfx::Rect(50, 50, 200, 200));
   overview_controller->StartOverview();
   EXPECT_TRUE(overview_controller->InOverviewSession());
   const auto* overview_grid =
       GetOverviewGridForRoot(Shell::GetPrimaryRootWindow());
-  EXPECT_TRUE(overview_grid->window_list().empty());
+  EXPECT_EQ(1u, overview_grid->window_list().size());
 
   // Remove desk_1 using the close button on its mini view. desk_1 is currently
   // inactive. Its windows should be moved to desk_4 and added to the overview
-  // grid in the MRU order (win0, and win1).
+  // grid in the MRU order (win0, win2, and win1) at the end after desk_4's
+  // existing window (win3).
   const auto* desks_bar_view = overview_grid->desks_bar_view();
   ASSERT_TRUE(desks_bar_view);
   ASSERT_EQ(4u, desks_bar_view->mini_views().size());
@@ -955,13 +961,20 @@ TEST_F(DesksTest, RemoveInactiveDeskFromOverview) {
 
   ASSERT_EQ(3u, desks_bar_view->mini_views().size());
   EXPECT_TRUE(overview_controller->InOverviewSession());
-  ASSERT_EQ(2u, overview_grid->window_list().size());
+  ASSERT_EQ(4u, overview_grid->window_list().size());
   EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win0.get()));
   EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win1.get()));
-  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win0.get()),
+  EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win2.get()));
+  EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win3.get()));
+  // Expected order of items: win3, win0, win2, win1.
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win3.get()),
             overview_grid->window_list()[0].get());
-  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win1.get()),
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win0.get()),
             overview_grid->window_list()[1].get());
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win2.get()),
+            overview_grid->window_list()[2].get());
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win1.get()),
+            overview_grid->window_list()[3].get());
 
   // Make sure overview mode remains active.
   base::RunLoop().RunUntilIdle();
@@ -980,6 +993,16 @@ TEST_F(DesksTest, RemoveInactiveDeskFromOverview) {
   EXPECT_FALSE(overview_controller->InOverviewSession());
   EXPECT_EQ(1, desk_4_observer.notify_counts());
   desk_4->RemoveObserver(&desk_4_observer);
+
+  // Verify that the stacking order is correct (top-most comes last, and
+  // top-most is the same as MRU).
+  EXPECT_EQ(std::vector<aura::Window*>(
+                {win3.get(), win0.get(), win2.get(), win1.get()}),
+            mru_tracker->BuildMruWindowList(DesksMruType::kActiveDesk));
+  EXPECT_EQ(std::vector<aura::Window*>(
+                {win1.get(), win2.get(), win0.get(), win3.get()}),
+            desk_4->GetDeskContainerForRoot(Shell::GetPrimaryRootWindow())
+                ->children());
 }
 
 TEST_F(DesksTest, RemoveActiveDeskFromOverview) {
@@ -1000,16 +1023,23 @@ TEST_F(DesksTest, RemoveActiveDeskFromOverview) {
   Desk* desk_2 = controller->desks()[1].get();
   ActivateDesk(desk_2);
   auto win2 = CreateAppWindow(gfx::Rect(50, 50, 200, 200));
+  auto win3 = CreateAppWindow(gfx::Rect(50, 50, 200, 200));
   wm::ActivateWindow(win2.get());
   EXPECT_EQ(win2.get(), window_util::GetActiveWindow());
 
+  // The MRU across all desks is now {win2, win3, win0, win1}.
+  auto* mru_tracker = Shell::Get()->mru_window_tracker();
+  EXPECT_EQ(std::vector<aura::Window*>(
+                {win2.get(), win3.get(), win0.get(), win1.get()}),
+            mru_tracker->BuildMruWindowList(DesksMruType::kAllDesks));
+
   // Enter overview mode, and remove desk_2 from its mini-view close button.
   auto* overview_controller = Shell::Get()->overview_controller();
   overview_controller->StartOverview();
   EXPECT_TRUE(overview_controller->InOverviewSession());
   const auto* overview_grid =
       GetOverviewGridForRoot(Shell::GetPrimaryRootWindow());
-  EXPECT_EQ(1u, overview_grid->window_list().size());
+  EXPECT_EQ(2u, overview_grid->window_list().size());
   const auto* desks_bar_view = overview_grid->desks_bar_view();
   ASSERT_TRUE(desks_bar_view);
   ASSERT_EQ(2u, desks_bar_view->mini_views().size());
@@ -1035,24 +1065,31 @@ TEST_F(DesksTest, RemoveActiveDeskFromOverview) {
   EXPECT_TRUE(DoesActiveDeskContainWindow(
       desks_bar_view->GetWidget()->GetNativeWindow()));
 
-  // desk_1 will become active, and windows from desk_2 and desk_1 will merge
-  // and added in the overview grid in the order of MRU.
+  // desk_1 will become active, and windows from desk_2 will move to desk_1 such
+  // that they become last in MRU order, and therefore appended at the end of
+  // the overview grid.
   ASSERT_EQ(1u, controller->desks().size());
   ASSERT_EQ(1u, desks_bar_view->mini_views().size());
   EXPECT_TRUE(desk_1->is_active());
   EXPECT_TRUE(overview_controller->InOverviewSession());
-  EXPECT_EQ(3u, overview_grid->window_list().size());
+  EXPECT_EQ(4u, overview_grid->window_list().size());
   EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win0.get()));
   EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win1.get()));
   EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win2.get()));
+  EXPECT_TRUE(overview_grid->GetOverviewItemContaining(win3.get()));
 
-  // The MRU order is {win2, win0, win1}.
-  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win2.get()),
-            overview_grid->window_list()[0].get());
+  // The new MRU order is {win0, win1, win2, win3}.
+  EXPECT_EQ(std::vector<aura::Window*>(
+                {win0.get(), win1.get(), win2.get(), win3.get()}),
+            mru_tracker->BuildMruWindowList(DesksMruType::kActiveDesk));
   EXPECT_EQ(overview_grid->GetOverviewItemContaining(win0.get()),
-            overview_grid->window_list()[1].get());
+            overview_grid->window_list()[0].get());
   EXPECT_EQ(overview_grid->GetOverviewItemContaining(win1.get()),
+            overview_grid->window_list()[1].get());
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win2.get()),
             overview_grid->window_list()[2].get());
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win3.get()),
+            overview_grid->window_list()[3].get());
 
   // Make sure overview mode remains active.
   base::RunLoop().RunUntilIdle();

ash/wm/mru_window_tracker.cc

@@ -241,6 +241,15 @@ void MruWindowTracker::SetIgnoreActivations(bool ignore) {
     SetActiveWindow(window_util::GetActiveWindow());
 }
 
+void MruWindowTracker::OnWindowMovedOutFromRemovingDesk(aura::Window* window) {
+  DCHECK(window);
+
+  auto iter = std::find(mru_windows_.begin(), mru_windows_.end(), window);
+  DCHECK(iter != mru_windows_.end());
+  mru_windows_.erase(iter);
+  mru_windows_.insert(mru_windows_.begin(), window);
+}
+
 void MruWindowTracker::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }

ash/wm/mru_window_tracker.h

@@ -84,6 +84,11 @@ class ASH_EXPORT MruWindowTracker : public ::wm::ActivationChangeObserver,
   // windows to the front of the MRU window list.
   void SetIgnoreActivations(bool ignore);
 
+  // Called after |window| moved out of its about-to-be-removed desk, to a new
+  // target desk's container. This causes |window| to be made the least-recently
+  // used window across all desks.
+  void OnWindowMovedOutFromRemovingDesk(aura::Window* window);
+
   // Add/Remove observers.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);