Introduces another change id to hierarchy mutations

mojo/services/view_manager/view_manager_connection_unittest.cc

Index: mojo/services/view_manager/view_manager_connection_unittest.cc
diff --git a/mojo/services/view_manager/view_manager_connection_unittest.cc b/mojo/services/view_manager/view_manager_connection_unittest.cc
index 6c71f39de2c3608410169bcc174f6610b3b90139..221e8040d8d90e6fc255f495ce27ce6637ddbc79 100644
--- a/mojo/services/view_manager/view_manager_connection_unittest.cc
+++ b/mojo/services/view_manager/view_manager_connection_unittest.cc
@@ -115,9 +115,10 @@ bool DeleteNode(IViewManager* view_manager,
 bool AddNode(IViewManager* view_manager,
              TransportNodeId parent,
              TransportNodeId child,
-             TransportChangeId change_id) {
+             TransportChangeId server_change_id,
+             TransportChangeId client_change_id) {
   bool result = false;
-  view_manager->AddNode(parent, child, change_id,
+  view_manager->AddNode(parent, child, server_change_id, client_change_id,
                         base::Bind(&BooleanCallback, &result));
   DoRunLoop();
   return result;
@@ -126,10 +127,12 @@ bool AddNode(IViewManager* view_manager,
 // Removes a node, blocking until done.
 bool RemoveNodeFromParent(IViewManager* view_manager,
                           TransportNodeId node_id,
-                          TransportChangeId change_id) {
+                          TransportChangeId server_change_id,
+                          TransportChangeId client_change_id) {
   bool result = false;
-  view_manager->RemoveNodeFromParent(node_id, change_id,
-                                     base::Bind(&BooleanCallback, &result));
+  view_manager->RemoveNodeFromParent(
+      node_id, server_change_id, client_change_id,
+      base::Bind(&BooleanCallback, &result));
   DoRunLoop();
   return result;
 }
@@ -170,10 +173,17 @@ typedef std::vector<std::string> Changes;
 
 class ViewManagerClientImpl : public IViewManagerClient {
  public:
-  ViewManagerClientImpl() : id_(0), quit_count_(0) {}
+  ViewManagerClientImpl()
+      : id_(0),
+        next_server_change_id_(0),
+        quit_count_(0) {}
 
   TransportConnectionId id() const { return id_; }
 
+  TransportChangeId next_server_change_id() const {
+    return next_server_change_id_;
+  }
+
   Changes GetAndClearChanges() {
     Changes changes;
     changes.swap(changes_);
@@ -195,23 +205,40 @@ class ViewManagerClientImpl : public IViewManagerClient {
  private:
   // IViewManagerClient overrides:
   virtual void OnConnectionEstablished(
-      TransportConnectionId connection_id) OVERRIDE {
+      TransportConnectionId connection_id,
+      TransportChangeId next_server_change_id) OVERRIDE {
     id_ = connection_id;
+    next_server_change_id_ = next_server_change_id;
     if (current_run_loop)
       current_run_loop->Quit();
   }
-  virtual void OnNodeHierarchyChanged(TransportNodeId node,
-                                      TransportNodeId new_parent,
-                                      TransportNodeId old_parent,
-                                      TransportChangeId change_id) OVERRIDE {
+  virtual void OnNodeHierarchyChanged(
+      TransportNodeId node,
+      TransportNodeId new_parent,
+      TransportNodeId old_parent,
+      TransportChangeId server_change_id,
+      TransportChangeId client_change_id) OVERRIDE {
     changes_.push_back(
         base::StringPrintf(
-            "change_id=%d node=%s new_parent=%s old_parent=%s",
-            static_cast<int>(change_id), NodeIdToString(node).c_str(),
+            "change_id=%d,%d node=%s new_parent=%s old_parent=%s",
+            static_cast<int>(server_change_id),
+            static_cast<int>(client_change_id),
+            NodeIdToString(node).c_str(),
             NodeIdToString(new_parent).c_str(),
             NodeIdToString(old_parent).c_str()));
     QuitIfNecessary();
   }
+  virtual void OnNodeDeleted(TransportNodeId node,
+                             TransportChangeId server_change_id,
+                             TransportChangeId client_change_id) OVERRIDE {
+    changes_.push_back(
+        base::StringPrintf(
+            "NodeDeleted change_id=%d,%d node=%s",
+            static_cast<int>(server_change_id),
+            static_cast<int>(client_change_id),
+            NodeIdToString(node).c_str()));
+    QuitIfNecessary();
+  }
   virtual void OnNodeViewReplaced(TransportNodeId node,
                                   TransportViewId new_view_id,
                                   TransportViewId old_view_id,
@@ -224,14 +251,6 @@ class ViewManagerClientImpl : public IViewManagerClient {
             NodeIdToString(old_view_id).c_str()));
     QuitIfNecessary();
   }
-  virtual void OnNodeDeleted(TransportNodeId node,
-                             TransportChangeId change_id) OVERRIDE {
-    changes_.push_back(
-        base::StringPrintf(
-            "change_id=%d node=%s deleted",
-            static_cast<int>(change_id), NodeIdToString(node).c_str()));
-    QuitIfNecessary();
-  }
 
   void QuitIfNecessary() {
     if (quit_count_ > 0 && --quit_count_ == 0)
@@ -239,6 +258,7 @@ class ViewManagerClientImpl : public IViewManagerClient {
   }
 
   TransportConnectionId id_;
+  TransportChangeId next_server_change_id_;
 
   // Used to determine when/if to quit the run loop.
   size_t quit_count_;
@@ -291,6 +311,9 @@ TEST_F(ViewManagerConnectionTest, ValidId) {
   // All these tests assume 1 for the client id. The only real assertion here is
   // the client id is not zero, but adding this as rest of code here assumes 1.
   EXPECT_EQ(1, client_.id());
+
+  // Change ids start at 1 as well.
+  EXPECT_EQ(static_cast<TransportChangeId>(1), client_.next_server_change_id());
 }
 
 // Verifies two clients/connections get different ids.
@@ -321,10 +344,11 @@ TEST_F(ViewManagerConnectionTest, AddRemoveNotify) {
     ASSERT_TRUE(AddNode(view_manager_.get(),
                         CreateNodeId(client_.id(), 1),
                         CreateNodeId(client_.id(), 2),
+                        1,
                         11));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=11 node=1,2 new_parent=1,1 old_parent=null",
+    EXPECT_EQ("change_id=1,11 node=1,2 new_parent=1,1 old_parent=null",
               changes[0]);
   }
 
@@ -333,12 +357,81 @@ TEST_F(ViewManagerConnectionTest, AddRemoveNotify) {
     AllocationScope scope;
     ASSERT_TRUE(RemoveNodeFromParent(view_manager_.get(),
                                      CreateNodeId(client_.id(), 2),
+                                     2,
                                      101));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=101 node=1,2 new_parent=null old_parent=1,1",
+    EXPECT_EQ("change_id=2,101 node=1,2 new_parent=null old_parent=1,1",
+              changes[0]);
+  }
+}
+
+// Verifies AddNode fails when node is already in position.
+TEST_F(ViewManagerConnectionTest, AddNodeWithNoChange) {
+  ASSERT_TRUE(CreateNode(view_manager_.get(), 1));
+  ASSERT_TRUE(CreateNode(view_manager_.get(), 2));
+
+  EXPECT_TRUE(client_.GetAndClearChanges().empty());
+
+  // Make 2 a child of 1.
+  {
+    AllocationScope scope;
+    ASSERT_TRUE(AddNode(view_manager_.get(),
+                        CreateNodeId(client_.id(), 1),
+                        CreateNodeId(client_.id(), 2),
+                        1,
+                        11));
+    Changes changes(client_.GetAndClearChanges());
+    ASSERT_EQ(1u, changes.size());
+    EXPECT_EQ("change_id=1,11 node=1,2 new_parent=1,1 old_parent=null",
+              changes[0]);
+  }
+
+  // Try again, this should fail.
+  {
+    AllocationScope scope;
+    EXPECT_FALSE(AddNode(view_manager_.get(),
+                         CreateNodeId(client_.id(), 1),
+                         CreateNodeId(client_.id(), 2),
+                         2,
+                         11));
+    Changes changes(client_.GetAndClearChanges());
+    EXPECT_TRUE(changes.empty());
+  }
+}
+
+// Verifies AddNode fails when node is already in position.
+TEST_F(ViewManagerConnectionTest, AddAncestorFails) {
+  ASSERT_TRUE(CreateNode(view_manager_.get(), 1));
+  ASSERT_TRUE(CreateNode(view_manager_.get(), 2));
+
+  EXPECT_TRUE(client_.GetAndClearChanges().empty());
+
+  // Make 2 a child of 1.
+  {
+    AllocationScope scope;
+    ASSERT_TRUE(AddNode(view_manager_.get(),
+                        CreateNodeId(client_.id(), 1),
+                        CreateNodeId(client_.id(), 2),
+                        1,
+                        11));
+    Changes changes(client_.GetAndClearChanges());
+    ASSERT_EQ(1u, changes.size());
+    EXPECT_EQ("change_id=1,11 node=1,2 new_parent=1,1 old_parent=null",
               changes[0]);
   }
+
+  // Try to make 1 a child of 2, this should fail since 1 is an ancestor of 2.
+  {
+    AllocationScope scope;
+    EXPECT_FALSE(AddNode(view_manager_.get(),
+                         CreateNodeId(client_.id(), 2),
+                         CreateNodeId(client_.id(), 1),
+                         2,
+                         21));
+    Changes changes(client_.GetAndClearChanges());
+    EXPECT_TRUE(changes.empty());
+  }
 }
 
 // Verifies hierarchy changes are sent to multiple clients.
@@ -358,10 +451,11 @@ TEST_F(ViewManagerConnectionTest, AddRemoveNotifyMultipleConnections) {
     ASSERT_TRUE(AddNode(view_manager_.get(),
                         CreateNodeId(client_.id(), 1),
                         CreateNodeId(client_.id(), 2),
+                        1,
                         11));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=11 node=1,2 new_parent=1,1 old_parent=null",
+    EXPECT_EQ("change_id=1,11 node=1,2 new_parent=1,1 old_parent=null",
               changes[0]);
   }
 
@@ -370,11 +464,30 @@ TEST_F(ViewManagerConnectionTest, AddRemoveNotifyMultipleConnections) {
     client2_.DoRunLoopUntilChangesCount(1);
     Changes changes(client2_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=0 node=1,2 new_parent=1,1 old_parent=null",
+    EXPECT_EQ("change_id=1,0 node=1,2 new_parent=1,1 old_parent=null",
               changes[0]);
   }
 }
 
+// Verifies adding with an invalid id fails.
+TEST_F(ViewManagerConnectionTest, AddWithInvalidServerId) {
+  // Create two nodes.
+  ASSERT_TRUE(CreateNode(view_manager_.get(), 1));
+  ASSERT_TRUE(CreateNode(view_manager_.get(), 2));
+
+  // Make 2 a child of 1. Supply an invalid change id, which should fail.
+  {
+    AllocationScope scope;
+    ASSERT_FALSE(AddNode(view_manager_.get(),
+                         CreateNodeId(client_.id(), 1),
+                         CreateNodeId(client_.id(), 2),
+                         0,
+                         11));
+    Changes changes(client_.GetAndClearChanges());
+    EXPECT_TRUE(changes.empty());
+  }
+}
+
 // Verifies adding to root sends right notifications.
 TEST_F(ViewManagerConnectionTest, AddToRoot) {
   ASSERT_TRUE(CreateNode(view_manager_.get(), 21));
@@ -387,10 +500,11 @@ TEST_F(ViewManagerConnectionTest, AddToRoot) {
     ASSERT_TRUE(AddNode(view_manager_.get(),
                         CreateNodeId(client_.id(), 21),
                         CreateNodeId(client_.id(), 3),
+                        1,
                         11));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=11 node=1,3 new_parent=1,21 old_parent=null",
+    EXPECT_EQ("change_id=1,11 node=1,3 new_parent=1,21 old_parent=null",
               changes[0]);
   }
 
@@ -400,10 +514,11 @@ TEST_F(ViewManagerConnectionTest, AddToRoot) {
     ASSERT_TRUE(AddNode(view_manager_.get(),
                         CreateNodeId(0, 1),
                         CreateNodeId(client_.id(), 21),
+                        2,
                         44));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=44 node=1,21 new_parent=0,1 old_parent=null",
+    EXPECT_EQ("change_id=2,44 node=1,21 new_parent=0,1 old_parent=null",
               changes[0]);
   }
 }
@@ -420,10 +535,11 @@ TEST_F(ViewManagerConnectionTest, DeleteNode) {
     ASSERT_TRUE(AddNode(view_manager_.get(),
                         CreateNodeId(client_.id(), 1),
                         CreateNodeId(client_.id(), 2),
+                        1,
                         11));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=11 node=1,2 new_parent=1,1 old_parent=null",
+    EXPECT_EQ("change_id=1,11 node=1,2 new_parent=1,1 old_parent=null",
               changes[0]);
   }
 
@@ -433,14 +549,16 @@ TEST_F(ViewManagerConnectionTest, DeleteNode) {
     ASSERT_TRUE(AddNode(view_manager_.get(),
                         CreateNodeId(0, 1),
                         CreateNodeId(client_.id(), 1),
+                        2,
                         101));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(1u, changes.size());
-    EXPECT_EQ("change_id=101 node=1,1 new_parent=0,1 old_parent=null",
+    EXPECT_EQ("change_id=2,101 node=1,1 new_parent=0,1 old_parent=null",
               changes[0]);
   }
 
-  // Delete 1.
+  // Delete 1. Deleting 1 sends out notification of a removal for both nodes (1
+  // and 2).
   {
     AllocationScope scope;
     ASSERT_TRUE(DeleteNode(view_manager_.get(),
@@ -448,11 +566,11 @@ TEST_F(ViewManagerConnectionTest, DeleteNode) {
                            121));
     Changes changes(client_.GetAndClearChanges());
     ASSERT_EQ(3u, changes.size());
-    EXPECT_EQ("change_id=121 node=1,1 new_parent=null old_parent=0,1",
+    EXPECT_EQ("change_id=3,121 node=1,1 new_parent=null old_parent=0,1",
               changes[0]);
-    EXPECT_EQ("change_id=121 node=1,2 new_parent=null old_parent=1,1",
+    EXPECT_EQ("change_id=3,121 node=1,2 new_parent=null old_parent=1,1",
               changes[1]);
-    EXPECT_EQ("change_id=121 node=1,1 deleted", changes[2]);
+    EXPECT_EQ("NodeDeleted change_id=3,121 node=1,1", changes[2]);
   }
 }
 
@@ -513,7 +631,7 @@ TEST_F(ViewManagerConnectionTest, DeleteNodeWithView) {
     ASSERT_EQ(2u, changes.size());
     EXPECT_EQ("change_id=121 node=1,1 new_view=null old_view=1,11",
               changes[0]);
-    EXPECT_EQ("change_id=121 node=1,1 deleted", changes[1]);
+    EXPECT_EQ("NodeDeleted change_id=1,121 node=1,1", changes[1]);
   }
 
   // Set view 11 on node 2.
@@ -580,10 +698,12 @@ TEST_F(ViewManagerConnectionTest, GetNodeTree) {
   ASSERT_TRUE(AddNode(view_manager_.get(),
                       CreateNodeId(0, 1),
                       CreateNodeId(client_.id(), 1),
+                      1,
                       101));
   ASSERT_TRUE(AddNode(view_manager_.get(),
                       CreateNodeId(client_.id(), 1),
                       CreateNodeId(client_.id(), 11),
+                      2,
                       102));
 
   // Create two nodes in second connection, 2 and 3, both children of the root.
@@ -592,10 +712,12 @@ TEST_F(ViewManagerConnectionTest, GetNodeTree) {
   ASSERT_TRUE(AddNode(view_manager2_.get(),
                       CreateNodeId(0, 1),
                       CreateNodeId(client2_.id(), 2),
+                      3,
                       99));
   ASSERT_TRUE(AddNode(view_manager2_.get(),
                       CreateNodeId(0, 1),
                       CreateNodeId(client2_.id(), 3),
+                      4,
                       99));
 
   // Attach view to node 11 in the first connection.