Map new subtrees when they are attached to the node hierarchy visible to a connection.

R=sky@chromium.org
http://crbug.com/365012

Review URL: https://codereview.chromium.org/290703002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271493 0039d316-1c4b-4281-b951-d872f2087c98

mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc

@@ -51,6 +51,30 @@ ViewTreeNode* AddNodeToViewManager(ViewManager* manager,
   return node;
 }
 
+ViewTreeNode* BuildNodeTree(ViewManager* manager,
+                            const Array<INode>& nodes) {
+  std::vector<ViewTreeNode*> parents;
+  ViewTreeNode* root = NULL;
+  ViewTreeNode* last_node = NULL;
+  for (size_t i = 0; i < nodes.size(); ++i) {
+    if (last_node && nodes[i].parent_id() == last_node->id()) {
+      parents.push_back(last_node);
+    } else if (!parents.empty()) {
+      while (parents.back()->id() != nodes[i].parent_id())
+        parents.pop_back();
+    }
+    ViewTreeNode* node = AddNodeToViewManager(
+        manager,
+        !parents.empty() ? parents.back() : NULL,
+        nodes[i].node_id(),
+        nodes[i].view_id());
+    if (!last_node)
+      root = node;
+    last_node = node;
+  }
+  return root;
+}
+
 class ViewManagerTransaction {
  public:
   virtual ~ViewManagerTransaction() {}
@@ -63,13 +87,6 @@ class ViewManagerTransaction {
 
   bool committed() const { return committed_; }
 
-  // General callback to be used for commits to the service.
-  void OnActionCompleted(bool success) {
-    DCHECK(success);
-    DoActionCompleted(success);
-    synchronizer_->RemoveFromPendingQueue(this);
-  }
-
  protected:
   enum TransactionType {
     // View creation and destruction.
@@ -105,7 +122,19 @@ class ViewManagerTransaction {
     return synchronizer_->next_server_change_id_++;
   }
 
+  base::Callback<void(bool)> ActionCompletedCallback() {
+    return base::Bind(&ViewManagerTransaction::OnActionCompleted,
+                      base::Unretained(this));
+  }
+
  private:
+  // General callback to be used for commits to the service.
+  void OnActionCompleted(bool success) {
+    DCHECK(success);
+    DoActionCompleted(success);
+    synchronizer_->RemoveFromPendingQueue(this);
+  }
+
   const TransactionType transaction_type_;
   bool committed_;
   ViewManagerSynchronizer* synchronizer_;
@@ -124,10 +153,7 @@ class CreateViewTransaction : public ViewManagerTransaction {
  private:
   // Overridden from ViewManagerTransaction:
   virtual void DoCommit() OVERRIDE {
-    service()->CreateView(
-        view_id_,
-        base::Bind(&ViewManagerTransaction::OnActionCompleted,
-                   base::Unretained(this)));
+    service()->CreateView(view_id_, ActionCompletedCallback());
   }
   virtual void DoActionCompleted(bool success) OVERRIDE {
     // TODO(beng): failure.
@@ -149,10 +175,7 @@ class DestroyViewTransaction : public ViewManagerTransaction {
  private:
   // Overridden from ViewManagerTransaction:
   virtual void DoCommit() OVERRIDE {
-    service()->DeleteView(
-        view_id_,
-        base::Bind(&ViewManagerTransaction::OnActionCompleted,
-                   base::Unretained(this)));
+    service()->DeleteView(view_id_, ActionCompletedCallback());
   }
   virtual void DoActionCompleted(bool success) OVERRIDE {
     // TODO(beng): recovery?
@@ -174,10 +197,7 @@ class CreateViewTreeNodeTransaction : public ViewManagerTransaction {
  private:
   // Overridden from ViewManagerTransaction:
   virtual void DoCommit() OVERRIDE {
-    service()->CreateNode(
-        node_id_,
-        base::Bind(&ViewManagerTransaction::OnActionCompleted,
-                   base::Unretained(this)));
+    service()->CreateNode(node_id_, ActionCompletedCallback());
   }
   virtual void DoActionCompleted(bool success) OVERRIDE {
     // TODO(beng): Failure means we tried to create with an extant id for this
@@ -202,10 +222,7 @@ class DestroyViewTreeNodeTransaction : public ViewManagerTransaction {
  private:
   // Overridden from ViewManagerTransaction:
   virtual void DoCommit() OVERRIDE {
-    service()->DeleteNode(
-        node_id_,
-        base::Bind(&ViewManagerTransaction::OnActionCompleted,
-                   base::Unretained(this)));
+    service()->DeleteNode(node_id_, ActionCompletedCallback());
   }
   virtual void DoActionCompleted(bool success) OVERRIDE {
     // TODO(beng): recovery?
@@ -240,15 +257,13 @@ class HierarchyTransaction : public ViewManagerTransaction {
             parent_id_,
             child_id_,
             GetAndAdvanceNextServerChangeId(),
-            base::Bind(&ViewManagerTransaction::OnActionCompleted,
-                       base::Unretained(this)));
+            ActionCompletedCallback());
         break;
       case TYPE_REMOVE:
         service()->RemoveNodeFromParent(
             child_id_,
             GetAndAdvanceNextServerChangeId(),
-            base::Bind(&ViewManagerTransaction::OnActionCompleted,
-                       base::Unretained(this)));
+            ActionCompletedCallback());
         break;
     }
   }
@@ -278,11 +293,7 @@ class SetActiveViewTransaction : public ViewManagerTransaction {
  private:
   // Overridden from ViewManagerTransaction:
   virtual void DoCommit() OVERRIDE {
-    service()->SetView(
-        node_id_,
-        view_id_,
-        base::Bind(&ViewManagerTransaction::OnActionCompleted,
-                   base::Unretained(this)));
+    service()->SetView(node_id_, view_id_, ActionCompletedCallback());
   }
   virtual void DoActionCompleted(bool success) OVERRIDE {
     // TODO(beng): recovery?
@@ -394,32 +405,13 @@ void ViewManagerSynchronizer::SetActiveView(TransportNodeId node_id,
 void ViewManagerSynchronizer::OnConnectionEstablished(
     TransportConnectionId connection_id,
     TransportChangeId next_server_change_id,
-    const mojo::Array<INode>& nodes) {
+    const Array<INode>& nodes) {
   connected_ = true;
   connection_id_ = connection_id;
   next_server_change_id_ = next_server_change_id;
 
-  ViewManagerPrivate private_manager(view_manager_);
-  std::vector<ViewTreeNode*> parents;
-  ViewTreeNode* root = NULL;
-  ViewTreeNode* last_node = NULL;
-  for (size_t i = 0; i < nodes.size(); ++i) {
-    if (last_node && nodes[i].parent_id() == last_node->id()) {
-      parents.push_back(last_node);
-    } else if (!parents.empty()) {
-      while (parents.back()->id() != nodes[i].parent_id())
-        parents.pop_back();
-    }
-    ViewTreeNode* node =
-        AddNodeToViewManager(view_manager_,
-                             !parents.empty() ? parents.back() : NULL,
-                             nodes[i].node_id(),
-                             nodes[i].view_id());
-    if (!last_node)
-      root = node;
-    last_node = node;
-  }
-  private_manager.set_root(root);
+  ViewManagerPrivate(view_manager_).set_root(
+      BuildNodeTree(view_manager_, nodes));
   if (init_loop_)
     init_loop_->Quit();
 
@@ -436,30 +428,15 @@ void ViewManagerSynchronizer::OnNodeHierarchyChanged(
     uint32_t new_parent_id,
     uint32_t old_parent_id,
     TransportChangeId server_change_id,
-    const mojo::Array<INode>& nodes) {
+    const Array<INode>& nodes) {
   // TODO: deal with |nodes|.
   next_server_change_id_ = server_change_id + 1;
 
-  ViewTreeNode* new_parent =
-      view_manager_->tree()->GetChildById(new_parent_id);
-  ViewTreeNode* old_parent =
-      view_manager_->tree()->GetChildById(old_parent_id);
-  ViewTreeNode* node = NULL;
-  if (old_parent) {
-    // Existing node, mapped in this connection's tree.
-    // TODO(beng): verify this is actually true.
-    node = view_manager_->GetNodeById(node_id);
-    DCHECK_EQ(node->parent(), old_parent);
-  } else {
-    // New node, originating from another connection.
-    node = ViewTreeNodePrivate::LocalCreate();
-    ViewTreeNodePrivate private_node(node);
-    private_node.set_view_manager(view_manager_);
-    private_node.set_id(node_id);
-    ViewManagerPrivate(view_manager_).AddNode(node->id(), node);
+  BuildNodeTree(view_manager_, nodes);
 
-    // TODO(beng): view changes.
-  }
+  ViewTreeNode* new_parent = view_manager_->GetNodeById(new_parent_id);
+  ViewTreeNode* old_parent = view_manager_->GetNodeById(old_parent_id);
+  ViewTreeNode* node = view_manager_->GetNodeById(node_id);
   if (new_parent)
     ViewTreeNodePrivate(new_parent).LocalAddChild(node);
   else
@@ -520,6 +497,8 @@ void ViewManagerSynchronizer::RemoveFromPendingQueue(
     ViewManagerTransaction* transaction) {
   DCHECK_EQ(transaction, pending_transactions_.front());
   pending_transactions_.erase(pending_transactions_.begin());
+  if (pending_transactions_.empty() && !changes_acked_callback_.is_null())
+    changes_acked_callback_.Run();
 }
 
 }  // namespace view_manager

mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h

@@ -6,6 +6,7 @@
 #define MOJO_SERVICES_PUBLIC_CPP_VIEW_MANAGER_LIB_VIEW_MANAGER_SYNCHRONIZER_H_
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "mojo/services/public/cpp/view_manager/view_manager_types.h"
@@ -48,6 +49,13 @@ class ViewManagerSynchronizer : public IViewManagerClient {
 
   void SetActiveView(TransportNodeId node_id, TransportViewId view_id);
 
+  void set_changes_acked_callback(const base::Callback<void(void)>& callback) {
+    changes_acked_callback_ = callback;
+  }
+  void ClearChangesAckedCallback() {
+    changes_acked_callback_ = base::Callback<void(void)>();
+  }
+
  private:
   friend class ViewManagerTransaction;
   typedef ScopedVector<ViewManagerTransaction> Transactions;
@@ -94,6 +102,8 @@ class ViewManagerSynchronizer : public IViewManagerClient {
   // construction.
   base::RunLoop* init_loop_;
 
+  base::Callback<void(void)> changes_acked_callback_;
+
   IViewManagerPtr service_;
 
   DISALLOW_COPY_AND_ASSIGN(ViewManagerSynchronizer);

mojo/services/public/cpp/view_manager/tests/view_manager_unittest.cc

@@ -6,6 +6,8 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "mojo/services/public/cpp/view_manager/lib/view_manager_private.h"
+#include "mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h"
 #include "mojo/services/public/cpp/view_manager/lib/view_tree_node_private.h"
 #include "mojo/services/public/cpp/view_manager/util.h"
 #include "mojo/services/public/cpp/view_manager/view.h"
@@ -30,6 +32,159 @@ void QuitRunLoop() {
   current_run_loop->Quit();
 }
 
+void QuitRunLoopOnChangesAcked() {
+  QuitRunLoop();
+}
+
+void WaitForAllChangesToBeAcked(ViewManager* manager) {
+  ViewManagerPrivate(manager).synchronizer()->set_changes_acked_callback(
+      base::Bind(&QuitRunLoopOnChangesAcked));
+  DoRunLoop();
+  ViewManagerPrivate(manager).synchronizer()->ClearChangesAckedCallback();
+}
+
+class ActiveViewChangedObserver : public ViewTreeNodeObserver {
+ public:
+  explicit ActiveViewChangedObserver(ViewTreeNode* node)
+      : node_(node) {}
+  virtual ~ActiveViewChangedObserver() {}
+
+ private:
+  // Overridden from ViewTreeNodeObserver:
+  virtual void OnNodeActiveViewChange(ViewTreeNode* node,
+                                      View* old_view,
+                                      View* new_view,
+                                      DispositionChangePhase phase) OVERRIDE {
+    DCHECK_EQ(node, node_);
+    QuitRunLoop();
+  }
+
+  ViewTreeNode* node_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActiveViewChangedObserver);
+};
+
+// Waits until the active view id of the supplied node changes.
+void WaitForActiveViewToChange(ViewTreeNode* node) {
+  ActiveViewChangedObserver observer(node);
+  node->AddObserver(&observer);
+  DoRunLoop();
+  node->RemoveObserver(&observer);
+}
+
+// Spins a runloop until the tree beginning at |root| has |tree_size| nodes
+// (including |root|).
+class TreeSizeMatchesObserver : public ViewTreeNodeObserver {
+ public:
+  TreeSizeMatchesObserver(ViewTreeNode* tree, size_t tree_size)
+      : tree_(tree),
+        tree_size_(tree_size) {}
+  virtual ~TreeSizeMatchesObserver() {}
+
+  bool IsTreeCorrectSize() {
+    return CountNodes(tree_) == tree_size_;
+  }
+
+ private:
+  // Overridden from ViewTreeNodeObserver:
+  virtual void OnTreeChange(const TreeChangeParams& params) OVERRIDE {
+    if (params.phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
+      return;
+    if (IsTreeCorrectSize())
+      QuitRunLoop();
+  }
+
+  size_t CountNodes(const ViewTreeNode* node) const {
+    size_t count = 1;
+    ViewTreeNode::Children::const_iterator it = node->children().begin();
+    for (; it != node->children().end(); ++it)
+      count += CountNodes(*it);
+    return count;
+  }
+
+  ViewTreeNode* tree_;
+  size_t tree_size_;
+  DISALLOW_COPY_AND_ASSIGN(TreeSizeMatchesObserver);
+};
+
+void WaitForTreeSizeToMatch(ViewTreeNode* node, size_t tree_size) {
+  TreeSizeMatchesObserver observer(node, tree_size);
+  if (observer.IsTreeCorrectSize())
+    return;
+  node->AddObserver(&observer);
+  DoRunLoop();
+  node->RemoveObserver(&observer);
+}
+
+
+// Utility class that waits for the destruction of some number of nodes and
+// views.
+class DestructionObserver : public ViewTreeNodeObserver,
+                            public ViewObserver {
+ public:
+  // |nodes| or |views| can be NULL.
+  DestructionObserver(std::set<TransportNodeId>* nodes,
+                      std::set<TransportViewId>* views)
+      : nodes_(nodes),
+        views_(views) {}
+
+ private:
+  // Overridden from ViewTreeNodeObserver:
+  virtual void OnNodeDestroy(
+      ViewTreeNode* node,
+      ViewTreeNodeObserver::DispositionChangePhase phase) OVERRIDE {
+    if (phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
+      return;
+    std::set<TransportNodeId>::const_iterator it = nodes_->find(node->id());
+    if (it != nodes_->end())
+      nodes_->erase(it);
+    if (CanQuit())
+      QuitRunLoop();
+  }
+
+  // Overridden from ViewObserver:
+  virtual void OnViewDestroy(
+      View* view,
+      ViewObserver::DispositionChangePhase phase) OVERRIDE {
+    if (phase != ViewObserver::DISPOSITION_CHANGED)
+      return;
+    std::set<TransportViewId>::const_iterator it = views_->find(view->id());
+    if (it != views_->end())
+      views_->erase(it);
+    if (CanQuit())
+      QuitRunLoop();
+  }
+
+  bool CanQuit() {
+    return (!nodes_ || nodes_->empty()) && (!views_ || views_->empty());
+  }
+
+  std::set<TransportNodeId>* nodes_;
+  std::set<TransportViewId>* views_;
+
+  DISALLOW_COPY_AND_ASSIGN(DestructionObserver);
+};
+
+void WaitForDestruction(ViewManager* view_manager,
+                        std::set<TransportNodeId>* nodes,
+                        std::set<TransportViewId>* views) {
+  DestructionObserver observer(nodes, views);
+  DCHECK(nodes || views);
+  if (nodes) {
+    for (std::set<TransportNodeId>::const_iterator it = nodes->begin();
+          it != nodes->end(); ++it) {
+      view_manager->GetNodeById(*it)->AddObserver(&observer);
+    }
+  }
+  if (views) {
+    for (std::set<TransportViewId>::const_iterator it = views->begin();
+          it != views->end(); ++it) {
+      view_manager->GetViewById(*it)->AddObserver(&observer);
+    }
+  }
+  DoRunLoop();
+}
+
 // ViewManager -----------------------------------------------------------------
 
 // These tests model synchronization of two peer connections to the view manager
@@ -101,38 +256,6 @@ class TreeObserverBase : public ViewTreeNodeObserver {
   DISALLOW_COPY_AND_ASSIGN(TreeObserverBase);
 };
 
-// Spins a runloop until the tree beginning at |root| has |tree_size| nodes
-// (including |root|).
-class TreeSizeMatchesWaiter : public TreeObserverBase {
- public:
-  TreeSizeMatchesWaiter(ViewManager* view_manager, size_t tree_size)
-      : TreeObserverBase(view_manager),
-        tree_size_(tree_size) {
-    DoRunLoop();
-  }
-  virtual ~TreeSizeMatchesWaiter() {}
-
- private:
-  // Overridden from TreeObserverBase:
-  virtual bool ShouldQuitRunLoop(const TreeChangeParams& params) OVERRIDE {
-    if (params.phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
-      return false;
-    return CountNodes(view_manager()->tree()) == tree_size_;
-  }
-
-  size_t CountNodes(ViewTreeNode* node) const {
-    size_t count = 1;
-    ViewTreeNode::Children::const_iterator it = node->children().begin();
-    for (; it != node->children().end(); ++it)
-      count += CountNodes(*it);
-    return count;
-  }
-
-  size_t tree_size_;
-  DISALLOW_COPY_AND_ASSIGN(TreeSizeMatchesWaiter);
-};
-
-
 class HierarchyChanged_NodeCreatedObserver : public TreeObserverBase {
  public:
   explicit HierarchyChanged_NodeCreatedObserver(ViewManager* view_manager)
@@ -193,7 +316,7 @@ TEST_F(ViewManagerTest, HierarchyChanged_NodeMoved) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
   ViewTreeNode* node2 = CreateNodeInParent(view_manager_1()->tree());
   ViewTreeNode* node21 = CreateNodeInParent(node2);
-  TreeSizeMatchesWaiter waiter(view_manager_2(), 4);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 4);
 
   HierarchyChanged_NodeMovedObserver observer(view_manager_2(),
                                               node2->id(),
@@ -234,7 +357,7 @@ class HierarchyChanged_NodeRemovedObserver : public TreeObserverBase {
 
 TEST_F(ViewManagerTest, HierarchyChanged_NodeRemoved) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   HierarchyChanged_NodeRemovedObserver observer(view_manager_2());
 
@@ -246,73 +369,9 @@ TEST_F(ViewManagerTest, HierarchyChanged_NodeRemoved) {
   EXPECT_TRUE(tree2->children().empty());
 }
 
-// Utility class that waits for the destruction of some number of nodes and
-// views.
-class DestructionWaiter : public ViewTreeNodeObserver,
-                          public ViewObserver {
- public:
-  // |nodes| or |views| can be NULL.
-  DestructionWaiter(ViewManager* view_manager,
-                    std::set<TransportNodeId>* nodes,
-                    std::set<TransportViewId>* views)
-      : nodes_(nodes),
-        views_(views) {
-    DCHECK(nodes || views);
-    if (nodes) {
-      for (std::set<TransportNodeId>::const_iterator it = nodes->begin();
-           it != nodes->end(); ++it) {
-        view_manager->GetNodeById(*it)->AddObserver(this);
-      }
-    }
-    if (views) {
-      for (std::set<TransportViewId>::const_iterator it = views->begin();
-           it != views->end(); ++it) {
-        view_manager->GetViewById(*it)->AddObserver(this);
-      }
-    }
-    DoRunLoop();
-  }
-
- private:
-  // Overridden from ViewTreeNodeObserver:
-  virtual void OnNodeDestroy(
-      ViewTreeNode* node,
-      ViewTreeNodeObserver::DispositionChangePhase phase) OVERRIDE {
-    if (phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
-      return;
-    std::set<TransportNodeId>::const_iterator it = nodes_->find(node->id());
-    if (it != nodes_->end())
-      nodes_->erase(it);
-    if (CanQuit())
-      QuitRunLoop();
-  }
-
-  // Overridden from ViewObserver:
-  virtual void OnViewDestroy(
-      View* view,
-      ViewObserver::DispositionChangePhase phase) OVERRIDE {
-    if (phase != ViewObserver::DISPOSITION_CHANGED)
-      return;
-    std::set<TransportViewId>::const_iterator it = views_->find(view->id());
-    if (it != views_->end())
-      views_->erase(it);
-    if (CanQuit())
-      QuitRunLoop();
-  }
-
-  bool CanQuit() {
-    return (!nodes_ || nodes_->empty()) && (!views_ || views_->empty());
-  }
-
-  std::set<TransportNodeId>* nodes_;
-  std::set<TransportViewId>* views_;
-
-  DISALLOW_COPY_AND_ASSIGN(DestructionWaiter);
-};
-
 TEST_F(ViewManagerTest, NodeDestroyed) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   // |node1| will be deleted after calling Destroy() below.
   TransportNodeId id = node1->id();
@@ -320,7 +379,7 @@ TEST_F(ViewManagerTest, NodeDestroyed) {
 
   std::set<TransportNodeId> nodes;
   nodes.insert(id);
-  DestructionWaiter destroyed_waiter(view_manager_2(), &nodes, NULL);
+  WaitForDestruction(view_manager_2(), &nodes, NULL);
 
   EXPECT_TRUE(view_manager_2()->tree()->children().empty());
   EXPECT_EQ(NULL, view_manager_2()->GetNodeById(id));
@@ -328,74 +387,47 @@ TEST_F(ViewManagerTest, NodeDestroyed) {
 
 TEST_F(ViewManagerTest, ViewManagerDestroyed_CleanupNode) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   TransportNodeId id = node1->id();
   DestroyViewManager1();
   std::set<TransportNodeId> nodes;
   nodes.insert(id);
-  DestructionWaiter destroyed_waiter(view_manager_2(), &nodes, NULL);
+  WaitForDestruction(view_manager_2(), &nodes, NULL);
 
   // tree() should still be valid, since it's owned by neither connection.
   EXPECT_TRUE(view_manager_2()->tree()->children().empty());
 }
 
-// Waits until the active view id of the supplied node changes.
-class ActiveViewChangedWaiter : public ViewTreeNodeObserver {
- public:
-  explicit ActiveViewChangedWaiter(ViewTreeNode* node)
-      : node_(node) {
-    node_->AddObserver(this);
-    DoRunLoop();
-  }
-  virtual ~ActiveViewChangedWaiter() {
-    node_->RemoveObserver(this);
-  }
-
- private:
-  // Overridden from ViewTreeNodeObserver:
-  virtual void OnNodeActiveViewChange(ViewTreeNode* node,
-                                      View* old_view,
-                                      View* new_view,
-                                      DispositionChangePhase phase) OVERRIDE {
-    DCHECK_EQ(node, node_);
-    QuitRunLoop();
-  }
-
-  ViewTreeNode* node_;
-
-  DISALLOW_COPY_AND_ASSIGN(ActiveViewChangedWaiter);
-};
-
 TEST_F(ViewManagerTest, SetActiveView) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   View* view1 = View::Create(view_manager_1());
   node1->SetActiveView(view1);
 
   ViewTreeNode* node1_2 = view_manager_2()->tree()->GetChildById(node1->id());
-  ActiveViewChangedWaiter waiter(node1_2);
+  WaitForActiveViewToChange(node1_2);
 
   EXPECT_EQ(node1_2->active_view()->id(), view1->id());
 }
 
 TEST_F(ViewManagerTest, DestroyView) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   View* view1 = View::Create(view_manager_1());
   node1->SetActiveView(view1);
 
   ViewTreeNode* node1_2 = view_manager_2()->tree()->GetChildById(node1->id());
-  ActiveViewChangedWaiter active_view_waiter(node1_2);
+  WaitForActiveViewToChange(node1_2);
 
   TransportViewId view1_id = view1->id();
   view1->Destroy();
 
   std::set<TransportViewId> views;
   views.insert(view1_id);
-  DestructionWaiter destruction_waiter(view_manager_2(), NULL, &views);
+  WaitForDestruction(view_manager_2(), NULL, &views);
   EXPECT_EQ(NULL, node1_2->active_view());
   EXPECT_EQ(NULL, view_manager_2()->GetViewById(view1_id));
 }
@@ -404,15 +436,13 @@ TEST_F(ViewManagerTest, DestroyView) {
 // node and view disappearing from all connections that see them.
 TEST_F(ViewManagerTest, ViewManagerDestroyed_CleanupNodeAndView) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   View* view1 = View::Create(view_manager_1());
   node1->SetActiveView(view1);
 
   ViewTreeNode* node1_2 = view_manager_2()->tree()->GetChildById(node1->id());
-  {
-    ActiveViewChangedWaiter active_view_waiter(node1_2);
-  }
+  WaitForActiveViewToChange(node1_2);
 
   TransportNodeId node1_id = node1->id();
   TransportViewId view1_id = view1->id();
@@ -422,9 +452,7 @@ TEST_F(ViewManagerTest, ViewManagerDestroyed_CleanupNodeAndView) {
   observed_nodes.insert(node1_id);
   std::set<TransportViewId> observed_views;
   observed_views.insert(view1_id);
-  DestructionWaiter destruction_waiter(view_manager_2(),
-                                       &observed_nodes,
-                                       &observed_views);
+  WaitForDestruction(view_manager_2(), &observed_nodes, &observed_views);
 
   // tree() should still be valid, since it's owned by neither connection.
   EXPECT_TRUE(view_manager_2()->tree()->children().empty());
@@ -441,15 +469,12 @@ TEST_F(ViewManagerTest, ViewManagerDestroyed_CleanupNodeAndView) {
 TEST_F(ViewManagerTest,
        ViewManagerDestroyed_CleanupNodeAndViewFromDifferentConnections) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   View* view1_2 = View::Create(view_manager_2());
   ViewTreeNode* node1_2 = view_manager_2()->tree()->GetChildById(node1->id());
   node1_2->SetActiveView(view1_2);
-
-  {
-    ActiveViewChangedWaiter active_view_waiter(node1);
-  }
+  WaitForActiveViewToChange(node1);
 
   TransportNodeId node1_id = node1->id();
   TransportViewId view1_2_id = view1_2->id();
@@ -457,7 +482,7 @@ TEST_F(ViewManagerTest,
   DestroyViewManager1();
   std::set<TransportNodeId> nodes;
   nodes.insert(node1_id);
-  DestructionWaiter destruction_waiter(view_manager_2(), &nodes, NULL);
+  WaitForDestruction(view_manager_2(), &nodes, NULL);
 
   // tree() should still be valid, since it's owned by neither connection.
   EXPECT_TRUE(view_manager_2()->tree()->children().empty());
@@ -476,11 +501,32 @@ TEST_F(ViewManagerTest,
 //             Contains().
 TEST_F(ViewManagerTest, SetActiveViewAcrossConnection) {
   ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
 
   View* view1_2 = View::Create(view_manager_2());
   EXPECT_DEATH(node1->SetActiveView(view1_2), "");
 }
 
+// This test verifies that a node hierarchy constructed in one connection
+// becomes entirely visible to the second connection when the hierarchy is
+// attached.
+TEST_F(ViewManagerTest, MapSubtreeOnAttach) {
+  ViewTreeNode* node1 = ViewTreeNode::Create(view_manager_1());
+  ViewTreeNode* node11 = CreateNodeInParent(node1);
+  View* view11 = View::Create(view_manager_1());
+  node11->SetActiveView(view11);
+  WaitForAllChangesToBeAcked(view_manager_1());
+
+  // Now attach this node tree to the root & wait for it to show up in the
+  // second connection.
+  view_manager_1()->tree()->AddChild(node1);
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 3);
+
+  ViewTreeNode* node11_2 = view_manager_2()->GetNodeById(node11->id());
+  View* view11_2 = view_manager_2()->GetViewById(view11->id());
+  EXPECT_TRUE(node11_2 != NULL);
+  EXPECT_EQ(view11_2, node11_2->active_view());
+}
+
 }  // namespace view_manager
 }  // namespace mojo

mojo/services/view_manager/view_manager_connection.cc

@@ -119,12 +119,14 @@ void ViewManagerConnection::ProcessNodeViewReplaced(
     bool originated_change) {
   if (originated_change)
     return;
+  if (known_nodes_.count(NodeIdToTransportId(node->id())) > 0) {
     const TransportViewId new_view_id = new_view ?
         ViewIdToTransportId(new_view->id()) : 0;
     const TransportViewId old_view_id = old_view ?
         ViewIdToTransportId(old_view->id()) : 0;
     client()->OnNodeViewReplaced(NodeIdToTransportId(node->id()),
                                  new_view_id, old_view_id);
+  }
 }
 
 void ViewManagerConnection::ProcessNodeDeleted(
@@ -270,6 +272,7 @@ void ViewManagerConnection::CreateNode(
     return;
   }
   node_map_[node_id.node_id] = new Node(this, node_id);
+  known_nodes_.insert(transport_node_id);
   callback.Run(true);
 }
 

mojo/services/view_manager/view_manager_connection_unittest.cc

@@ -891,6 +891,8 @@ TEST_F(ViewManagerConnectionTest, SetView) {
   ASSERT_TRUE(CreateNode(view_manager_.get(), 1, 1));
   ASSERT_TRUE(CreateNode(view_manager_.get(), 1, 2));
   ASSERT_TRUE(CreateView(view_manager_.get(), 1, 11));
+  ASSERT_TRUE(AddNode(view_manager_.get(), 1, CreateNodeId(1, 1), 1));
+  ASSERT_TRUE(AddNode(view_manager_.get(), 1, CreateNodeId(1, 2), 2));
   EXPECT_TRUE(client_.GetAndClearChanges().empty());
 
   EstablishSecondConnection();
@@ -946,17 +948,17 @@ TEST_F(ViewManagerConnectionTest, DeleteNodeWithView) {
   EstablishSecondConnection();
   EXPECT_TRUE(client2_.GetAndClearChanges().empty());
 
-  // Delete node 1.
+  // Delete node 1. The second connection should not see this because the node
+  // was not known to it.
   {
     ASSERT_TRUE(DeleteNode(view_manager_.get(), CreateNodeId(client_.id(), 1)));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_TRUE(changes.empty());
 
-    client2_.DoRunLoopUntilChangesCount(2);
+    client2_.DoRunLoopUntilChangesCount(1);
     changes = client2_.GetAndClearChanges();
-    ASSERT_EQ(2u, changes.size());
-    EXPECT_EQ("ViewReplaced node=1,1 new_view=null old_view=1,11", changes[0]);
-    EXPECT_EQ("ServerChangeIdAdvanced 2", changes[1]);
+    ASSERT_EQ(1u, changes.size());
+    EXPECT_EQ("ServerChangeIdAdvanced 2", changes[0]);
   }
 
   // Parent 2 to the root.