Trigger Node destruction notifications from Node's dtor

R=sky@chromium.org
BUG=none

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

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

7 files changed, 46 insertions, 82 deletions

diff --git a/mojo/services/view_manager/node.cc b/mojo/services/view_manager/node.cc
index 6351ac4..37da7b8 100644
--- a/mojo/services/view_manager/node.cc
+++ b/mojo/services/view_manager/node.cc
@@ -37,11 +37,17 @@ Node::Node(NodeDelegate* delegate, const NodeId& id)
 }
 
 Node::~Node() {
-  SetView(NULL);
   // This is implicitly done during deletion of the window, but we do it here so
   // that we're in a known state.
   if (window_.parent())
     window_.parent()->RemoveChild(&window_);
+
+  // This must be done *after* updating the hierarchy since the hierarchy change
+  // will remove the node from the connections that know about it, preventing
+  // this notification from being sent after the destruction notification.
+  SetView(NULL);
+
+  delegate_->OnNodeDestroyed(this);
 }
 
 // static
@@ -133,7 +139,12 @@ void Node::OnWindowHierarchyChanged(
       params.new_parent->GetProperty(kNodeKey) : NULL;
   const Node* old_parent = params.old_parent ?
       params.old_parent->GetProperty(kNodeKey) : NULL;
-  delegate_->OnNodeHierarchyChanged(this, new_parent, old_parent);
+  // This check is needed because even the root Node's aura::Window has a
+  // parent, but the Node itself has no parent (so it's possible for us to
+  // receive this notification from aura when no logical Node hierarchy change
+  // has actually ocurred).
+  if (new_parent != old_parent)
+    delegate_->OnNodeHierarchyChanged(this, new_parent, old_parent);
 }
 
 gfx::Size Node::GetMinimumSize() const {
diff --git a/mojo/services/view_manager/node_delegate.h b/mojo/services/view_manager/node_delegate.h
index ca4254e..1ad2ee7 100644
--- a/mojo/services/view_manager/node_delegate.h
+++ b/mojo/services/view_manager/node_delegate.h
@@ -24,6 +24,10 @@ class View;
 
 class MOJO_VIEW_MANAGER_EXPORT NodeDelegate {
  public:
+  // Invoked at the end of the Node's destructor (after it has been removed from
+  // the hierarchy and its active view has been reset).
+  virtual void OnNodeDestroyed(const Node* node) = 0;
+
   // Invoked when the hierarchy has changed.
   virtual void OnNodeHierarchyChanged(const Node* node,
                                       const Node* new_parent,
diff --git a/mojo/services/view_manager/root_node_manager.cc b/mojo/services/view_manager/root_node_manager.cc
index 5590e49..612e3ed 100644
--- a/mojo/services/view_manager/root_node_manager.cc
+++ b/mojo/services/view_manager/root_node_manager.cc
@@ -48,18 +48,19 @@ RootNodeManager::RootNodeManager(ApplicationConnection* app_connection,
       next_connection_id_(1),
       next_server_change_id_(1),
       root_view_manager_(app_connection, this, view_manager_delegate),
-      root_(this, RootNodeId()),
+      root_(new Node(this, RootNodeId())),
       current_change_(NULL) {
 }
 
 RootNodeManager::~RootNodeManager() {
   aura::client::FocusClient* focus_client =
-      aura::client::GetFocusClient(root_.window());
+      aura::client::GetFocusClient(root_->window());
   focus_client->RemoveObserver(this);
   while (!connections_created_by_connect_.empty())
     delete *(connections_created_by_connect_.begin());
   // All the connections should have been destroyed.
   DCHECK(connection_map_.empty());
+  root_.reset();
 }
 
 ConnectionSpecificId RootNodeManager::GetAndAdvanceNextConnectionId() {
@@ -104,8 +105,8 @@ ViewManagerServiceImpl* RootNodeManager::GetConnection(
 }
 
 Node* RootNodeManager::GetNode(const NodeId& id) {
-  if (id == root_.id())
-    return &root_;
+  if (id == root_->id())
+    return root_.get();
   ConnectionMap::iterator i = connection_map_.find(id.connection_id);
   return i == connection_map_.end() ? NULL : i->second->GetNode(id);
 }
@@ -195,7 +196,7 @@ void RootNodeManager::ProcessNodeDeleted(const NodeId& node) {
   for (ConnectionMap::iterator i = connection_map_.begin();
        i != connection_map_.end(); ++i) {
     i->second->ProcessNodeDeleted(node, next_server_change_id_,
-                                 IsChangeSource(i->first));
+                                  IsChangeSource(i->first));
   }
 }
 
@@ -250,15 +251,19 @@ ViewManagerServiceImpl* RootNodeManager::EmbedImpl(
 
   ViewManagerServiceImpl* connection =
       new ViewManagerServiceImpl(this,
-                                creator_id,
-                                creator_url,
-                                url.To<std::string>());
+                                 creator_id,
+                                 creator_url,
+                                 url.To<std::string>());
   connection->SetRoots(node_ids);
   BindToPipe(connection, pipe.handle0.Pass());
   connections_created_by_connect_.insert(connection);
   return connection;
 }
 
+void RootNodeManager::OnNodeDestroyed(const Node* node) {
+  ProcessNodeDeleted(node->id());
+}
+
 void RootNodeManager::OnNodeHierarchyChanged(const Node* node,
                                              const Node* new_parent,
                                              const Node* old_parent) {
diff --git a/mojo/services/view_manager/root_node_manager.h b/mojo/services/view_manager/root_node_manager.h
index c8e8c5f..12a862b 100644
--- a/mojo/services/view_manager/root_node_manager.h
+++ b/mojo/services/view_manager/root_node_manager.h
@@ -114,7 +114,7 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager
   // Returns the View identified by |id|.
   View* GetView(const ViewId& id);
 
-  Node* root() { return &root_; }
+  Node* root() { return root_.get(); }
 
   bool IsProcessingChange() const { return current_change_ != NULL; }
 
@@ -137,6 +137,7 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager
 
   // These functions trivially delegate to all ViewManagerServiceImpls, which in
   // term notify their clients.
+  void ProcessNodeDestroyed(Node* node);
   void ProcessNodeBoundsChanged(const Node* node,
                                 const gfx::Rect& old_bounds,
                                 const gfx::Rect& new_bounds);
@@ -187,6 +188,7 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager
                                    const Array<Id>& node_ids);
 
   // Overridden from NodeDelegate:
+  virtual void OnNodeDestroyed(const Node* node) OVERRIDE;
   virtual void OnNodeHierarchyChanged(const Node* node,
                                       const Node* new_parent,
                                       const Node* old_parent) OVERRIDE;
@@ -214,7 +216,7 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager
   RootViewManager root_view_manager_;
 
   // Root node.
-  Node root_;
+  scoped_ptr<Node> root_;
 
   // Set of ViewManagerServiceImpls created by way of Connect(). These have to
   // be explicitly destroyed.
diff --git a/mojo/services/view_manager/view_manager_service_impl.cc b/mojo/services/view_manager/view_manager_service_impl.cc
index 5b8954a..dd3fc70 100644
--- a/mojo/services/view_manager/view_manager_service_impl.cc
+++ b/mojo/services/view_manager/view_manager_service_impl.cc
@@ -47,33 +47,19 @@ ViewManagerServiceImpl::ViewManagerServiceImpl(
 }
 
 ViewManagerServiceImpl::~ViewManagerServiceImpl() {
-  // Delete any views we own.
+  // Delete any views we created.
   while (!view_map_.empty()) {
     bool result = DeleteViewImpl(this, view_map_.begin()->second->id());
     DCHECK(result);
   }
 
-  // We're about to destroy all our nodes. Detach any views from them.
-  for (NodeMap::iterator i = node_map_.begin(); i != node_map_.end(); ++i) {
-    if (i->second->view()) {
-      bool result = SetViewImpl(i->second, ViewId());
-      DCHECK(result);
-    }
-  }
-
+  // Ditto the nodes.
   if (!node_map_.empty()) {
     RootNodeManager::ScopedChange change(
         this, root_node_manager_,
         RootNodeManager::CHANGE_TYPE_ADVANCE_SERVER_CHANGE_ID, true);
-    while (!node_map_.empty()) {
-      scoped_ptr<Node> node(node_map_.begin()->second);
-      Node* parent = node->GetParent();
-      const NodeId node_id(node->id());
-      if (parent)
-        parent->Remove(node.get());
-      root_node_manager_->ProcessNodeDeleted(node_id);
-      node_map_.erase(NodeIdToTransportId(node_id));
-    }
+    while (!node_map_.empty())
+      delete node_map_.begin()->second;
   }
 
   root_node_manager_->RemoveConnection(this);
@@ -205,6 +191,8 @@ void ViewManagerServiceImpl::ProcessNodeViewReplaced(
 void ViewManagerServiceImpl::ProcessNodeDeleted(const NodeId& node,
                                                 Id server_change_id,
                                                 bool originated_change) {
+  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;
 
@@ -382,16 +370,7 @@ bool ViewManagerServiceImpl::DeleteNodeImpl(ViewManagerServiceImpl* source,
   RootNodeManager::ScopedChange change(
       source, root_node_manager_,
       RootNodeManager::CHANGE_TYPE_ADVANCE_SERVER_CHANGE_ID, true);
-  if (node->GetParent())
-    node->GetParent()->Remove(node);
-  std::vector<Node*> children(node->GetChildren());
-  for (size_t i = 0; i < children.size(); ++i)
-    node->Remove(children[i]);
-  DCHECK(node->GetChildren().empty());
-  node_map_.erase(node_id.node_id);
   delete node;
-  node = NULL;
-  root_node_manager_->ProcessNodeDeleted(node_id);
   return true;
 }
 
@@ -570,7 +549,7 @@ void ViewManagerServiceImpl::CreateNode(
     callback.Run(false);
     return;
   }
-  node_map_[node_id.node_id] = new Node(this, node_id);
+  node_map_[node_id.node_id] = new Node(root_node_manager_, node_id);
   known_nodes_.insert(transport_node_id);
   callback.Run(true);
 }
@@ -802,29 +781,6 @@ void ViewManagerServiceImpl::DispatchOnViewInputEvent(Id transport_view_id,
   }
 }
 
-void ViewManagerServiceImpl::OnNodeHierarchyChanged(const Node* node,
-                                                    const Node* new_parent,
-                                                    const Node* old_parent) {
-  root_node_manager_->ProcessNodeHierarchyChanged(node, new_parent, old_parent);
-}
-
-void ViewManagerServiceImpl::OnNodeBoundsChanged(const Node* node,
-                                                 const gfx::Rect& old_bounds,
-                                                 const gfx::Rect& new_bounds) {
-  root_node_manager_->ProcessNodeBoundsChanged(node, old_bounds, new_bounds);
-}
-
-void ViewManagerServiceImpl::OnNodeViewReplaced(const Node* node,
-                                                const View* new_view,
-                                                const View* old_view) {
-  root_node_manager_->ProcessNodeViewReplaced(node, new_view, old_view);
-}
-
-void ViewManagerServiceImpl::OnViewInputEvent(const View* view,
-                                              const ui::Event* event) {
-  root_node_manager_->DispatchViewInputEventToWindowManager(view, event);
-}
-
 void ViewManagerServiceImpl::OnConnectionEstablished() {
   root_node_manager_->AddConnection(this);
 
diff --git a/mojo/services/view_manager/view_manager_service_impl.h b/mojo/services/view_manager/view_manager_service_impl.h
index 7da14db..42dbb81 100644
--- a/mojo/services/view_manager/view_manager_service_impl.h
+++ b/mojo/services/view_manager/view_manager_service_impl.h
@@ -14,7 +14,6 @@
 #include "base/containers/hash_tables.h"
 #include "mojo/services/public/interfaces/view_manager/view_manager.mojom.h"
 #include "mojo/services/view_manager/ids.h"
-#include "mojo/services/view_manager/node_delegate.h"
 #include "mojo/services/view_manager/view_manager_export.h"
 
 namespace gfx {
@@ -38,8 +37,7 @@ class View;
 
 // Manages a connection from the client.
 class MOJO_VIEW_MANAGER_EXPORT ViewManagerServiceImpl
-    : public InterfaceImpl<ViewManagerService>,
-      public NodeDelegate {
+    : public InterfaceImpl<ViewManagerService> {
  public:
   ViewManagerServiceImpl(RootNodeManager* root_node_manager,
                          ConnectionSpecificId creator_id,
@@ -216,19 +214,6 @@ class MOJO_VIEW_MANAGER_EXPORT ViewManagerServiceImpl
   virtual void DispatchOnViewInputEvent(Id transport_view_id,
                                         EventPtr event) OVERRIDE;
 
-  // Overridden from NodeDelegate:
-  virtual void OnNodeHierarchyChanged(const Node* node,
-                                      const Node* new_parent,
-                                      const Node* old_parent) OVERRIDE;
-  virtual void OnNodeBoundsChanged(const Node* node,
-                                   const gfx::Rect& old_bounds,
-                                   const gfx::Rect& new_bounds) OVERRIDE;
-  virtual void OnNodeViewReplaced(const Node* node,
-                                  const View* new_view,
-                                  const View* old_view) OVERRIDE;
-  virtual void OnViewInputEvent(const View* view,
-                                const ui::Event* event) OVERRIDE;
-
   // InterfaceImp overrides:
   virtual void OnConnectionEstablished() MOJO_OVERRIDE;
 
@@ -247,8 +232,9 @@ class MOJO_VIEW_MANAGER_EXPORT ViewManagerServiceImpl
   // The URL of the app that embedded the app this connection was created for.
   const std::string creator_url_;
 
+  // The nodes and views created by this connection. This connection owns these
+  // objects.
   NodeMap node_map_;
-
   ViewMap view_map_;
 
   // The set of nodes that has been communicated to the client.
diff --git a/mojo/services/view_manager/view_manager_unittest.cc b/mojo/services/view_manager/view_manager_unittest.cc
index 4c1e93bb..b6aee00 100644
--- a/mojo/services/view_manager/view_manager_unittest.cc
+++ b/mojo/services/view_manager/view_manager_unittest.cc
@@ -415,8 +415,8 @@ void EmbedRootCallback(bool* result_cache,
   run_loop->Quit();
 }
 
-// Resposible for establishing the initial ViewManagerService connection. Blocks
-// until result is determined.
+// Responsible for establishing the initial ViewManagerService connection.
+// Blocks until result is determined.
 bool EmbedRoot(ViewManagerInitService* view_manager_init,
                const std::string& url) {
   bool result = false;