Cleans up usage of ViewManagerServiceImpl::roots_

. roots_ now always contains the root nodes. Previously I would add a
  fake id if a connection had roots_ and they were all removed. I'ved
  nuked that too.
. Fixed bug in ProcessNodeHierarchyChanged. Specifically if a
  connection added a new node that it didn't previously know about the
  set of known nodes needed to be updated. This is only matters for
  the window manager and may only come up in tests.
. GetUnknownNodes needed to check access policy.
. All children are now removed as part of an embed. Previously this
  wasn't done for the wm, but I can't see a good reason to keep
  that.
. Node::Destroy() would never complete if any of the children were
  from another connection.
. Client lib wasn't properly mapping in nodes from another
  connection.
. Updated a couple of tests appropriately.

BUG=397660
TEST=covered by tests
R=ben@chromium.org

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

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

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

@@ -216,8 +216,17 @@ void Node::Destroy() {
 
   if (manager_)
     static_cast<ViewManagerClientImpl*>(manager_)->DestroyNode(id_);
-  while (!children_.empty())
-    children_.front()->Destroy();
+  while (!children_.empty()) {
+    Node* child = children_.front();
+    if (!OwnsNode(manager_, child)) {
+      NodePrivate(child).ClearParent();
+      children_.erase(children_.begin());
+    } else {
+      child->Destroy();
+      DCHECK(std::find(children_.begin(), children_.end(), child) ==
+             children_.end());
+    }
+  }
   LocalDestroy();
 }
 

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

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
+#include "base/stl_util.h"
 #include "mojo/public/cpp/application/application_connection.h"
 #include "mojo/public/cpp/application/connect.h"
 #include "mojo/public/interfaces/service_provider/service_provider.mojom.h"
