Revert "Desks: Fix MRU and stacking order of windows of removed desks"

This reverts commit 8d889479.

Reason for revert: suspect causing ash_unittests failure on linux-chromeos-dbg
- TabletModeDesksTest.DesksCreationRemovalCycle
- TabletModeDesksTest.RemoveDeskWithMaximizedWindowAndMergeWithSnapped
- TabletModeDesksTest.RemovingDesksWithSplitView

Sample build: https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg/17282

Sample log: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8887345963796360720/+/steps/ash_unittests/0/logs/Deterministic_failure:_TabletModeDesksTest.DesksCreationRemovalCycle__status_CRASH_/0

Original change's description:
> 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: I06bc69e068e3ffc29bccf7a5983ed6d56fc04830
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2073366
> Reviewed-by: James Cook <jamescook@chromium.org>
> Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#744856}

TBR=jamescook@chromium.org,afakhry@chromium.org

Change-Id: I28c5e412f0b7b44b6dfa8f5219f3eb7f514237cf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1048271
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2076040Reviewed-by: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/master@{#744910}

ash/wm/desks/desk.cc

@@ -303,30 +303,19 @@ 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, such that
-    // windows in each container are copied from top-most (z-order) to
-    // bottom-most.
+    // the top-level windows on all the containers of this desk.
     std::vector<aura::Window*> windows_to_move;
     windows_to_move.reserve(windows_.size());
     for (aura::Window* root : Shell::GetAllRootWindows()) {
       const aura::Window* container = GetDeskContainerForRoot(root);
       const auto& children = container->children();
-      std::copy(children.rbegin(), children.rend(),
+      std::copy(children.begin(), children.end(),
                 std::back_inserter(windows_to_move));
     }
 
-    auto* mru_tracker = Shell::Get()->mru_window_tracker();
     for (auto* window : windows_to_move) {
-      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);
+      if (CanMoveWindowOutOfDeskContainer(window))
+        MoveWindowToDeskInternal(window, target_desk);
     }
   }
 

ash/wm/desks/desk.h

@@ -98,13 +98,7 @@ class ASH_EXPORT Desk {
   // on this desk will be deactivated.
   void Deactivate(bool update_window_activation);
 
-  // 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.
+  // Moves all the windows on this desk to |target_desk|.
   void MoveWindowsToDesk(Desk* target_desk);
 
   // Moves a single |window| from this desk to |target_desk|. |window| must

ash/wm/desks/desks_controller.cc

@@ -59,8 +59,10 @@ 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
-// The windows will animate to their positions in the overview grid.
-void AppendWindowsToOverview(const std::vector<aura::Window*>& windows) {
+// |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) {
   DCHECK(Shell::Get()->overview_controller()->InOverviewSession());
 
   auto* overview_session =
@@ -72,7 +74,7 @@ void AppendWindowsToOverview(const std::vector<aura::Window*>& windows) {
       continue;
     }
 
-    overview_session->AppendItem(window, /*reposition=*/true, /*animate=*/true);
+    overview_session->AppendItem(window, /*reposition=*/true, should_animate);
   }
 }
 
@@ -810,13 +812,11 @@ 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 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
+    // 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
     // the window MRU list should contain those windows.
     if (in_overview)
-      AppendWindowsToOverview(removed_desk_windows);
+      AppendWindowsToOverview(removed_desk_windows, /*should_animate=*/true);
   } else {
     Desk* target_desk = nullptr;
     if (iter_after == desks_.begin()) {
@@ -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 to the grid in the order of the new MRU.
+    // all without animation to the grid in the order of their MRU.
     if (in_overview)
-      AppendWindowsToOverview(target_desk->windows());
+      AppendWindowsToOverview(target_desk->windows(), /*should_animate=*/false);
   }
 
   // It's OK now to refresh the mini_views of *only* the active desk, and only

ash/wm/desks/desks_unittests.cc

@@ -913,32 +913,26 @@ TEST_F(DesksTest, RemoveInactiveDeskFromOverview) {
   NewDesk();
   ASSERT_EQ(4u, controller->desks().size());
 
-  // Create 3 windows on desk_1.
+  // Create two 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());
 
-  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.
+  // Active desk_4 and enter overview mode. Expect that the grid is currently
+  // empty.
   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_EQ(1u, overview_grid->window_list().size());
+  EXPECT_TRUE(overview_grid->window_list().empty());
 
   // 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, win2, and win1) at the end after desk_4's
-  // existing window (win3).
+  // grid in the MRU order (win0, and win1).
   const auto* desks_bar_view = overview_grid->desks_bar_view();
   ASSERT_TRUE(desks_bar_view);
   ASSERT_EQ(4u, desks_bar_view->mini_views().size());
@@ -961,20 +955,13 @@ TEST_F(DesksTest, RemoveInactiveDeskFromOverview) {
 
   ASSERT_EQ(3u, desks_bar_view->mini_views().size());
   EXPECT_TRUE(overview_controller->InOverviewSession());
-  ASSERT_EQ(4u, overview_grid->window_list().size());
+  ASSERT_EQ(2u, 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()));
-  // 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(win0.get()),
-            overview_grid->window_list()[1].get());
-  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win2.get()),
-            overview_grid->window_list()[2].get());
+            overview_grid->window_list()[0].get());
   EXPECT_EQ(overview_grid->GetOverviewItemContaining(win1.get()),
-            overview_grid->window_list()[3].get());
+            overview_grid->window_list()[1].get());
 
   // Make sure overview mode remains active.
   base::RunLoop().RunUntilIdle();
@@ -993,16 +980,6 @@ 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) {
@@ -1023,23 +1000,16 @@ 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(2u, overview_grid->window_list().size());
+  EXPECT_EQ(1u, 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());
@@ -1065,31 +1035,24 @@ TEST_F(DesksTest, RemoveActiveDeskFromOverview) {
   EXPECT_TRUE(DoesActiveDeskContainWindow(
       desks_bar_view->GetWidget()->GetNativeWindow()));
 
-  // 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.
+  // 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.
   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(4u, overview_grid->window_list().size());
+  EXPECT_EQ(3u, 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 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()),
+  // The MRU order is {win2, win0, win1}.
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win2.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()),
+  EXPECT_EQ(overview_grid->GetOverviewItemContaining(win1.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,15 +241,6 @@ 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,11 +84,6 @@ 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);