wm: On reconnect display MRU order should be reflected in stack order.

On reconnection, windows are moved to another display via a call to
Window::SetBoundsInScreen, which will add to the correct container
on the other display. Windows are currently added (if meets conditions)
in the order of the MRU, which means they are added to the new container
in that order. Stacking order of a window's children is in order of the
indexes, so visually the windows are the opposite of the MRU order.
Adding the windows in the reversed order would solve this.

On disconnection, windows are squashed onto the primary display and
stacked according to the MRU list. The formula is: Find window in MRU
list, and after that, the next window with the same parent will be the
window stacking on top target. If none is found, stack window at bottom.

Attached a video for more context in the bug.

Test: manual, PersistentWindowControllerTest.MRUOrderMatchesStacking
Bug: 1071246
Change-Id: I10a145081603a505491b4885fd2091aa4ea1c55f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2151180Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759790}

ash/display/persistent_window_controller.cc

@@ -11,6 +11,7 @@
 #include "ash/wm/window_state.h"
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/containers/adapters.h"
 #include "base/metrics/histogram_macros.h"
 #include "ui/display/manager/display_manager.h"
 #include "ui/display/screen.h"
@@ -93,7 +94,12 @@ void PersistentWindowController::MaybeRestorePersistentWindowBounds() {
 
   display::Screen* screen = display::Screen::GetScreen();
   int window_restored_count = 0;
-  for (auto* window : GetWindowList()) {
+  // Maybe add the windows to a new display via SetBoundsInScreen() depending on
+  // their persistent window info. Go backwards so that if they do get added to
+  // another root window's container, the stacking order will match the MRU
+  // order (windows added first are stacked at the bottom).
+  std::vector<aura::Window*> mru_window_list = GetWindowList();
+  for (auto* window : base::Reversed(mru_window_list)) {
     WindowState* window_state = WindowState::Get(window);
     if (!window_state->persistent_window_info())
       continue;

ash/display/persistent_window_controller_unittest.cc

@@ -9,6 +9,7 @@
 #include "ash/session/test_session_controller_client.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/wm/mru_window_tracker.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -469,4 +470,140 @@ TEST_F(PersistentWindowControllerTest, RestoreBounds) {
   EXPECT_EQ(restore_bounds_in_screen, window->GetBoundsInScreen());
 }
 
+// Tests that the MRU order is maintained visually after adding and removing a
+// display.
+TEST_F(PersistentWindowControllerTest, MRUOrderMatchesStacking) {
+  UpdateDisplay("0+0-500x500,0+501-500x500");
+
+  // Add three windows, all on the secondary display.
+  const gfx::Rect bounds(500, 0, 200, 200);
+  std::unique_ptr<aura::Window> window1 = CreateTestWindow(bounds);
+  std::unique_ptr<aura::Window> window2 = CreateTestWindow(bounds);
+  std::unique_ptr<aura::Window> window3 = CreateTestWindow(bounds);
+
+  // MRU order should be opposite of the order the windows were created. Verify
+  // that all three windows are indeed on the secondary display.
+  const int64_t primary_id = WindowTreeHostManager::GetPrimaryDisplayId();
+  const int64_t secondary_id =
+      display::test::DisplayManagerTestApi(display_manager())
+          .GetSecondaryDisplay()
+          .id();
+  display::Screen* screen = display::Screen::GetScreen();
+  const std::vector<aura::Window*> expected_mru_order = {
+      window3.get(), window2.get(), window1.get()};
+  ASSERT_EQ(
+      expected_mru_order,
+      Shell::Get()->mru_window_tracker()->BuildWindowForCycleList(kAllDesks));
+  for (auto* window : expected_mru_order)
+    ASSERT_EQ(secondary_id, screen->GetDisplayNearestWindow(window).id());
+
+  // Disconnect secondary display. The windows should move to the primary
+  // display and retain MRU ordering.
+  display::ManagedDisplayInfo primary_info =
+      display_manager()->GetDisplayInfo(primary_id);
+  display::ManagedDisplayInfo secondary_info =
+      display_manager()->GetDisplayInfo(secondary_id);
+  std::vector<display::ManagedDisplayInfo> display_info_list;
+  display_info_list.push_back(primary_info);
+  display_manager()->OnNativeDisplaysChanged(display_info_list);
+
+  // The order which the children are stacked in is the reverse of the order
+  // they are in the children() field.
+  aura::Window* parent = window1->parent();
+  ASSERT_TRUE(parent);
+  std::vector<aura::Window*> children_ordered_by_stacking = parent->children();
+  std::reverse(children_ordered_by_stacking.begin(),
+               children_ordered_by_stacking.end());
+  EXPECT_EQ(
+      expected_mru_order,
+      Shell::Get()->mru_window_tracker()->BuildWindowForCycleList(kAllDesks));
+  EXPECT_EQ(expected_mru_order, children_ordered_by_stacking);
+  EXPECT_EQ(primary_id, screen->GetDisplayNearestWindow(parent).id());
+
+  // Reconnect secondary display. The windows should move to the secondary
+  // display and retain MRU ordering.
+  display_info_list.push_back(secondary_info);
+  display_manager()->OnNativeDisplaysChanged(display_info_list);
+  parent = window1->parent();
+  children_ordered_by_stacking = parent->children();
+  std::reverse(children_ordered_by_stacking.begin(),
+               children_ordered_by_stacking.end());
+  ASSERT_TRUE(parent);
+  EXPECT_EQ(
+      expected_mru_order,
+      Shell::Get()->mru_window_tracker()->BuildWindowForCycleList(kAllDesks));
+  EXPECT_EQ(expected_mru_order, children_ordered_by_stacking);
+  EXPECT_EQ(secondary_id, screen->GetDisplayNearestWindow(parent).id());
+}
+
+// Similar to the above test but with windows created on both displays.
+TEST_F(PersistentWindowControllerTest, MRUOrderMatchesStackingInterleaved) {
+  UpdateDisplay("0+0-500x500,0+501-500x500");
+
+  // Add four windows, two on each display.
+  const gfx::Rect primary_bounds(200, 200);
+  const gfx::Rect secondary_bounds(500, 0, 200, 200);
+  std::unique_ptr<aura::Window> window1 = CreateTestWindow(primary_bounds);
+  std::unique_ptr<aura::Window> window2 = CreateTestWindow(secondary_bounds);
+  std::unique_ptr<aura::Window> window3 = CreateTestWindow(primary_bounds);
+  std::unique_ptr<aura::Window> window4 = CreateTestWindow(secondary_bounds);
+
+  // MRU order should be opposite of the order the windows were created.
+  const int64_t primary_id = WindowTreeHostManager::GetPrimaryDisplayId();
+  const int64_t secondary_id =
+      display::test::DisplayManagerTestApi(display_manager())
+          .GetSecondaryDisplay()
+          .id();
+  display::Screen* screen = display::Screen::GetScreen();
+  const std::vector<aura::Window*> expected_mru_order = {
+      window4.get(), window3.get(), window2.get(), window1.get()};
+  ASSERT_EQ(
+      expected_mru_order,
+      Shell::Get()->mru_window_tracker()->BuildWindowForCycleList(kAllDesks));
+
+  // Disconnect secondary display. The windows should move to the primary
+  // display and retain MRU ordering. Note that this logic is part of
+  // RootWindowController and not PersistentWindowController.
+  display::ManagedDisplayInfo primary_info =
+      display_manager()->GetDisplayInfo(primary_id);
+  display::ManagedDisplayInfo secondary_info =
+      display_manager()->GetDisplayInfo(secondary_id);
+  std::vector<display::ManagedDisplayInfo> display_info_list;
+  display_info_list.push_back(primary_info);
+  display_manager()->OnNativeDisplaysChanged(display_info_list);
+
+  // The order which the children are stacked in is the reverse of the order
+  // they are in the children() field.
+  aura::Window* parent = window1->parent();
+  ASSERT_TRUE(parent);
+  ASSERT_EQ(parent, window2->parent());
+  std::vector<aura::Window*> children_ordered_by_stacking = parent->children();
+  std::reverse(children_ordered_by_stacking.begin(),
+               children_ordered_by_stacking.end());
+  EXPECT_EQ(
+      expected_mru_order,
+      Shell::Get()->mru_window_tracker()->BuildWindowForCycleList(kAllDesks));
+  EXPECT_EQ(expected_mru_order, children_ordered_by_stacking);
+  EXPECT_EQ(primary_id, screen->GetDisplayNearestWindow(parent).id());
+
+  // Reconnect secondary display. |window2| and |window4| should move back to
+  // the secondary display.
+  display_info_list.push_back(secondary_info);
+  display_manager()->OnNativeDisplaysChanged(display_info_list);
+  EXPECT_EQ(
+      expected_mru_order,
+      Shell::Get()->mru_window_tracker()->BuildWindowForCycleList(kAllDesks));
+  parent = window1->parent();
+  EXPECT_EQ(primary_id, screen->GetDisplayNearestWindow(parent).id());
+  ASSERT_EQ(2u, parent->children().size());
+  EXPECT_EQ(window1.get(), parent->children()[0]);
+  EXPECT_EQ(window3.get(), parent->children()[1]);
+
+  parent = window2->parent();
+  EXPECT_EQ(secondary_id, screen->GetDisplayNearestWindow(parent).id());
+  ASSERT_EQ(2u, parent->children().size());
+  EXPECT_EQ(window2.get(), parent->children()[0]);
+  EXPECT_EQ(window4.get(), parent->children()[1]);
+}
+
 }  // namespace ash

ash/root_window_controller.cc

@@ -53,6 +53,7 @@
 #include "ash/wm/fullscreen_window_finder.h"
 #include "ash/wm/lock_action_handler_layout_manager.h"
 #include "ash/wm/lock_layout_manager.h"
+#include "ash/wm/mru_window_tracker.h"
 #include "ash/wm/overlay_layout_manager.h"
 #include "ash/wm/overview/overview_controller.h"
 #include "ash/wm/overview/overview_session.h"
@@ -247,9 +248,12 @@ void ReparentAllWindows(aura::Window* src, aura::Window* dst) {
       container_ids.emplace_back(id);
   }
 
+  const std::vector<aura::Window*> mru_list =
+      Shell::Get()->mru_window_tracker()->BuildWindowForCycleList(kAllDesks);
   for (int id : container_ids) {
     aura::Window* src_container = src->GetChildById(id);
     aura::Window* dst_container = dst->GetChildById(id);
+    const bool switchable_container = IsSwitchableContainer(src_container);
     while (!src_container->children().empty()) {
       // Restart iteration from the source container windows each time as they
       // may change as a result of moving other windows.
@@ -267,7 +271,37 @@ void ReparentAllWindows(aura::Window* src, aura::Window* dst) {
       // |iter| is invalidated after ReparentWindow. Cache it to use afterwards.
       aura::Window* const window = *iter;
       ReparentWindow(window, dst_container);
-      dst_container->StackChildAtBottom(window);
+
+      aura::Window* stacking_target = nullptr;
+      if (switchable_container) {
+        // Find the first window that comes after |window| in the MRU list that
+        // shares the same parent.
+        bool found_window = false;
+        for (aura::Window* window_iter : mru_list) {
+          // First determine the position of |window| in the |mru_list|.
+          if (!found_window && window == window_iter) {
+            found_window = true;
+            continue;
+          }
+
+          if (!found_window || window_iter->parent() != dst_container)
+            continue;
+
+          // Once |window| is found, the next item in |mru_list| with the same
+          // parent (container) is the stacking target.
+          stacking_target = window_iter;
+          break;
+        }
+      }
+
+      // |stacking_target| may be null if |switchable_container| is false, which
+      // means the children of that container wouldn't be in the MRU list or if
+      // |window| was the last item in the MRU list with parent id |id|. In
+      // this case stack |window| at the bottom.
+      if (stacking_target)
+        dst_container->StackChildAbove(window, stacking_target);
+      else
+        dst_container->StackChildAtBottom(window);
     }
   }
 }

ash/root_window_controller_unittest.cc

@@ -347,23 +347,26 @@ TEST_F(RootWindowControllerTest, MoveWindows_LockWindowsInUnified) {
   EXPECT_EQ("0,0 600x500", lock_screen->GetNativeWindow()->bounds().ToString());
 }
 
-// Tests that the moved windows (except the active one) are put under existing
-// ones.
-TEST_F(RootWindowControllerTest, MoveWindows_UnderExisting) {
+// Tests that the moved windows maintain MRU ordering.
+TEST_F(RootWindowControllerTest, MoveWindows_MaintainMRUordering) {
   UpdateDisplay("600x600,300x300");
 
   display::Screen* screen = display::Screen::GetScreen();
   const display::Display primary_display = screen->GetPrimaryDisplay();
   const display::Display secondary_display = GetSecondaryDisplay();
 
-  views::Widget* existing = CreateTestWidget(gfx::Rect(0, 10, 100, 100));
+  views::Widget* existing1 = CreateTestWidget(gfx::Rect(0, 10, 100, 100));
   ASSERT_EQ(primary_display.id(),
-            screen->GetDisplayNearestWindow(existing->GetNativeWindow()).id());
+            screen->GetDisplayNearestWindow(existing1->GetNativeWindow()).id());
 
   views::Widget* moved = CreateTestWidget(gfx::Rect(650, 10, 100, 100));
   ASSERT_EQ(secondary_display.id(),
             screen->GetDisplayNearestWindow(moved->GetNativeWindow()).id());
 
+  views::Widget* existing2 = CreateTestWidget(gfx::Rect(0, 10, 100, 100));
+  ASSERT_EQ(primary_display.id(),
+            screen->GetDisplayNearestWindow(existing2->GetNativeWindow()).id());
+
   views::Widget* active = CreateTestWidget(gfx::Rect(650, 10, 100, 100));
   ASSERT_TRUE(active->IsActive());
   ASSERT_EQ(secondary_display.id(),
@@ -375,20 +378,14 @@ TEST_F(RootWindowControllerTest, MoveWindows_UnderExisting) {
   // |active| is still active.
   ASSERT_TRUE(active->IsActive());
 
-  // |moved| should be put under |existing|.
-  ASSERT_EQ(moved->GetNativeWindow()->parent(),
-            existing->GetNativeWindow()->parent());
-  bool found_existing = false;
-  bool found_moved = false;
-  for (auto* child : moved->GetNativeWindow()->parent()->children()) {
-    if (child == existing->GetNativeWindow())
-      found_existing = true;
-    if (child == moved->GetNativeWindow()) {
-      found_moved = true;
-      break;
-    }
-  }
-  EXPECT_TRUE(found_moved && !found_existing);
+  // |moved| should be put between |existing2| and |existing1| to maintain MRU
+  // ordering.
+  aura::Window* parent = moved->GetNativeWindow()->parent();
+  ASSERT_EQ(parent, existing1->GetNativeWindow()->parent());
+  const std::vector<aura::Window*> expected_order = {
+      existing1->GetNativeWindow(), moved->GetNativeWindow(),
+      existing2->GetNativeWindow(), active->GetNativeWindow()};
+  EXPECT_EQ(expected_order, parent->children());
 }
 
 TEST_F(RootWindowControllerTest, ModalContainer) {