@@ -59,10 +60,13 @@ Node* AddNodeToViewManager(ViewManagerClientImpl* client,
 }
 
 Node* BuildNodeTree(ViewManagerClientImpl* client,
-                    const Array<NodeDataPtr>& nodes) {
+                    const Array<NodeDataPtr>& nodes,
+                    Node* initial_parent) {
   std::vector<Node*> parents;
   Node* root = NULL;
   Node* last_node = NULL;
+  if (initial_parent)
+    parents.push_back(initial_parent);
   for (size_t i = 0; i < nodes.size(); ++i) {
     if (last_node && nodes[i]->parent_id == last_node->id()) {
       parents.push_back(last_node);
@@ -137,12 +141,15 @@ ViewManagerClientImpl::ViewManagerClientImpl(ViewManagerDelegate* delegate)
 }
 
 ViewManagerClientImpl::~ViewManagerClientImpl() {
+  std::vector<Node*> non_owned;
   while (!nodes_.empty()) {
     IdToNodeMap::iterator it = nodes_.begin();
-    if (OwnsNode(it->second->id()))
+    if (OwnsNode(it->second->id())) {
       it->second->Destroy();
-    else
+    } else {
+      non_owned.push_back(it->second);
       nodes_.erase(it);
+    }
   }
   while (!views_.empty()) {
     IdToViewMap::iterator it = views_.begin();
@@ -151,6 +158,12 @@ ViewManagerClientImpl::~ViewManagerClientImpl() {
     else
       views_.erase(it);
   }
+  // Delete the non-owned nodes last. In the typical case these are roots. The
+  // exception is the window manager, which may know aboutother random nodes
+  // that it doesn't own.
+  // NOTE: we manually delete as we're a friend.
+  for (size_t i = 0; i < non_owned.size(); ++i)
+    delete non_owned[i];
   delegate_->OnViewManagerDisconnected(this);
 }
 
@@ -324,11 +337,11 @@ void ViewManagerClientImpl::OnViewManagerConnectionEstablished(
   connected_ = true;
   connection_id_ = connection_id;
   creator_url_ = TypeConverter<String, std::string>::ConvertFrom(creator_url);
-  AddRoot(BuildNodeTree(this, nodes));
+  AddRoot(BuildNodeTree(this, nodes, NULL));
 }
 
 void ViewManagerClientImpl::OnRootAdded(Array<NodeDataPtr> nodes) {
-  AddRoot(BuildNodeTree(this, nodes));
+  AddRoot(BuildNodeTree(this, nodes, NULL));
 }
 
 void ViewManagerClientImpl::OnNodeBoundsChanged(Id node_id,
@@ -344,7 +357,10 @@ void ViewManagerClientImpl::OnNodeHierarchyChanged(
     Id new_parent_id,
     Id old_parent_id,
     mojo::Array<NodeDataPtr> nodes) {
-  BuildNodeTree(this, nodes);
+  Node* initial_parent = nodes.size() ?
+      GetNodeById(nodes[0]->parent_id) : NULL;
+
+  BuildNodeTree(this, nodes, initial_parent);
 
   Node* new_parent = GetNodeById(new_parent_id);
   Node* old_parent = GetNodeById(old_parent_id);

mojo/services/public/cpp/view_manager/node.h

@@ -81,6 +81,7 @@ class Node {
 
  private:
   friend class NodePrivate;
+  friend class ViewManagerClientImpl;
 
   explicit Node(ViewManager* manager);
 

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

@@ -174,6 +174,15 @@ class TreeSizeMatchesObserver : public NodeObserver {
   DISALLOW_COPY_AND_ASSIGN(TreeSizeMatchesObserver);
 };
 
+void WaitForTreeSizeToMatch(Node* 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 NodeObserver, public ViewObserver {
@@ -626,33 +635,36 @@ TEST_F(ViewManagerTest, Reorder) {
   Node* node1 = Node::Create(window_manager());
   window_manager()->GetRoots().front()->AddChild(node1);
 
-  Node* node11 = Node::Create(window_manager());
-  node1->AddChild(node11);
-  Node* node12 = Node::Create(window_manager());
-  node1->AddChild(node12);
-
   ViewManager* embedded = Embed(window_manager(), node1);
 
-  Node* node1_in_embedded = embedded->GetNodeById(node1->id());
+  Node* node11 = Node::Create(embedded);
+  embedded->GetRoots().front()->AddChild(node11);
+  Node* node12 = Node::Create(embedded);
+  embedded->GetRoots().front()->AddChild(node12);
+
+  Node* node1_in_wm = window_manager()->GetNodeById(node1->id());
 
   {
+    WaitForTreeSizeToMatch(node1, 2u);
     node11->MoveToFront();
-    WaitForOrderChange(embedded, embedded->GetNodeById(node11->id()));
+    WaitForOrderChange(window_manager(),
+                       window_manager()->GetNodeById(node11->id()));
 
-    EXPECT_EQ(node1_in_embedded->children().front(),
-              embedded->GetNodeById(node12->id()));
-    EXPECT_EQ(node1_in_embedded->children().back(),
-              embedded->GetNodeById(node11->id()));
+    EXPECT_EQ(node1_in_wm->children().front(),
+              window_manager()->GetNodeById(node12->id()));
+    EXPECT_EQ(node1_in_wm->children().back(),
+              window_manager()->GetNodeById(node11->id()));
   }
 
   {
     node11->MoveToBack();
-    WaitForOrderChange(embedded, embedded->GetNodeById(node11->id()));
+    WaitForOrderChange(window_manager(),
+                       window_manager()->GetNodeById(node11->id()));
 
-    EXPECT_EQ(node1_in_embedded->children().front(),
-              embedded->GetNodeById(node11->id()));
-    EXPECT_EQ(node1_in_embedded->children().back(),
-              embedded->GetNodeById(node12->id()));
+    EXPECT_EQ(node1_in_wm->children().front(),
+              window_manager()->GetNodeById(node11->id()));
+    EXPECT_EQ(node1_in_wm->children().back(),
+              window_manager()->GetNodeById(node12->id()));
   }
 }
 

mojo/services/view_manager/ids.h

@@ -12,8 +12,9 @@
 namespace mojo {
 namespace service {
 
-// Connection id reserved for the root.
-const ConnectionSpecificId kRootConnection = 0;
+// Connection id is used to indicate no connection. That is, no
+// ViewManagerServiceImpl ever gets this id.
+const ConnectionSpecificId kInvalidConnectionId = 0;
 
 // TODO(sky): remove this, temporary while window manager API is in existing
 // api.
@@ -77,13 +78,13 @@ inline Id ViewIdToTransportId(const ViewId& id) {
 }
 
 inline NodeId RootNodeId() {
-  return NodeId(kRootConnection, 1);
+  return NodeId(kInvalidConnectionId, 1);
 }
 
 // Returns a NodeId that is reserved to indicate no node. That is, no node will
 // ever be created with this id.
 inline NodeId InvalidNodeId() {
-  return NodeId(kRootConnection, 0);
+  return NodeId(kInvalidConnectionId, 0);
 }
 
 }  // namespace service

mojo/services/view_manager/root_node_manager.cc

@@ -88,7 +88,7 @@ void RootNodeManager::RemoveConnection(ViewManagerServiceImpl* connection) {
 
 void RootNodeManager::EmbedRoot(const std::string& url) {
   if (connection_map_.empty()) {
-    EmbedImpl(kRootConnection, String::From(url), InvalidNodeId());
+    EmbedImpl(kInvalidConnectionId, String::From(url), RootNodeId());
     return;
   }
   ViewManagerServiceImpl* connection = GetConnection(kWindowManagerConnection);

mojo/services/view_manager/view_manager_service_impl.cc

@@ -31,14 +31,12 @@ ViewManagerServiceImpl::ViewManagerServiceImpl(
       creator_id_(creator_id),
       creator_url_(creator_url),
       delete_on_connection_error_(false) {
-  // TODO: http://crbug.com/397660 .
-  if (root_id != InvalidNodeId()) {
-    CHECK(GetNode(root_id));
-    roots_.insert(NodeIdToTransportId(root_id));
-    access_policy_.reset(new DefaultAccessPolicy(id_, this));
-  } else {
+  CHECK(GetNode(root_id));
+  roots_.insert(NodeIdToTransportId(root_id));
+  if (root_id == RootNodeId())
     access_policy_.reset(new WindowManagerAccessPolicy(id_, this));
-  }
+  else
+    access_policy_.reset(new DefaultAccessPolicy(id_, this));
 }
 
 ViewManagerServiceImpl::~ViewManagerServiceImpl() {
@@ -81,7 +79,7 @@ bool ViewManagerServiceImpl::HasRoot(const NodeId& id) const {
 void ViewManagerServiceImpl::OnViewManagerServiceImplDestroyed(
     ConnectionSpecificId id) {
   if (creator_id_ == id)
-    creator_id_ = kRootConnection;
+    creator_id_ = kInvalidConnectionId;
 }
 
 void ViewManagerServiceImpl::ProcessNodeBoundsChanged(
@@ -101,6 +99,11 @@ void ViewManagerServiceImpl::ProcessNodeHierarchyChanged(
     const Node* new_parent,
     const Node* old_parent,
     bool originated_change) {
+  if (originated_change && !IsNodeKnown(node) && new_parent &&
+      IsNodeKnown(new_parent)) {
+    std::vector<const Node*> unused;
+    GetUnknownNodesFrom(node, &unused);
+  }
   if (originated_change || root_node_manager_->is_processing_delete_node() ||
       root_node_manager_->DidConnectionMessageClient(id_)) {
     return;
@@ -158,11 +161,7 @@ void ViewManagerServiceImpl::ProcessNodeDeleted(const NodeId& node,
   node_map_.erase(node.node_id);
 
   const bool in_known = known_nodes_.erase(NodeIdToTransportId(node)) > 0;
-  const bool in_roots = roots_.erase(NodeIdToTransportId(node)) > 0;
-
-  // TODO(sky): cleanup!
-  if (in_roots && roots_.empty())
-    roots_.insert(NodeIdToTransportId(InvalidNodeId()));
+  roots_.erase(NodeIdToTransportId(node));
 
   if (originated_change)
     return;
@@ -266,10 +265,12 @@ bool ViewManagerServiceImpl::SetViewImpl(Node* node, View* view) {
 void ViewManagerServiceImpl::GetUnknownNodesFrom(
     const Node* node,
     std::vector<const Node*>* nodes) {
-  if (IsNodeKnown(node))
+  if (IsNodeKnown(node) || !access_policy_->CanGetNodeTree(node))
     return;
   nodes->push_back(node);
   known_nodes_.insert(NodeIdToTransportId(node->id()));
+  if (!access_policy_->CanDescendIntoNodeForNodeTree(node))
+    return;
   std::vector<const Node*> children(node->GetChildren());
   for (size_t i = 0 ; i < children.size(); ++i)
     GetUnknownNodesFrom(children[i], nodes);
@@ -314,8 +315,6 @@ void ViewManagerServiceImpl::RemoveRoot(const NodeId& node_id) {
   CHECK(roots_.count(transport_node_id) > 0);
 
   roots_.erase(transport_node_id);
-  if (roots_.empty())
-    roots_.insert(NodeIdToTransportId(InvalidNodeId()));
 
   // No need to do anything if we created the node.
   if (node_id.connection_id == id_)
@@ -334,10 +333,6 @@ void ViewManagerServiceImpl::RemoveRoot(const NodeId& node_id) {
 
 void ViewManagerServiceImpl::RemoveChildrenAsPartOfEmbed(
     const NodeId& node_id) {
-  // Let the root do what it wants.
-  if (roots_.empty())
-    return;
-
   Node* node = GetNode(node_id);
   CHECK(node);
   CHECK(node->id().connection_id == node_id.connection_id);
@@ -633,12 +628,8 @@ void ViewManagerServiceImpl::OnConnectionEstablished() {
   root_node_manager_->AddConnection(this);
 
   std::vector<const Node*> to_send;
-  if (roots_.empty()) {
-    GetUnknownNodesFrom(root_node_manager_->root(), &to_send);
-  } else {
-    for (NodeIdSet::const_iterator i = roots_.begin(); i != roots_.end(); ++i)
-      GetUnknownNodesFrom(GetNode(NodeIdFromTransportId(*i)), &to_send);
-  }
+  for (NodeIdSet::const_iterator i = roots_.begin(); i != roots_.end(); ++i)
+    GetUnknownNodesFrom(GetNode(NodeIdFromTransportId(*i)), &to_send);
 
   client()->OnViewManagerConnectionEstablished(
       id_,

mojo/services/view_manager/view_manager_service_impl.h

@@ -235,12 +235,12 @@ class MOJO_VIEW_MANAGER_EXPORT ViewManagerServiceImpl
   // The set of nodes that has been communicated to the client.
   NodeIdSet known_nodes_;
 
-  // This is the set of nodes the connection can parent nodes to (in addition to
-  // any nodes created by this connection). If empty the connection can
-  // manipulate any nodes (except for deleting other connections nodes/views).
-  // The connection can not delete or move these. If this is set to a non-empty
-  // value and all the nodes are deleted (by another connection), then an
-  // invalid node is added here to ensure this connection is still constrained.
+  // Set of root nodes from other connections. More specifically any time
+  // Embed() is invoked the id of the node is added to this set for the child
+  // connection. The connection Embed() was invoked on (the parent) doesn't
+  // directly track which connections are attached to which of its nodes. That
+  // information can be found by looking through the |roots_| of all
+  // connections.
   NodeIdSet roots_;
 
   // See description above setter.

mojo/services/view_manager/view_manager_unittest.cc

@@ -596,16 +596,18 @@ TEST_F(ViewManagerTest, NodesRemovedWhenEmbedding) {
   ASSERT_TRUE(connection_->AddNode(BuildNodeId(1, 1), BuildNodeId(1, 2)));
 
   ASSERT_NO_FATAL_FAILURE(EstablishSecondConnection(false));
+  EXPECT_EQ("[node=1,1 parent=null view=null]",
+            ChangeNodeDescription(connection2_->changes()));
 
-  // Because |connection_| is the root, the embed call doesn't remove.
+  // Embed() removed node 2.
   {
     std::vector<TestNode> nodes;
     connection_->GetNodeTree(BuildNodeId(1, 2), &nodes);
     ASSERT_EQ(1u, nodes.size());
-    EXPECT_EQ("node=1,2 parent=1,1 view=null", nodes[0].ToString());
+    EXPECT_EQ("node=1,2 parent=null view=null", nodes[0].ToString());
   }
 
-  // But |connection2_| should not see node 2.
+  // |connection2_| should not see node 2.
   {
     std::vector<TestNode> nodes;
     connection2_->GetNodeTree(BuildNodeId(1, 1), &nodes);
@@ -889,55 +891,60 @@ TEST_F(ViewManagerTest, NodeHierarchyChangedAddingKnownToUnknown) {
 }
 
 TEST_F(ViewManagerTest, ReorderNode) {
-  Id node1_id = BuildNodeId(1, 1);
-  Id node2_id = BuildNodeId(1, 2);
-  Id node3_id = BuildNodeId(1, 3);
+  ASSERT_NO_FATAL_FAILURE(EstablishSecondConnection(true));
+
+  Id node1_id = BuildNodeId(2, 1);
+  Id node2_id = BuildNodeId(2, 2);
+  Id node3_id = BuildNodeId(2, 3);
   Id node4_id = BuildNodeId(1, 4);  // Peer to 1,1
   Id node5_id = BuildNodeId(1, 5);  // Peer to 1,1
-  Id node6_id = BuildNodeId(1, 6);  // Child of 1,2.
-  Id node7_id = BuildNodeId(1, 7);  // Unparented.
-  Id node8_id = BuildNodeId(1, 8);  // Unparented.
-  ASSERT_TRUE(connection_->CreateNode(node1_id));
-  ASSERT_TRUE(connection_->CreateNode(node2_id));
-  ASSERT_TRUE(connection_->CreateNode(node3_id));
+  Id node6_id = BuildNodeId(2, 6);  // Child of 1,2.
+  Id node7_id = BuildNodeId(2, 7);  // Unparented.
+  Id node8_id = BuildNodeId(2, 8);  // Unparented.
+  ASSERT_TRUE(connection2_->CreateNode(node1_id));
+  ASSERT_TRUE(connection2_->CreateNode(node2_id));
+  ASSERT_TRUE(connection2_->CreateNode(node3_id));
   ASSERT_TRUE(connection_->CreateNode(node4_id));
   ASSERT_TRUE(connection_->CreateNode(node5_id));
-  ASSERT_TRUE(connection_->CreateNode(node6_id));
-  ASSERT_TRUE(connection_->CreateNode(node7_id));
-  ASSERT_TRUE(connection_->CreateNode(node8_id));
-  ASSERT_TRUE(connection_->AddNode(node1_id, node2_id));
-  ASSERT_TRUE(connection_->AddNode(node2_id, node6_id));
-  ASSERT_TRUE(connection_->AddNode(node1_id, node3_id));
+  ASSERT_TRUE(connection2_->CreateNode(node6_id));
+  ASSERT_TRUE(connection2_->CreateNode(node7_id));
+  ASSERT_TRUE(connection2_->CreateNode(node8_id));
+  ASSERT_TRUE(connection2_->AddNode(node1_id, node2_id));
+  ASSERT_TRUE(connection2_->AddNode(node2_id, node6_id));
+  ASSERT_TRUE(connection2_->AddNode(node1_id, node3_id));
   ASSERT_TRUE(connection_->AddNode(
       NodeIdToTransportId(RootNodeId()), node4_id));
   ASSERT_TRUE(connection_->AddNode(
       NodeIdToTransportId(RootNodeId()), node5_id));
 
-  ASSERT_NO_FATAL_FAILURE(EstablishSecondConnection(false));
+  ASSERT_TRUE(connection_->AddNode(
+      NodeIdToTransportId(RootNodeId()), node1_id));
 
   {
-    connection_->ReorderNode(node2_id, node3_id, ORDER_DIRECTION_ABOVE);
+    ASSERT_TRUE(
+        connection2_->ReorderNode(node2_id, node3_id, ORDER_DIRECTION_ABOVE));
 
-    connection2_->DoRunLoopUntilChangesCount(1);
-    const Changes changes(ChangesToDescription1(connection2_->changes()));
+    connection_->DoRunLoopUntilChangesCount(1);
+    const Changes changes(ChangesToDescription1(connection_->changes()));
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("Reordered node=1,2 relative=1,3 direction=above",
+    EXPECT_EQ("Reordered node=2,2 relative=2,3 direction=above",
               changes[0]);
   }
 
   {
-    connection_->ReorderNode(node2_id, node3_id, ORDER_DIRECTION_BELOW);
+    ASSERT_TRUE(connection2_->ReorderNode(
+                    node2_id, node3_id, ORDER_DIRECTION_BELOW));
 
-    connection2_->DoRunLoopUntilChangesCount(1);
-    const Changes changes(ChangesToDescription1(connection2_->changes()));
+    connection_->DoRunLoopUntilChangesCount(1);
+    const Changes changes(ChangesToDescription1(connection_->changes()));
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("Reordered node=1,2 relative=1,3 direction=below",
+    EXPECT_EQ("Reordered node=2,2 relative=2,3 direction=below",
               changes[0]);
   }
 
   // node2 is already below node3.
   EXPECT_FALSE(
-      connection_->ReorderNode(node2_id, node3_id, ORDER_DIRECTION_BELOW));
+      connection2_->ReorderNode(node2_id, node3_id, ORDER_DIRECTION_BELOW));
 
   // node4 & 5 are unknown to connection2_.
   EXPECT_FALSE(connection2_->ReorderNode(
@@ -1040,38 +1047,41 @@ TEST_F(ViewManagerTest, ReuseDeletedNodeId) {
 
 // Assertions around setting a view.
 TEST_F(ViewManagerTest, SetView) {
+  ASSERT_NO_FATAL_FAILURE(EstablishSecondConnection(true));
+
   // Create nodes 1, 2 and 3 and the view 11. Nodes 2 and 3 are parented to 1.
-  ASSERT_TRUE(connection_->CreateNode(BuildNodeId(1, 1)));
-  ASSERT_TRUE(connection_->CreateNode(BuildNodeId(1, 2)));
-  ASSERT_TRUE(connection_->CreateNode(BuildNodeId(1, 3)));
-  ASSERT_TRUE(connection_->CreateView(BuildViewId(1, 11)));
-  ASSERT_TRUE(connection_->AddNode(BuildNodeId(1, 1), BuildNodeId(1, 2)));
-  ASSERT_TRUE(connection_->AddNode(BuildNodeId(1, 1), BuildNodeId(1, 3)));
+  ASSERT_TRUE(connection2_->CreateNode(BuildNodeId(2, 2)));
+  ASSERT_TRUE(connection2_->CreateNode(BuildNodeId(2, 3)));
+  ASSERT_TRUE(connection2_->CreateView(BuildViewId(2, 11)));
+  ASSERT_TRUE(connection2_->AddNode(BuildNodeId(1, 1), BuildNodeId(2, 2)));
+  ASSERT_TRUE(connection2_->AddNode(BuildNodeId(1, 1), BuildNodeId(2, 3)));
 
-  ASSERT_NO_FATAL_FAILURE(EstablishSecondConnection(false));
+  // Do this to clear out the changes conncection_ has seen and ensure it's up
+  // to date.
+  connection_->CopyChangesFromTracker();
+  ASSERT_TRUE(connection_->CreateNode(BuildNodeId(1, 100)));
 
   // Set view 11 on node 1.
   {
-    ASSERT_TRUE(connection_->SetView(BuildNodeId(1, 1),
-                                     BuildViewId(1, 11)));
+    ASSERT_TRUE(connection2_->SetView(BuildNodeId(1, 1), BuildViewId(2, 11)));
 
-    connection2_->DoRunLoopUntilChangesCount(1);
-    const Changes changes(ChangesToDescription1(connection2_->changes()));
+    connection_->DoRunLoopUntilChangesCount(1);
+    const Changes changes(ChangesToDescription1(connection_->changes()));
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("ViewReplaced node=1,1 new_view=1,11 old_view=null",
+    EXPECT_EQ("ViewReplaced node=1,1 new_view=2,11 old_view=null",
               changes[0]);
   }
 
   // Set view 11 on node 2.
   {
-    ASSERT_TRUE(connection_->SetView(BuildNodeId(1, 2), BuildViewId(1, 11)));
+    ASSERT_TRUE(connection2_->SetView(BuildNodeId(2, 2), BuildViewId(2, 11)));
 
-    connection2_->DoRunLoopUntilChangesCount(2);
-    const Changes changes(ChangesToDescription1(connection2_->changes()));
+    connection_->DoRunLoopUntilChangesCount(2);
+    const Changes changes(ChangesToDescription1(connection_->changes()));
     ASSERT_EQ(2u, changes.size());
-    EXPECT_EQ("ViewReplaced node=1,1 new_view=null old_view=1,11",
+    EXPECT_EQ("ViewReplaced node=1,1 new_view=null old_view=2,11",
               changes[0]);
-    EXPECT_EQ("ViewReplaced node=1,2 new_view=1,11 old_view=null",
+    EXPECT_EQ("ViewReplaced node=2,2 new_view=2,11 old_view=null",
               changes[1]);
   }
 }