chromeos/mus: fix bug where state not updated on display change

Specifically if an ancestor of the focused window moves to a different
display, then the FocusController is left in a base state. This is
because the FocusController is per display.

BUG=none
TEST=covered by test

Change-Id: I3fbcaf91f5991bd5bb3a8aa7113503937e4c97a2
Reviewed-on: https://chromium-review.googlesource.com/574927Reviewed-by: Elliot Glaysher <erg@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487210}

services/ui/ws/focus_controller.cc

@@ -238,12 +238,8 @@ bool FocusController::SetFocusedWindowImpl(
   return true;
 }
 
-void FocusController::OnDrawnStateWillChange(ServerWindow* ancestor,
-                                             ServerWindow* window,
-                                             bool is_drawn) {
-  DCHECK(!is_drawn);
-  DCHECK_NE(ancestor, window);
-  DCHECK(root_->Contains(window));
+void FocusController::ProcessDrawnOrRootChange(ServerWindow* ancestor,
+                                               ServerWindow* window) {
   drawn_tracker_.reset();
 
   // Find the window that triggered this state-change notification.
@@ -297,11 +293,25 @@ void FocusController::OnDrawnStateWillChange(ServerWindow* ancestor,
   SetFocusedWindowImpl(FocusControllerChangeSource::DRAWN_STATE_CHANGED, focus);
 }
 
+void FocusController::OnDrawnStateWillChange(ServerWindow* ancestor,
+                                             ServerWindow* window,
+                                             bool is_drawn) {
+  DCHECK(!is_drawn);
+  DCHECK_NE(ancestor, window);
+  DCHECK(root_->Contains(window));
+  ProcessDrawnOrRootChange(ancestor, window);
+}
+
 void FocusController::OnDrawnStateChanged(ServerWindow* ancestor,
                                           ServerWindow* window,
                                           bool is_drawn) {
   // DCHECK(false);  TODO(sadrul):
 }
 
+void FocusController::OnRootWillChange(ServerWindow* ancestor,
+                                       ServerWindow* window) {
+  ProcessDrawnOrRootChange(ancestor, window);
+}
+
 }  // namespace ws
 }  // namespace ui

services/ui/ws/focus_controller.h

