Fixes bug where IViewManagerClient could be messaged unnecessarily

When deleting a node view manager could end up messaging client both
of the delete and to advance. If delete is sent there is no need to
send an advance too.

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

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

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

4 files changed, 73 insertions, 42 deletions

diff --git a/mojo/services/view_manager/root_node_manager.cc b/mojo/services/view_manager/root_node_manager.cc
index 9f5b2af..5fa9168 100644
--- a/mojo/services/view_manager/root_node_manager.cc
+++ b/mojo/services/view_manager/root_node_manager.cc
@@ -19,12 +19,14 @@ RootNodeManager::ScopedChange::ScopedChange(
     RootNodeManager::ChangeType change_type,
     bool is_delete_node)
     : root_(root),
-      change_type_(change_type) {
-  root_->PrepareForChange(connection, is_delete_node);
+      connection_id_(connection->id()),
+      change_type_(change_type),
+      is_delete_node_(is_delete_node) {
+  root_->PrepareForChange(this);
 }
 
 RootNodeManager::ScopedChange::~ScopedChange() {
-  root_->FinishChange(change_type_);
+  root_->FinishChange();
 }
 
 RootNodeManager::Context::Context() {
@@ -41,10 +43,9 @@ RootNodeManager::RootNodeManager(ServiceProvider* service_provider,
     : service_provider_(service_provider),
       next_connection_id_(1),
       next_server_change_id_(1),
-      change_source_(kRootConnection),
-      is_processing_delete_node_(false),
       root_view_manager_(service_provider, this, view_manager_delegate),
-      root_(this, RootNodeId()) {
+      root_(this, RootNodeId()),
+      current_change_(NULL) {
 }
 
 RootNodeManager::~RootNodeManager() {
@@ -100,6 +101,16 @@ View* RootNodeManager::GetView(const ViewId& id) {
   return i == connection_map_.end() ? NULL : i->second->GetView(id);
 }
 
+void RootNodeManager::OnConnectionMessagedClient(TransportConnectionId id) {
+  if (current_change_)
+    current_change_->MarkConnectionAsMessaged(id);
+}
+
+bool RootNodeManager::DidConnectionMessageClient(
+    TransportConnectionId id) const {
+  return current_change_ && current_change_->DidMessageConnection(id);
+}
+
 void RootNodeManager::ProcessNodeBoundsChanged(const Node* node,
                                                const gfx::Rect& old_bounds,
                                                const gfx::Rect& new_bounds) {
@@ -146,21 +157,18 @@ void RootNodeManager::ProcessViewDeleted(const ViewId& view) {
   }
 }
 
-void RootNodeManager::PrepareForChange(ViewManagerConnection* connection,
-                                       bool is_delete_node) {
+void RootNodeManager::PrepareForChange(ScopedChange* change) {
   // Should only ever have one change in flight.
-  DCHECK_EQ(kRootConnection, change_source_);
-  change_source_ = connection->id();
-  is_processing_delete_node_ = is_delete_node;
+  CHECK(!current_change_);
+  current_change_ = change;
 }
 
-void RootNodeManager::FinishChange(ChangeType change_type) {
+void RootNodeManager::FinishChange() {
   // PrepareForChange/FinishChange should be balanced.
-  DCHECK_NE(kRootConnection, change_source_);
-  change_source_ = 0;
-  is_processing_delete_node_ = false;
-  if (change_type == CHANGE_TYPE_ADVANCE_SERVER_CHANGE_ID)
+  CHECK(current_change_);
+  if (current_change_->change_type() == CHANGE_TYPE_ADVANCE_SERVER_CHANGE_ID)
     next_server_change_id_++;
+  current_change_ = NULL;
 }
 
 ViewManagerConnection* RootNodeManager::ConnectImpl(
diff --git a/mojo/services/view_manager/root_node_manager.h b/mojo/services/view_manager/root_node_manager.h
index 204f158..1598e0d 100644
--- a/mojo/services/view_manager/root_node_manager.h
+++ b/mojo/services/view_manager/root_node_manager.h
@@ -48,9 +48,28 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager : public NodeDelegate {
                  bool is_delete_node);
     ~ScopedChange();
 
+    TransportConnectionId connection_id() const { return connection_id_; }
+    ChangeType change_type() const { return change_type_; }
+    bool is_delete_node() const { return is_delete_node_; }
+
+    // Marks the connection with the specified id as having seen a message.
+    void MarkConnectionAsMessaged(TransportConnectionId connection_id) {
+      message_ids_.insert(connection_id);
+    }
+
+    // Returns true if MarkConnectionAsMessaged(connection_id) was invoked.
+    bool DidMessageConnection(TransportConnectionId connection_id) const {
+      return message_ids_.count(connection_id) > 0;
+    }
+
    private:
     RootNodeManager* root_;
+    const TransportConnectionId connection_id_;
     const ChangeType change_type_;
+    const bool is_delete_node_;
+
+    // See description of MarkConnectionAsMessaged/DidMessageConnection.
+    std::set<TransportConnectionId> message_ids_;
 
     DISALLOW_COPY_AND_ASSIGN(ScopedChange);
   };
@@ -88,9 +107,17 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager : public NodeDelegate {
 
   Node* root() { return &root_; }
 
-  bool IsProcessingChange() const { return change_source_ != 0; }
+  bool IsProcessingChange() const { return current_change_ != NULL; }
+
+  bool is_processing_delete_node() const {
+    return current_change_ && current_change_->is_delete_node(); }
 
-  bool is_processing_delete_node() const { return is_processing_delete_node_; }
+  // Invoked when a connection messages a client about the change. This is used
+  // to avoid sending ServerChangeIdAdvanced() unnecessarily.
+  void OnConnectionMessagedClient(TransportConnectionId id);
+
+  // Returns true if OnConnectionMessagedClient() was invoked for id.
+  bool DidConnectionMessageClient(TransportConnectionId id) const;
 
   // These functions trivially delegate to all ViewManagerConnections, which in
   // term notify their clients.
@@ -115,20 +142,20 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager : public NodeDelegate {
 
   typedef std::map<TransportConnectionId, ViewManagerConnection*> ConnectionMap;
 
-  // Invoked when a particular connection is about to make a change.
-  // Subsequently followed by FinishChange() once the change is done.
+  // Invoked when a connection is about to make a change.  Subsequently followed
+  // by FinishChange() once the change is done.
   //
   // Changes should never nest, meaning each PrepareForChange() must be
   // balanced with a call to FinishChange() with no PrepareForChange()
   // in between.
-  void PrepareForChange(ViewManagerConnection* connection, bool is_delete_node);
+  void PrepareForChange(ScopedChange* change);
 
   // Balances a call to PrepareForChange().
-  void FinishChange(ChangeType change_type);
+  void FinishChange();
 
   // Returns true if the specified connection originated the current change.
   bool IsChangeSource(TransportConnectionId connection_id) const {
-    return connection_id == change_source_;
+    return current_change_ && current_change_->connection_id() == connection_id;
   }
 
   // Implementation of the two connect variants.
@@ -155,12 +182,6 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager : public NodeDelegate {
   // Set of ViewManagerConnections.
   ConnectionMap connection_map_;
 
-  // If non-zero we're processing a change from this client.
-  TransportConnectionId change_source_;
-
-  // True if we're processing a DeleteNode request.
-  bool is_processing_delete_node_;
-
   RootViewManager root_view_manager_;
 
   // Root node.
@@ -170,6 +191,10 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager : public NodeDelegate {
   // explicitly destroyed.
   std::set<ViewManagerConnection*> connections_created_by_connect_;
 
+  // If non-null we're processing a change. The ScopedChange is not owned by us
+  // (it's created on the stack by ViewManagerConnection).
+  ScopedChange* current_change_;
+
   DISALLOW_COPY_AND_ASSIGN(RootNodeManager);
 };
 
diff --git a/mojo/services/view_manager/view_manager_connection.cc b/mojo/services/view_manager/view_manager_connection.cc
index d42c403..6b7a8c8 100644
--- a/mojo/services/view_manager/view_manager_connection.cc
+++ b/mojo/services/view_manager/view_manager_connection.cc
@@ -127,6 +127,7 @@ void ViewManagerConnection::ProcessNodeHierarchyChanged(
       RemoveFromKnown(node);
       client()->OnNodeDeleted(NodeIdToTransportId(node->id()),
                               server_change_id);
+      root_node_manager_->OnConnectionMessagedClient(id_);
       return;
     }
   }
@@ -185,9 +186,12 @@ void ViewManagerConnection::ProcessNodeDeleted(
 
   if (in_known) {
     client()->OnNodeDeleted(NodeIdToTransportId(node), server_change_id);
-  } else if (root_node_manager_->IsProcessingChange()) {
+    root_node_manager_->OnConnectionMessagedClient(id_);
+  } else if (root_node_manager_->IsProcessingChange() &&
+             !root_node_manager_->DidConnectionMessageClient(id_)) {
     client()->OnServerChangeIdAdvanced(
         root_node_manager_->next_server_change_id() + 1);
+    root_node_manager_->OnConnectionMessagedClient(id_);
   }
 }
 
diff --git a/mojo/services/view_manager/view_manager_connection_unittest.cc b/mojo/services/view_manager/view_manager_connection_unittest.cc
index e69520a..6224fe2 100644
--- a/mojo/services/view_manager/view_manager_connection_unittest.cc
+++ b/mojo/services/view_manager/view_manager_connection_unittest.cc
@@ -900,12 +900,10 @@ TEST_F(ViewManagerConnectionTest, DeleteNode) {
     ASSERT_TRUE(connection_->DeleteNode(BuildNodeId(1, 2)));
     EXPECT_TRUE(connection_->changes().empty());
 
-    // TODO(sky): fix this, client should not get ServerChangeIdAdvanced.
-    connection2_->DoRunLoopUntilChangesCount(2);
+    connection2_->DoRunLoopUntilChangesCount(1);
     const Changes changes(ChangesToDescription1(connection2_->changes()));
-    ASSERT_EQ(2u, changes.size());
+    ASSERT_EQ(1u, changes.size());
     EXPECT_EQ("NodeDeleted change_id=2 node=1,2", changes[0]);
-    EXPECT_EQ("ServerChangeIdAdvanced 3", changes[1]);
   }
 }
 
@@ -945,12 +943,10 @@ TEST_F(ViewManagerConnectionTest, ReuseDeletedNodeId) {
   {
     ASSERT_TRUE(connection_->DeleteNode(BuildNodeId(1, 2)));
 
-    // TODO(sky): fix this, shouldn't get ServerChangeIdAdvanced.
-    connection2_->DoRunLoopUntilChangesCount(2);
+    connection2_->DoRunLoopUntilChangesCount(1);
     const Changes changes(ChangesToDescription1(connection2_->changes()));
-    ASSERT_EQ(2u, changes.size());
+    ASSERT_EQ(1u, changes.size());
     EXPECT_EQ("NodeDeleted change_id=2 node=1,2", changes[0]);
-    EXPECT_EQ("ServerChangeIdAdvanced 3", changes[1]);
   }
 
   // Create 2 again, and add it back to 1. Should get the same notification.
@@ -1047,12 +1043,10 @@ TEST_F(ViewManagerConnectionTest, DeleteNodeWithView) {
   {
     ASSERT_TRUE(connection_->DeleteNode(BuildNodeId(1, 3)));
 
-    // TODO(sky): shouldn't get ServerChangeIdAdvanced here.
-    connection2_->DoRunLoopUntilChangesCount(2);
+    connection2_->DoRunLoopUntilChangesCount(1);
     const Changes changes(ChangesToDescription1(connection2_->changes()));
-    ASSERT_EQ(2u, changes.size());
+    ASSERT_EQ(1u, changes.size());
     EXPECT_EQ("NodeDeleted change_id=3 node=1,3", changes[0]);
-    EXPECT_EQ("ServerChangeIdAdvanced 4", changes[1]);
   }
 }