wm: Only store persistent bounds when removing display.

Without this, we can get some cases where a non-remove display change
can get some persistent bounds stored and won't get overwritten when
a remove display change happens.

Introduce a mechanism where persistent bounds are calculated when a
display change signal is received, but not written to the window
state unless a remove signal is received.

Test: manual, added tests
Bug: 1115675
Change-Id: Id3b642d5912510c451e78a0f9227437d1ce2de1b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2353065Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797895}

ash/display/persistent_window_controller.cc

@@ -46,11 +46,9 @@ constexpr char PersistentWindowController::kNumOfWindowsRestoredHistogramName[];
 
 PersistentWindowController::PersistentWindowController() {
   display::Screen::GetScreen()->AddObserver(this);
-  Shell::Get()->window_tree_host_manager()->AddObserver(this);
 }
 
 PersistentWindowController::~PersistentWindowController() {
-  Shell::Get()->window_tree_host_manager()->RemoveObserver(this);
   display::Screen::GetScreen()->RemoveObserver(this);
 }
 
@@ -64,7 +62,10 @@ void PersistentWindowController::OnWillProcessDisplayChanges() {
     // to restore, or until they're cleared by user-invoked bounds change.
     if (window_state->persistent_window_info())
       continue;
-    window_state->SetPersistentWindowInfo(PersistentWindowInfo(window));
+    // Place the window that needs persistent window info into the temporary
+    // set. The persistent window info will be created and set if a display is
+    // removed.
+    need_persistent_info_windows_.Add(window);
   }
 }
 
@@ -75,16 +76,25 @@ void PersistentWindowController::OnDisplayAdded(
       base::Unretained(this));
 }
 