@@ -67,6 +67,10 @@ class FocusController : public ServerWindowDrawnTrackerObserver {
   bool SetFocusedWindowImpl(FocusControllerChangeSource change_source,
                             ServerWindow* window);
 
+  // Called internally when the drawn state or root changes. |ancestor| is
+  // the |ancestor| argument from those two functions, see them for details.
+  void ProcessDrawnOrRootChange(ServerWindow* ancestor, ServerWindow* window);
+
   // ServerWindowDrawnTrackerObserver:
   void OnDrawnStateWillChange(ServerWindow* ancestor,
                               ServerWindow* window,
@@ -74,6 +78,7 @@ class FocusController : public ServerWindowDrawnTrackerObserver {
   void OnDrawnStateChanged(ServerWindow* ancestor,
                            ServerWindow* window,
                            bool is_drawn) override;
+  void OnRootWillChange(ServerWindow* ancestor, ServerWindow* window) override;
 
   FocusControllerDelegate* delegate_;
 

services/ui/ws/focus_controller_unittest.cc

@@ -308,5 +308,57 @@ TEST(FocusControllerTest, NonFocusableWindowNotActivated) {
   EXPECT_EQ(&parent, focus_observer.new_active_window());
 }
 
+namespace {
+
+// ServerWindowDelegate implementation whose GetRootWindow() implementation
+// returns the last ancestor as the root of a window.
+class TestServerWindowDelegate2 : public ServerWindowDelegate {
+ public:
+  TestServerWindowDelegate2() = default;
+  ~TestServerWindowDelegate2() override = default;
+
+  // ServerWindowDelegate:
+  cc::mojom::FrameSinkManager* GetFrameSinkManager() override {
+    return nullptr;
+  }
+  ServerWindow* GetRootWindow(const ServerWindow* window) override {
+    const ServerWindow* root = window;
+    while (root && root->parent())
+      root = root->parent();
+    // TODO(sky): this cast shouldn't be necessary!
+    return const_cast<ServerWindow*>(root);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestServerWindowDelegate2);
+};
+
+}  // namespace
+
+TEST(FocusControllerTest, ActiveWindowMovesToDifferentDisplay) {
+  TestServerWindowDelegate2 server_window_delegate;
+  ServerWindow root1(&server_window_delegate, WindowId());
+  root1.SetVisible(true);
+  ServerWindow root2(&server_window_delegate, WindowId());
+  root2.SetVisible(true);
+
+  ServerWindow child(&server_window_delegate, WindowId());
+  root1.Add(&child);
+  child.SetVisible(true);
+
+  TestFocusControllerObserver focus_observer;
+  focus_observer.set_ignore_explicit(false);
+  FocusController focus_controller(&focus_observer, &root1);
+  focus_controller.AddObserver(&focus_observer);
+
+  focus_controller.SetFocusedWindow(&child);
+  EXPECT_EQ(&child, focus_controller.GetFocusedWindow());
+
+  root2.Add(&child);
+  // As the focused window is moving to a different root focus should move
+  // to the root.
+  EXPECT_EQ(&root1, focus_controller.GetFocusedWindow());
+}
+
 }  // namespace ws
 }  // namespace ui

services/ui/ws/server_window_drawn_tracker.cc

@@ -14,7 +14,10 @@ namespace ws {
 ServerWindowDrawnTracker::ServerWindowDrawnTracker(
     ServerWindow* window,
     ServerWindowDrawnTrackerObserver* observer)
-    : window_(window), observer_(observer), drawn_(window->IsDrawn()) {
+    : window_(window),
+      observer_(observer),
+      drawn_(window->IsDrawn()),
+      weak_factory_(this) {
   AddObservers();
 }
 
@@ -38,13 +41,19 @@ void ServerWindowDrawnTracker::SetDrawn(ServerWindow* ancestor, bool drawn) {
 }
 
 void ServerWindowDrawnTracker::AddObservers() {
-  if (!window_)
+  if (!window_) {
+    root_ = nullptr;
     return;
+  }
 
+  ServerWindow* last = window_;
   for (ServerWindow* v = window_; v; v = v->parent()) {
     v->AddObserver(this);
     windows_.insert(v);
+    last = v;
   }
+  DCHECK(last);
+  root_ = last;
 }
 
 void ServerWindowDrawnTracker::RemoveObservers() {
@@ -81,9 +90,16 @@ void ServerWindowDrawnTracker::OnWillChangeWindowHierarchy(
     }
   }
   if (drawn_ != new_is_drawn) {
+    auto ref = weak_factory_.GetWeakPtr();
     observer_->OnDrawnStateWillChange(new_is_drawn ? nullptr : old_parent,
                                       window_, new_is_drawn);
+    // Allow for the |observer_| to delete |this|.
+    if (!ref)
+      return;
   }
+
+  if (!root_->Contains(new_parent))
+    observer_->OnRootWillChange(old_parent, window_);
 }
 
 void ServerWindowDrawnTracker::OnWindowHierarchyChanged(

services/ui/ws/server_window_drawn_tracker.h

@@ -8,6 +8,7 @@
 #include <set>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "services/ui/ws/server_window_observer.h"
 
 namespace ui {
@@ -49,12 +50,15 @@ class ServerWindowDrawnTracker : public ServerWindowObserver {
   void OnWillChangeWindowVisibility(ServerWindow* window) override;
   void OnWindowVisibilityChanged(ServerWindow* window) override;
 
+  ServerWindow* root_ = nullptr;
   ServerWindow* window_;
   ServerWindowDrawnTrackerObserver* observer_;
   bool drawn_;
   // Set of windows we're observing. This is |window_| and all its ancestors.
   std::set<ServerWindow*> windows_;
 
+  base::WeakPtrFactory<ServerWindowDrawnTracker> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ServerWindowDrawnTracker);
 };
 

services/ui/ws/server_window_drawn_tracker_observer.h

@@ -30,6 +30,10 @@ class ServerWindowDrawnTrackerObserver {
                                    ServerWindow* window,
                                    bool is_drawn) {}
 
+  // Invoked if the root will change as the result of a child of |ancestor|
+  // being moved to a new root.
+  virtual void OnRootWillChange(ServerWindow* ancestor, ServerWindow* window) {}
+
  protected:
   virtual ~ServerWindowDrawnTrackerObserver() {}
 };

services/ui/ws/server_window_drawn_tracker_unittest.cc

@@ -20,17 +20,17 @@ namespace {
 class TestServerWindowDrawnTrackerObserver
     : public ServerWindowDrawnTrackerObserver {
  public:
-  TestServerWindowDrawnTrackerObserver()
-      : change_count_(0u),
-        ancestor_(nullptr),
-        window_(nullptr),
-        is_drawn_(false) {}
+  TestServerWindowDrawnTrackerObserver() = default;
 
-  void clear_change_count() { change_count_ = 0u; }
+  void clear_change_count() {
+    change_count_ = 0u;
+    root_changed_count_ = 0u;
+  }
   size_t change_count() const { return change_count_; }
   const ServerWindow* ancestor() const { return ancestor_; }
   const ServerWindow* window() const { return window_; }
   bool is_drawn() const { return is_drawn_; }
+  size_t root_changed_count() const { return root_changed_count_; }
 
  private:
   // ServerWindowDrawnTrackerObserver:
@@ -50,11 +50,15 @@ class TestServerWindowDrawnTrackerObserver
     EXPECT_EQ(window_, window);
     EXPECT_EQ(is_drawn_, is_drawn);
   }
+  void OnRootWillChange(ServerWindow* ancestor, ServerWindow* window) override {
+    root_changed_count_++;
+  }
 
-  size_t change_count_;
-  const ServerWindow* ancestor_;
-  const ServerWindow* window_;
-  bool is_drawn_;
+  size_t change_count_ = 0u;
+  size_t root_changed_count_ = 0u;
+  const ServerWindow* ancestor_ = nullptr;
+  const ServerWindow* window_ = nullptr;
+  bool is_drawn_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestServerWindowDrawnTrackerObserver);
 };
@@ -70,6 +74,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfDeletionAndVisibility) {
   ServerWindowDrawnTracker tracker(window.get(), &drawn_observer);
   window->SetVisible(true);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(window.get(), drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_TRUE(drawn_observer.is_drawn());
@@ -77,6 +82,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfDeletionAndVisibility) {
 
   window->SetVisible(false);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(window.get(), drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_FALSE(drawn_observer.is_drawn());
@@ -84,6 +90,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfDeletionAndVisibility) {
 
   window->SetVisible(true);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(window.get(), drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_TRUE(drawn_observer.is_drawn());
@@ -92,6 +99,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfDeletionAndVisibility) {
   ServerWindow* old_window = window.get();
   window.reset();
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(old_window, drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_FALSE(drawn_observer.is_drawn());
@@ -110,6 +118,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfRemovingFromRoot) {
   ServerWindowDrawnTracker tracker(&child, &drawn_observer);
   root.Remove(&child);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(1u, drawn_observer.root_changed_count());
   EXPECT_EQ(&child, drawn_observer.window());
   EXPECT_EQ(&root, drawn_observer.ancestor());
   EXPECT_FALSE(drawn_observer.is_drawn());
@@ -117,6 +126,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfRemovingFromRoot) {
 
   root.Add(&child);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(1u, drawn_observer.root_changed_count());
   EXPECT_EQ(&child, drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_TRUE(drawn_observer.is_drawn());
@@ -139,6 +149,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfRemovingAncestorFromRoot) {
   ServerWindowDrawnTracker tracker(&child_child, &drawn_observer);
   root.Remove(&child);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(1u, drawn_observer.root_changed_count());
   EXPECT_EQ(&child_child, drawn_observer.window());
   EXPECT_EQ(&root, drawn_observer.ancestor());
   EXPECT_FALSE(drawn_observer.is_drawn());
@@ -146,6 +157,7 @@ TEST(ServerWindowDrawnTrackerTest, ChangeBecauseOfRemovingAncestorFromRoot) {
 
   root.Add(&child_child);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(1u, drawn_observer.root_changed_count());
   EXPECT_EQ(&child_child, drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_TRUE(drawn_observer.is_drawn());
@@ -177,6 +189,7 @@ TEST(ServerWindowDrawnTrackerTest, VisibilityChangeFromNonParentAncestor) {
   // is still invisible.
   child1.SetVisible(true);
   EXPECT_EQ(0u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(nullptr, drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_FALSE(drawn_observer.is_drawn());
@@ -184,6 +197,7 @@ TEST(ServerWindowDrawnTrackerTest, VisibilityChangeFromNonParentAncestor) {
 
   child2.SetVisible(true);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(&child3, drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_TRUE(drawn_observer.is_drawn());
@@ -217,6 +231,7 @@ TEST(ServerWindowDrawnTrackerTest, TreeHierarchyChangeFromNonParentAncestor) {
   // Move |child11| as a child of |child2|. |child111| should remain not drawn.
   child2.Add(&child11);
   EXPECT_EQ(0u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(nullptr, drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_FALSE(drawn_observer.is_drawn());
@@ -224,6 +239,7 @@ TEST(ServerWindowDrawnTrackerTest, TreeHierarchyChangeFromNonParentAncestor) {
 
   child11.SetVisible(true);
   EXPECT_EQ(1u, drawn_observer.change_count());
+  EXPECT_EQ(0u, drawn_observer.root_changed_count());
   EXPECT_EQ(&child111, drawn_observer.window());
   EXPECT_EQ(nullptr, drawn_observer.ancestor());
   EXPECT_TRUE(drawn_observer.is_drawn());