-void PersistentWindowController::OnDisplayConfigurationChanged() {
+void PersistentWindowController::OnDisplayRemoved(
+    const display::Display& old_display) {
+  for (aura::Window* window : need_persistent_info_windows_.windows()) {
+    WindowState* window_state = WindowState::Get(window);
+    window_state->SetPersistentWindowInfo(PersistentWindowInfo(window));
+  }
+  need_persistent_info_windows_.RemoveAll();
+}
+
+void PersistentWindowController::OnDidProcessDisplayChanges() {
   if (restore_callback_)
     std::move(restore_callback_).Run();
+  need_persistent_info_windows_.RemoveAll();
 }
 
 void PersistentWindowController::MaybeRestorePersistentWindowBounds() {
   if (!ShouldProcessWindowList())
     return;
 
-  display::Screen* screen = display::Screen::GetScreen();
   int window_restored_count = 0;
   // 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
@@ -98,8 +108,6 @@ void PersistentWindowController::MaybeRestorePersistentWindowBounds() {
     PersistentWindowInfo persistent_window_info =
         *window_state->persistent_window_info();
     const int64_t persistent_display_id = persistent_window_info.display_id;
-    if (persistent_display_id == screen->GetDisplayNearestWindow(window).id())
-      continue;
     auto* display_manager = GetDisplayManager();
     if (!display_manager->IsDisplayIdValid(persistent_display_id))
       continue;
@@ -115,7 +123,8 @@ void PersistentWindowController::MaybeRestorePersistentWindowBounds() {
         persistent_window_info.display_bounds_in_screen;
     // It is possible to have display size change, such as changing cable, bad
     // cable signal etc., but it should be rare.
-    DCHECK(display.bounds().size() == persistent_display_bounds.size());
+    if (display.bounds().size() != persistent_display_bounds.size())
+      continue;
     const gfx::Vector2d offset = display.bounds().OffsetFromOrigin() -
                                  persistent_display_bounds.OffsetFromOrigin();
     persistent_window_bounds.Offset(offset);

ash/display/persistent_window_controller.h

@@ -6,17 +6,15 @@
 #define ASH_DISPLAY_PERSISTENT_WINDOW_CONTROLLER_H_
 
 #include "ash/ash_export.h"
-#include "ash/display/window_tree_host_manager.h"
 #include "base/callback.h"
+#include "ui/aura/window_tracker.h"
 #include "ui/display/display_observer.h"
 
 namespace ash {
 
 // Observes display changes and saves/restores window bounds persistently in
 // multi-displays scenario.
-class ASH_EXPORT PersistentWindowController
-    : public display::DisplayObserver,
-      public WindowTreeHostManager::Observer {
+class ASH_EXPORT PersistentWindowController : public display::DisplayObserver {
  public:
   // Public so it can be used by unit tests.
   constexpr static char kNumOfWindowsRestoredHistogramName[] =
@@ -32,15 +30,18 @@ class ASH_EXPORT PersistentWindowController
   // display::DisplayObserver:
   void OnWillProcessDisplayChanges() override;
   void OnDisplayAdded(const display::Display& new_display) override;
-
-  // WindowTreeHostManager::Observer:
-  void OnDisplayConfigurationChanged() override;
+  void OnDisplayRemoved(const display::Display& old_display) override;
+  void OnDidProcessDisplayChanges() override;
 
   // Called when restoring persistent window placement is wanted.
   void MaybeRestorePersistentWindowBounds();
 
-  // Callback binded on display added and run on display configuration changed.
-  base::OnceCallback<void()> restore_callback_;
+  // Callback binded on display added and run on display changes are processed.
+  base::OnceClosure restore_callback_;
+
+  // Temporary storage that stores windows that may need persistent info
+  // stored on display removal. Cleared when display changes are processed.
+  aura::WindowTracker need_persistent_info_windows_;
 };
 
 }  // namespace ash

ash/display/persistent_window_controller_unittest.cc

@@ -606,4 +606,43 @@ TEST_F(PersistentWindowControllerTest, MRUOrderMatchesStackingInterleaved) {
   EXPECT_EQ(window4.get(), parent->children()[1]);
 }
 
+// Tests that if a window is on a primary display which gets disconnected, on
+// reconnect the windows bounds will be persisted.
+TEST_F(PersistentWindowControllerTest, DisconnectingPrimaryDisplay) {
+  // Create two displays with the one higher resolution.
+  UpdateDisplay("0+0-500x500,0+501-1500x500");
+  const int64_t small_id = WindowTreeHostManager::GetPrimaryDisplayId();
+  const int64_t large_id =
+      display::test::DisplayManagerTestApi(display_manager())
+          .GetSecondaryDisplay()
+          .id();
+
+  // Set the larger display to be the primary display.
+  Shell::Get()->window_tree_host_manager()->SetPrimaryDisplayId(large_id);
+  ASSERT_EQ(large_id, WindowTreeHostManager::GetPrimaryDisplayId());
+
+  // Add a window on the larger display.
+  const gfx::Rect bounds(0, 200, 1500, 200);
+  std::unique_ptr<aura::Window> window = CreateTestWindow(bounds);
+
+  // Disconnect the large display. The windows should move to the new primary
+  // display (small display) and shrink to fit.
+  display::ManagedDisplayInfo small_info =
+      display_manager()->GetDisplayInfo(small_id);
+  display::ManagedDisplayInfo large_info =
+      display_manager()->GetDisplayInfo(large_id);
+  std::vector<display::ManagedDisplayInfo> display_info_list;
+  display_info_list.push_back(small_info);
+  display_manager()->OnNativeDisplaysChanged(display_info_list);
+  EXPECT_EQ(small_id, WindowTreeHostManager::GetPrimaryDisplayId());
+  EXPECT_EQ(gfx::Size(500, 200), window->bounds().size());
+
+  // Reconnect the large display. The window should move back and have the old
+  // size.
+  display_info_list.push_back(large_info);
+  display_manager()->OnNativeDisplaysChanged(display_info_list);
+  EXPECT_EQ(large_id, WindowTreeHostManager::GetPrimaryDisplayId());
+  EXPECT_EQ(gfx::Size(1500, 200), window->bounds().size());
+}
+
 }  // namespace ash