Revert 269414 "Changes to deletion/ownership of nodes in the cli..."

> Changes to deletion/ownership of nodes in the client lib.
> 
> Nodes are now owned by the view manager. Constructors/destructors moved to private/protected. The ViewManager now maintains a map of id->node.
> Adds an observer method for destruction. Clients will need to implement this to invalidate their pointer (perhaps I should invent a node smart ptr).
> Adds lib tests for node removal, destruction, and connection destruction (when a connection is destroyed, all nodes it created should be destroyed).
> Adds a client notification from the service to notify other clients of node destruction & some tests.
> 
> R=sky@chromium.org
> http://crbug.com/365012
> 
> Review URL: https://codereview.chromium.org/274733004

TBR=ben@chromium.org

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

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

20 files changed, 106 insertions, 327 deletions

diff --git a/mojo/examples/sample_view_manager_app/sample_view_manager_app.cc b/mojo/examples/sample_view_manager_app/sample_view_manager_app.cc
index 69d6a44..43ee7f2 100644
--- a/mojo/examples/sample_view_manager_app/sample_view_manager_app.cc
+++ b/mojo/examples/sample_view_manager_app/sample_view_manager_app.cc
@@ -32,12 +32,11 @@ class SampleApp : public Application {
   explicit SampleApp(MojoHandle shell_handle)
       : Application(shell_handle) {
     view_manager_.reset(new services::view_manager::ViewManager(shell()));
-    view_manager_->Init();
-    services::view_manager::ViewTreeNode* node1 =
-        services::view_manager::ViewTreeNode::Create(view_manager_.get());
-    services::view_manager::ViewTreeNode* node11 =
-        services::view_manager::ViewTreeNode::Create(view_manager_.get());
-    node1->AddChild(node11);
+    node_1_.reset(
+        new services::view_manager::ViewTreeNode(view_manager_.get()));
+    node_11_.reset(
+      new services::view_manager::ViewTreeNode(view_manager_.get()));
+    node_1_->AddChild(node_11_.get());
   }
 
   virtual ~SampleApp() {
@@ -46,6 +45,8 @@ class SampleApp : public Application {
  private:
   // SampleApp creates a ViewManager and a trivial node hierarchy.
   scoped_ptr<services::view_manager::ViewManager> view_manager_;
+  scoped_ptr<services::view_manager::ViewTreeNode> node_1_;
+  scoped_ptr<services::view_manager::ViewTreeNode> node_11_;
 
   DISALLOW_COPY_AND_ASSIGN(SampleApp);
 };
diff --git a/mojo/services/public/cpp/view_manager/lib/view_manager.cc b/mojo/services/public/cpp/view_manager/lib/view_manager.cc
index 9325a99..4603abc 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_manager.cc
+++ b/mojo/services/public/cpp/view_manager/lib/view_manager.cc
@@ -5,7 +5,6 @@
 #include "mojo/services/public/cpp/view_manager/view_manager.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"
 
 namespace mojo {
 namespace services {
@@ -14,25 +13,12 @@ namespace view_manager {
 ViewManager::ViewManager(Shell* shell)
     : shell_(shell) {}
 
-ViewManager::~ViewManager() {
-  while (!nodes_.empty()) {
-    IdToNodeMap::iterator it = nodes_.begin();
-    if (synchronizer_->OwnsNode(it->second->id()))
-      it->second->Destroy();
-    else
-      nodes_.erase(it);
-  }
-}
+ViewManager::~ViewManager() {}
 
 void ViewManager::Init() {
   synchronizer_.reset(new ViewManagerSynchronizer(this));
 }
 
-ViewTreeNode* ViewManager::GetNodeById(TransportNodeId id) {
-  IdToNodeMap::const_iterator it = nodes_.find(id);
-  return it != nodes_.end() ? it->second : NULL;
-}
-
 }  // namespace view_manager
 }  // namespace services
 }  // namespace mojo
diff --git a/mojo/services/public/cpp/view_manager/lib/view_manager_private.cc b/mojo/services/public/cpp/view_manager/lib/view_manager_private.cc
index bf08b39..bcd4542 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_manager_private.cc
+++ b/mojo/services/public/cpp/view_manager/lib/view_manager_private.cc
@@ -12,14 +12,8 @@ ViewManagerPrivate::ViewManagerPrivate(ViewManager* manager)
     : manager_(manager) {}
 ViewManagerPrivate::~ViewManagerPrivate() {}
 
-void ViewManagerPrivate::AddNode(TransportNodeId node_id, ViewTreeNode* node) {
-  manager_->nodes_[node_id] = node;
-}
-
-void ViewManagerPrivate::RemoveNode(TransportNodeId node_id) {
-  ViewManager::IdToNodeMap::iterator it = manager_->nodes_.find(node_id);
-  if (it != manager_->nodes_.end())
-    manager_->nodes_.erase(it);
+void ViewManagerPrivate::SetRoot(ViewTreeNode* root) {
+  manager_->tree_.reset(root);
 }
 
 }  // namespace view_manager
diff --git a/mojo/services/public/cpp/view_manager/lib/view_manager_private.h b/mojo/services/public/cpp/view_manager/lib/view_manager_private.h
index f12fc64..cad72ec 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_manager_private.h
+++ b/mojo/services/public/cpp/view_manager/lib/view_manager_private.h
@@ -25,10 +25,7 @@ class ViewManagerPrivate {
   }
   Shell* shell() { return manager_->shell_; }
 
-  void set_root(ViewTreeNode* root) { manager_->tree_ = root; }
-
-  void AddNode(TransportNodeId node_id, ViewTreeNode* node);
-  void RemoveNode(TransportNodeId node_id);
+  void SetRoot(ViewTreeNode* root);
 
   // Returns true if the ViewManager's synchronizer is connected to the service.
   bool connected() { return manager_->synchronizer_->connected(); }
diff --git a/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc b/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
index 793255c..1f82fd8 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
+++ b/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
@@ -185,7 +185,6 @@ ViewManagerSynchronizer::ViewManagerSynchronizer(ViewManager* view_manager)
       connection_id_(0),
       next_id_(1),
       next_change_id_(0),
-      sync_factory_(this),
       init_loop_(NULL) {
   InterfacePipe<services::view_manager::IViewManager, AnyInterface>
       view_manager_pipe;
@@ -205,7 +204,6 @@ ViewManagerSynchronizer::ViewManagerSynchronizer(ViewManager* view_manager)
 }
 
 ViewManagerSynchronizer::~ViewManagerSynchronizer() {
-  DoSync();
 }
 
 TransportNodeId ViewManagerSynchronizer::CreateViewTreeNode() {
@@ -245,10 +243,6 @@ void ViewManagerSynchronizer::RemoveChild(TransportNodeId child_id,
   ScheduleSync();
 }
 
-bool ViewManagerSynchronizer::OwnsNode(TransportNodeId id) const {
-  return HiWord(id) == connection_id_;
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // ViewManagerSynchronizer, IViewManagerClient implementation:
 
@@ -263,26 +257,24 @@ void ViewManagerSynchronizer::OnNodeHierarchyChanged(uint32_t node_id,
                                                      uint32_t old_parent_id,
                                                      uint32_t change_id) {
   if (change_id == 0) {
-    ViewTreeNode* new_parent = view_manager_->GetNodeById(new_parent_id);
-    ViewTreeNode* old_parent = view_manager_->GetNodeById(old_parent_id);
+    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);
+      node = view_manager_->tree()->GetChildById(node_id);
       DCHECK_EQ(node->parent(), old_parent);
     } else {
       // New node, originating from another connection.
-      node = ViewTreeNodePrivate::LocalCreate();
+      node = new ViewTreeNode;
       ViewTreeNodePrivate private_node(node);
       private_node.set_view_manager(view_manager_);
       private_node.set_id(node_id);
-      ViewManagerPrivate(view_manager_).AddNode(node->id(), node);
     }
-    if (new_parent)
-      ViewTreeNodePrivate(new_parent).LocalAddChild(node);
-    else
-      ViewTreeNodePrivate(old_parent).LocalRemoveChild(node);
+    ViewTreeNodePrivate(new_parent).LocalAddChild(node);
   }
 }
 
@@ -293,24 +285,13 @@ void ViewManagerSynchronizer::OnNodeViewReplaced(uint32_t node,
   // ..
 }
 
-void ViewManagerSynchronizer::OnNodeDeleted(uint32_t node_id,
-                                            uint32_t change_id) {
-  if (change_id == 0) {
-    ViewTreeNode* node = view_manager_->GetNodeById(node_id);
-    if (node)
-      ViewTreeNodePrivate(node).LocalDestroy();
-  }
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // ViewManagerSynchronizer, private:
 
 void ViewManagerSynchronizer::ScheduleSync() {
-  if (sync_factory_.HasWeakPtrs())
-    return;
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
-      base::Bind(&ViewManagerSynchronizer::DoSync, sync_factory_.GetWeakPtr()));
+      base::Bind(&ViewManagerSynchronizer::DoSync, base::Unretained(this)));
 }
 
 void ViewManagerSynchronizer::DoSync() {
@@ -355,7 +336,7 @@ void ViewManagerSynchronizer::OnRootTreeReceived(
     }
     // We don't use the ctor that takes a ViewManager here, since it will call
     // back to the service and attempt to create a new node.
-    ViewTreeNode* node = ViewTreeNodePrivate::LocalCreate();
+    ViewTreeNode* node = new ViewTreeNode;
     ViewTreeNodePrivate private_node(node);
     private_node.set_view_manager(view_manager_);
     private_node.set_id(nodes[i].node_id());
@@ -364,9 +345,8 @@ void ViewManagerSynchronizer::OnRootTreeReceived(
     if (!last_node)
       root = node;
     last_node = node;
-    ViewManagerPrivate(view_manager_).AddNode(node->id(), node);
   }
-  ViewManagerPrivate(view_manager_).set_root(root);
+  ViewManagerPrivate(view_manager_).SetRoot(root);
   if (init_loop_)
     init_loop_->Quit();
 }
diff --git a/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h b/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h
index 78ae300..f161516 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h
+++ b/mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h
@@ -7,7 +7,6 @@
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_vector.h"
-#include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/remote_ptr.h"
 #include "mojo/services/public/cpp/view_manager/view_manager_types.h"
 #include "mojo/services/public/interfaces/view_manager/view_manager.mojom.h"
@@ -42,8 +41,6 @@ class ViewManagerSynchronizer : public IViewManagerClient {
   void AddChild(TransportNodeId child_id, TransportNodeId parent_id);
   void RemoveChild(TransportNodeId child_id, TransportNodeId parent_id);
 
-  bool OwnsNode(TransportNodeId id) const;
-
  private:
   friend class ViewManagerTransaction;
   typedef ScopedVector<ViewManagerTransaction> Transactions;
@@ -58,7 +55,6 @@ class ViewManagerSynchronizer : public IViewManagerClient {
                                   uint32_t new_view_id,
                                   uint32_t old_view_id,
                                   uint32_t change_id) OVERRIDE;
-  virtual void OnNodeDeleted(uint32_t node_id, uint32_t change_id) OVERRIDE;
 
   // Called to schedule a sync of the client model with the service after a
   // return to the message loop.
@@ -88,8 +84,6 @@ class ViewManagerSynchronizer : public IViewManagerClient {
 
   Transactions pending_transactions_;
 
-  base::WeakPtrFactory<ViewManagerSynchronizer> sync_factory_;
-
   // Non-NULL while blocking on the connection to |service_| during
   // construction.
   base::RunLoop* init_loop_;
diff --git a/mojo/services/public/cpp/view_manager/lib/view_tree_node.cc b/mojo/services/public/cpp/view_manager/lib/view_tree_node.cc
index 74e88a9..33d90a9 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_tree_node.cc
+++ b/mojo/services/public/cpp/view_manager/lib/view_tree_node.cc
@@ -90,19 +90,36 @@ void RemoveChildImpl(ViewTreeNode* child, ViewTreeNode::Children* children) {
 ////////////////////////////////////////////////////////////////////////////////
 // ViewTreeNode, public:
 
-// static
-ViewTreeNode* ViewTreeNode::Create(ViewManager* view_manager) {
-  ViewTreeNode* node = new ViewTreeNode(view_manager);
-  ViewManagerPrivate(view_manager).AddNode(node->id(), node);
-  return node;
-}
+ViewTreeNode::ViewTreeNode()
+    : manager_(NULL),
+      id_(-1),
+      owned_by_parent_(true),
+      parent_(NULL) {}
+
+ViewTreeNode::ViewTreeNode(ViewManager* manager)
+    : manager_(manager),
+      id_(ViewManagerPrivate(manager).synchronizer()->CreateViewTreeNode()),
+      owned_by_parent_(true),
+      parent_(NULL) {}
+
+ViewTreeNode::~ViewTreeNode() {
+  while (!children_.empty()) {
+    ViewTreeNode* child = children_.front();
+    if (child->owned_by_parent_) {
+      delete child;
+      // Deleting the child also removes it from our child list.
+      DCHECK(std::find(children_.begin(), children_.end(), child) ==
+             children_.end());
+    } else {
+      RemoveChild(child);
+    }
+  }
+
+  if (parent_)
+    parent_->RemoveChild(this);
 
-void ViewTreeNode::Destroy() {
   if (manager_)
     ViewManagerPrivate(manager_).synchronizer()->DestroyViewTreeNode(id_);
-  while (!children_.empty())
-    children_.front()->Destroy();
-  LocalDestroy();
 }
 
 void ViewTreeNode::AddObserver(ViewTreeNodeObserver* observer) {
@@ -147,40 +164,8 @@ ViewTreeNode* ViewTreeNode::GetChildById(TransportNodeId id) {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// ViewTreeNode, protected:
-
-ViewTreeNode::ViewTreeNode()
-    : manager_(NULL),
-      id_(-1),
-      parent_(NULL) {}
-
-ViewTreeNode::~ViewTreeNode() {
-  FOR_EACH_OBSERVER(
-      ViewTreeNodeObserver,
-      observers_,
-      OnNodeDestroy(this, ViewTreeNodeObserver::DISPOSITION_CHANGING));
-  if (parent_)
-    parent_->LocalRemoveChild(this);
-  FOR_EACH_OBSERVER(
-      ViewTreeNodeObserver,
-      observers_,
-      OnNodeDestroy(this, ViewTreeNodeObserver::DISPOSITION_CHANGED));
-  if (manager_)
-    ViewManagerPrivate(manager_).RemoveNode(id_);
-}
-
-////////////////////////////////////////////////////////////////////////////////
 // ViewTreeNode, private:
 
-ViewTreeNode::ViewTreeNode(ViewManager* manager)
-    : manager_(manager),
-      id_(ViewManagerPrivate(manager).synchronizer()->CreateViewTreeNode()),
-      parent_(NULL) {}
-
-void ViewTreeNode::LocalDestroy() {
-  delete this;
-}
-
 void ViewTreeNode::LocalAddChild(ViewTreeNode* child) {
   ScopedTreeNotifier notifier(child, child->parent(), this);
   if (child->parent())
diff --git a/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.cc b/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.cc
index b38d8fc..a1c3a40 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.cc
+++ b/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.cc
@@ -15,11 +15,6 @@ ViewTreeNodePrivate::ViewTreeNodePrivate(ViewTreeNode* node)
 ViewTreeNodePrivate::~ViewTreeNodePrivate() {
 }
 
-// static
-ViewTreeNode* ViewTreeNodePrivate::LocalCreate() {
-  return new ViewTreeNode;
-}
-
 }  // namespace view_manager
 }  // namespace services
 }  // namespace mojo
diff --git a/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.h b/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.h
index 4837a97..366dbd8 100644
--- a/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.h
+++ b/mojo/services/public/cpp/view_manager/lib/view_tree_node_private.h
@@ -20,8 +20,6 @@ class ViewTreeNodePrivate {
   explicit ViewTreeNodePrivate(ViewTreeNode* node);
   ~ViewTreeNodePrivate();
 
-  static ViewTreeNode* LocalCreate();
-
   ObserverList<ViewTreeNodeObserver>* observers() { return &node_->observers_; }
 
   void ClearParent() { node_->parent_ = NULL; }
@@ -33,15 +31,9 @@ class ViewTreeNodePrivate {
     node_->manager_ = manager;
   }
 
-  void LocalDestroy() {
-    node_->LocalDestroy();
-  }
   void LocalAddChild(ViewTreeNode* child) {
     node_->LocalAddChild(child);
   }
-  void LocalRemoveChild(ViewTreeNode* child) {
-    node_->LocalRemoveChild(child);
-  }
 
  private:
   ViewTreeNode* node_;
diff --git a/mojo/services/public/cpp/view_manager/tests/view_manager_unittest.cc b/mojo/services/public/cpp/view_manager/tests/view_manager_unittest.cc
index 0f09984..94dfe1f 100644
--- a/mojo/services/public/cpp/view_manager/tests/view_manager_unittest.cc
+++ b/mojo/services/public/cpp/view_manager/tests/view_manager_unittest.cc
@@ -31,9 +31,6 @@ void QuitRunLoop() {
 
 // ViewManager -----------------------------------------------------------------
 
-// These tests model synchronization of two peer connections to the view manager
-// service, that are given access to some root node.
-
 class ViewManagerTest : public testing::Test {
  public:
   ViewManagerTest() : commit_count_(0) {}
@@ -42,15 +39,11 @@ class ViewManagerTest : public testing::Test {
   ViewManager* view_manager_1() { return view_manager_1_.get(); }
   ViewManager* view_manager_2() { return view_manager_2_.get(); }
 
-  ViewTreeNode* CreateNodeInParent(ViewTreeNode* parent) {
+  scoped_ptr<ViewTreeNode> CreateNodeInParent(ViewTreeNode* parent) {
     ViewManager* parent_manager = ViewTreeNodePrivate(parent).view_manager();
-    ViewTreeNode* node = ViewTreeNode::Create(parent_manager);
-    parent->AddChild(node);
-    return node;
-  }
-
-  void DestroyViewManager1() {
-    view_manager_1_.reset();
+    scoped_ptr<ViewTreeNode> node(new ViewTreeNode(parent_manager));
+    parent->AddChild(node.get());
+    return node.Pass();
   }
 
  private:
@@ -92,6 +85,8 @@ class TreeObserverBase : public ViewTreeNodeObserver {
  private:
   // Overridden from ViewTreeNodeObserver:
   virtual void OnTreeChange(const TreeChangeParams& params) OVERRIDE {
+    if (params.phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
+      return;
     if (ShouldQuitRunLoop(params))
       QuitRunLoop();
   }
@@ -114,8 +109,6 @@ class TreeSizeMatchesWaiter : public TreeObserverBase {
  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_;
   }
 
@@ -141,8 +134,6 @@ class HierarchyChanged_NodeCreatedObserver : public TreeObserverBase {
  private:
   // Overridden from TreeObserverBase:
   virtual bool ShouldQuitRunLoop(const TreeChangeParams& params) OVERRIDE {
-    if (params.phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
-      return false;
     return params.receiver == view_manager()->tree() &&
         !params.old_parent &&
         params.new_parent == view_manager()->tree();
@@ -153,8 +144,8 @@ class HierarchyChanged_NodeCreatedObserver : public TreeObserverBase {
 
 TEST_F(ViewManagerTest, HierarchyChanged_NodeCreated) {
   HierarchyChanged_NodeCreatedObserver observer(view_manager_2());
-  ViewTreeNode* node1 = ViewTreeNode::Create(view_manager_1());
-  view_manager_1()->tree()->AddChild(node1);
+  scoped_ptr<ViewTreeNode> node1(new ViewTreeNode(view_manager_1()));
+  view_manager_1()->tree()->AddChild(node1.get());
   DoRunLoop();
 
   EXPECT_EQ(view_manager_2()->tree()->children().front()->id(), node1->id());
@@ -175,8 +166,6 @@ class HierarchyChanged_NodeMovedObserver : public TreeObserverBase {
  private:
   // Overridden from TreeObserverBase:
   virtual bool ShouldQuitRunLoop(const TreeChangeParams& params) OVERRIDE {
-    if (params.phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
-      return false;
     return params.receiver == view_manager()->tree() &&
         params.old_parent->id() == old_parent_id_&&
         params.new_parent->id() == new_parent_id_;
@@ -189,16 +178,16 @@ class HierarchyChanged_NodeMovedObserver : public TreeObserverBase {
 };
 
 TEST_F(ViewManagerTest, HierarchyChanged_NodeMoved) {
-  ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  ViewTreeNode* node2 = CreateNodeInParent(view_manager_1()->tree());
-  ViewTreeNode* node21 = CreateNodeInParent(node2);
+  scoped_ptr<ViewTreeNode> node1(CreateNodeInParent(view_manager_1()->tree()));
+  scoped_ptr<ViewTreeNode> node2(CreateNodeInParent(view_manager_1()->tree()));
+  scoped_ptr<ViewTreeNode> node21(CreateNodeInParent(node2.get()));
   TreeSizeMatchesWaiter waiter(view_manager_2(), 4);
 
   HierarchyChanged_NodeMovedObserver observer(view_manager_2(),
                                               node2->id(),
                                               node1->id());
 
-  node1->AddChild(node21);
+  node1->AddChild(node21.get());
   DoRunLoop();
 
   ViewTreeNode* tree2 = view_manager_2()->tree();
@@ -212,89 +201,7 @@ TEST_F(ViewManagerTest, HierarchyChanged_NodeMoved) {
   EXPECT_EQ(tree2_node21->parent(), tree2_node1);
 }
 
-class HierarchyChanged_NodeRemovedObserver : public TreeObserverBase {
- public:
-  HierarchyChanged_NodeRemovedObserver(ViewManager* view_manager)
-      : TreeObserverBase(view_manager) {}
-  virtual ~HierarchyChanged_NodeRemovedObserver() {}
-
- private:
-  // Overridden from TreeObserverBase:
-  virtual bool ShouldQuitRunLoop(const TreeChangeParams& params) OVERRIDE {
-    if (params.phase != ViewTreeNodeObserver::DISPOSITION_CHANGING)
-      return false;
-    return params.receiver == view_manager()->tree() &&
-        params.old_parent->id() == params.receiver->id() &&
-        params.new_parent == 0;
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(HierarchyChanged_NodeRemovedObserver);
-};
-
-TEST_F(ViewManagerTest, HierarchyChanged_NodeRemoved) {
-  ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter waiter(view_manager_2(), 2);
-
-  HierarchyChanged_NodeRemovedObserver observer(view_manager_2());
-
-  view_manager_1()->tree()->RemoveChild(node1);
-  DoRunLoop();
-
-  ViewTreeNode* tree2 = view_manager_2()->tree();
-
-  EXPECT_TRUE(tree2->children().empty());
-}
-
-class NodeDestroyed_Waiter : public ViewTreeNodeObserver {
- public:
-  NodeDestroyed_Waiter(ViewManager* view_manager, TransportNodeId id)
-      : view_manager_(view_manager),
-        id_(id) {
-    view_manager_->GetNodeById(id)->AddObserver(this);
-    DoRunLoop();
-  }
-  virtual ~NodeDestroyed_Waiter() {
-  }
-
- private:
-  // Overridden from TreeObserverBase:
-  virtual void OnNodeDestroy(ViewTreeNode* node,
-                             DispositionChangePhase phase) OVERRIDE {
-    if (phase != DISPOSITION_CHANGED)
-      return;
-    if (node->id() == id_)
-      QuitRunLoop();
-  }
-
-  TransportNodeId id_;
-  ViewManager* view_manager_;
-
-  DISALLOW_COPY_AND_ASSIGN(NodeDestroyed_Waiter);
-};
-
-TEST_F(ViewManagerTest, NodeDestroyed) {
-  ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
-
-  // |node1| will be deleted after calling Destroy() below.
-  TransportNodeId id = node1->id();
-  node1->Destroy();
-  NodeDestroyed_Waiter destroyed_waiter(view_manager_2(), id);
-
-  EXPECT_TRUE(view_manager_2()->tree()->children().empty());
-}
-
-TEST_F(ViewManagerTest, ViewManagerDestroyed) {
-  ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
-  TreeSizeMatchesWaiter init_waiter(view_manager_2(), 2);
-
-  TransportNodeId id = node1->id();
-  DestroyViewManager1();
-  NodeDestroyed_Waiter destroyed_waiter(view_manager_2(), id);
-
-  // tree() should still be valid, since it's owned by neither connection.
-  EXPECT_TRUE(view_manager_2()->tree()->children().empty());
-}
+// TODO(beng): node destruction
 
 }  // namespace view_manager
 }  // namespace services
diff --git a/mojo/services/public/cpp/view_manager/tests/view_tree_node_unittest.cc b/mojo/services/public/cpp/view_manager/tests/view_tree_node_unittest.cc
index 9751040..3425044 100644
--- a/mojo/services/public/cpp/view_manager/tests/view_tree_node_unittest.cc
+++ b/mojo/services/public/cpp/view_manager/tests/view_tree_node_unittest.cc
@@ -17,64 +17,54 @@ namespace view_manager {
 
 typedef testing::Test ViewTreeNodeTest;
 
-// Subclass with public ctor/dtor.
-class TestViewTreeNode : public ViewTreeNode {
- public:
-  TestViewTreeNode() {}
-  ~TestViewTreeNode() {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestViewTreeNode);
-};
-
 TEST_F(ViewTreeNodeTest, AddChild) {
-  TestViewTreeNode v1;
-  TestViewTreeNode v11;
-  v1.AddChild(&v11);
+  ViewTreeNode v1;
+  ViewTreeNode* v11 = new ViewTreeNode;
+  v1.AddChild(v11);
   EXPECT_EQ(1U, v1.children().size());
 }
 
 TEST_F(ViewTreeNodeTest, RemoveChild) {
-  TestViewTreeNode v1;
-  TestViewTreeNode v11;
-  v1.AddChild(&v11);
+  ViewTreeNode v1;
+  ViewTreeNode* v11 = new ViewTreeNode;
+  v1.AddChild(v11);
   EXPECT_EQ(1U, v1.children().size());
-  v1.RemoveChild(&v11);
+  v1.RemoveChild(v11);
   EXPECT_EQ(0U, v1.children().size());
 }
 
 TEST_F(ViewTreeNodeTest, Reparent) {
-  TestViewTreeNode v1;
-  TestViewTreeNode v2;
-  TestViewTreeNode v11;
-  v1.AddChild(&v11);
+  ViewTreeNode v1;
+  ViewTreeNode v2;
+  ViewTreeNode* v11 = new ViewTreeNode;
+  v1.AddChild(v11);
   EXPECT_EQ(1U, v1.children().size());
-  v2.AddChild(&v11);
+  v2.AddChild(v11);
   EXPECT_EQ(1U, v2.children().size());
   EXPECT_EQ(0U, v1.children().size());
 }
 
 TEST_F(ViewTreeNodeTest, Contains) {
-  TestViewTreeNode v1;
+  ViewTreeNode v1;
 
   // Direct descendant.
-  TestViewTreeNode v11;
-  v1.AddChild(&v11);
-  EXPECT_TRUE(v1.Contains(&v11));
+  ViewTreeNode* v11 = new ViewTreeNode;
+  v1.AddChild(v11);
+  EXPECT_TRUE(v1.Contains(v11));
 
   // Indirect descendant.
-  TestViewTreeNode v111;
-  v11.AddChild(&v111);
-  EXPECT_TRUE(v1.Contains(&v111));
+  ViewTreeNode* v111 = new ViewTreeNode;
+  v11->AddChild(v111);
+  EXPECT_TRUE(v1.Contains(v111));
 }
 
 TEST_F(ViewTreeNodeTest, GetChildById) {
-  TestViewTreeNode v1;
+  ViewTreeNode v1;
   ViewTreeNodePrivate(&v1).set_id(1);
-  TestViewTreeNode v11;
+  ViewTreeNode v11;
   ViewTreeNodePrivate(&v11).set_id(11);
   v1.AddChild(&v11);
-  TestViewTreeNode v111;
+  ViewTreeNode v111;
   ViewTreeNodePrivate(&v111).set_id(111);
   v11.AddChild(&v111);
 
@@ -125,11 +115,12 @@ class TreeChangeObserver : public ViewTreeNodeObserver {
 
 // Adds/Removes v11 to v1.
 TEST_F(ViewTreeNodeObserverTest, TreeChange_SimpleAddRemove) {
-  TestViewTreeNode v1;
+  ViewTreeNode v1;
   TreeChangeObserver o1(&v1);
   EXPECT_TRUE(o1.received_params().empty());
 
-  TestViewTreeNode v11;
+  ViewTreeNode v11;
+  v11.set_owned_by_parent(false);
   TreeChangeObserver o11(&v11);
   EXPECT_TRUE(o11.received_params().empty());
 
@@ -187,13 +178,17 @@ TEST_F(ViewTreeNodeObserverTest, TreeChange_SimpleAddRemove) {
 //  +- v1112
 // Then adds/removes v111 from v11.
 TEST_F(ViewTreeNodeObserverTest, TreeChange_NestedAddRemove) {
-  TestViewTreeNode v1, v11, v111, v1111, v1112;
+  ViewTreeNode v1, v11, v111, v1111, v1112;
 
   // Root tree.
+  v11.set_owned_by_parent(false);
   v1.AddChild(&v11);
 
   // Tree to be attached.
+  v111.set_owned_by_parent(false);
+  v1111.set_owned_by_parent(false);
   v111.AddChild(&v1111);
+  v1112.set_owned_by_parent(false);
   v111.AddChild(&v1112);
 
   TreeChangeObserver o1(&v1), o11(&v11), o111(&v111), o1111(&v1111),
@@ -294,7 +289,10 @@ TEST_F(ViewTreeNodeObserverTest, TreeChange_NestedAddRemove) {
 }
 
 TEST_F(ViewTreeNodeObserverTest, TreeChange_Reparent) {
-  TestViewTreeNode v1, v11, v12, v111;
+  ViewTreeNode v1, v11, v12, v111;
+  v11.set_owned_by_parent(false);
+  v111.set_owned_by_parent(false);
+  v12.set_owned_by_parent(false);
   v1.AddChild(&v11);
   v1.AddChild(&v12);
   v11.AddChild(&v111);
diff --git a/mojo/services/public/cpp/view_manager/view_manager.h b/mojo/services/public/cpp/view_manager/view_manager.h
index a06d566..629a933 100644
--- a/mojo/services/public/cpp/view_manager/view_manager.h
+++ b/mojo/services/public/cpp/view_manager/view_manager.h
@@ -5,8 +5,6 @@
 #ifndef MOJO_SERVICES_PUBLIC_CPP_VIEW_MANAGER_VIEW_MANAGER_H_
 #define MOJO_SERVICES_PUBLIC_CPP_VIEW_MANAGER_VIEW_MANAGER_H_
 
-#include <map>
-
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "mojo/public/cpp/bindings/callback.h"
@@ -41,19 +39,14 @@ class ViewManager {
   // TODO(beng): this method could optionally not block if supplied a callback.
   void Init();
 
-  ViewTreeNode* tree() { return tree_; }
-
-  ViewTreeNode* GetNodeById(TransportNodeId id);
+  ViewTreeNode* tree() { return tree_.get(); }
 
  private:
   friend class ViewManagerPrivate;
-  typedef std::map<TransportNodeId, ViewTreeNode*> IdToNodeMap;
 
   Shell* shell_;
   scoped_ptr<ViewManagerSynchronizer> synchronizer_;
-  ViewTreeNode* tree_;
-
-  IdToNodeMap nodes_;
+  scoped_ptr<ViewTreeNode> tree_;
 
   DISALLOW_COPY_AND_ASSIGN(ViewManager);
 };
diff --git a/mojo/services/public/cpp/view_manager/view_tree_node.h b/mojo/services/public/cpp/view_manager/view_tree_node.h
index b2af9cb..280aaf8 100644
--- a/mojo/services/public/cpp/view_manager/view_tree_node.h
+++ b/mojo/services/public/cpp/view_manager/view_tree_node.h
@@ -18,21 +18,19 @@ namespace view_manager {
 class ViewManager;
 class ViewTreeNodeObserver;
 
-// ViewTreeNodes are owned by the ViewManager.
-// TODO(beng): Right now, you'll have to implement a ViewTreeNodeObserver to
-//             track destruction and NULL any pointers you have.
-//             Investigate some kind of smart pointer or weak pointer for these.
 class ViewTreeNode {
  public:
   typedef std::vector<ViewTreeNode*> Children;
 
-  static ViewTreeNode* Create(ViewManager* view_manager);
-
-  // Destroys this node and all its children.
-  void Destroy();
+  explicit ViewTreeNode(ViewManager* manager);
+  ViewTreeNode();  // Used for tests.
+  ~ViewTreeNode();
 
   // Configuration.
   TransportNodeId id() const { return id_; }
+  void set_owned_by_parent(bool owned_by_parent) {
+    owned_by_parent_ = owned_by_parent;
+  }
 
   // Observation.
   void AddObserver(ViewTreeNodeObserver* observer);
@@ -50,22 +48,15 @@ class ViewTreeNode {
 
   ViewTreeNode* GetChildById(TransportNodeId id);
 
- protected:
-  // This class is subclassed only by test classes that provide a public ctor.
-  ViewTreeNode();
-  ~ViewTreeNode();
-
  private:
   friend class ViewTreeNodePrivate;
 
-  explicit ViewTreeNode(ViewManager* manager);
-
-  void LocalDestroy();
   void LocalAddChild(ViewTreeNode* child);
   void LocalRemoveChild(ViewTreeNode* child);
 
   ViewManager* manager_;
   TransportNodeId id_;
+  bool owned_by_parent_;
   ViewTreeNode* parent_;
   Children children_;
 
diff --git a/mojo/services/public/cpp/view_manager/view_tree_node_observer.h b/mojo/services/public/cpp/view_manager/view_tree_node_observer.h
index ac55ae6..4b4e6e0 100644
--- a/mojo/services/public/cpp/view_manager/view_tree_node_observer.h
+++ b/mojo/services/public/cpp/view_manager/view_tree_node_observer.h
@@ -33,9 +33,6 @@ class ViewTreeNodeObserver {
 
   virtual void OnTreeChange(const TreeChangeParams& params) {}
 
-  virtual void OnNodeDestroy(ViewTreeNode* node,
-                             DispositionChangePhase phase) {}
-
  protected:
   virtual ~ViewTreeNodeObserver() {}
 };
diff --git a/mojo/services/public/interfaces/view_manager/view_manager.mojom b/mojo/services/public/interfaces/view_manager/view_manager.mojom
index ebc82cb..1636a6a 100644
--- a/mojo/services/public/interfaces/view_manager/view_manager.mojom
+++ b/mojo/services/public/interfaces/view_manager/view_manager.mojom
@@ -80,9 +80,6 @@ interface IViewManagerClient {
                      uint32 new_view_id,
                      uint32 old_view_id,
                      uint32 change_id);
-
-  // Invoked when a node is deleted.
-  OnNodeDeleted(uint32 node, uint32 change_id);
 };
 
 }
diff --git a/mojo/services/view_manager/root_node_manager.cc b/mojo/services/view_manager/root_node_manager.cc
index 6517b32..529306d 100644
--- a/mojo/services/view_manager/root_node_manager.cc
+++ b/mojo/services/view_manager/root_node_manager.cc
@@ -108,16 +108,6 @@ void RootNodeManager::NotifyNodeViewReplaced(const NodeId& node,
   }
 }
 
-void RootNodeManager::NotifyNodeDeleted(const NodeId& node) {
-  for (ConnectionMap::iterator i = connection_map_.begin();
-       i != connection_map_.end(); ++i) {
-    const TransportChangeId change_id =
-        (change_ && i->first == change_->connection_id) ?
-        change_->change_id : 0;
-    i->second->NotifyNodeDeleted(node, change_id);
-  }
-}
-
 void RootNodeManager::PrepareForChange(ViewManagerConnection* connection,
                                        TransportChangeId change_id) {
   DCHECK(!change_.get());  // Should only ever have one change in flight.
diff --git a/mojo/services/view_manager/root_node_manager.h b/mojo/services/view_manager/root_node_manager.h
index d97acea..91aa8e7 100644
--- a/mojo/services/view_manager/root_node_manager.h
+++ b/mojo/services/view_manager/root_node_manager.h
@@ -71,7 +71,6 @@ class MOJO_VIEW_MANAGER_EXPORT RootNodeManager : public NodeDelegate {
   void NotifyNodeViewReplaced(const NodeId& node,
                               const ViewId& new_view_id,
                               const ViewId& old_view_id);
-  void NotifyNodeDeleted(const NodeId& node);
 
  private:
   // Used to setup any static state needed by RootNodeManager.
diff --git a/mojo/services/view_manager/view_manager_connection.cc b/mojo/services/view_manager/view_manager_connection.cc
index e1a4650..b11badd 100644
--- a/mojo/services/view_manager/view_manager_connection.cc
+++ b/mojo/services/view_manager/view_manager_connection.cc
@@ -129,11 +129,6 @@ void ViewManagerConnection::NotifyNodeViewReplaced(
                                change_id);
 }
 
-void ViewManagerConnection::NotifyNodeDeleted(const NodeId& node,
-                                              TransportChangeId change_id) {
-  client()->OnNodeDeleted(NodeIdToTransportId(node), change_id);
-}
-
 bool ViewManagerConnection::DeleteNodeImpl(ViewManagerConnection* source,
                                            const NodeId& node_id,
                                            TransportChangeId change_id) {
@@ -150,7 +145,6 @@ bool ViewManagerConnection::DeleteNodeImpl(ViewManagerConnection* source,
   DCHECK(node->GetChildren().empty());
   node_map_.erase(node_id.node_id);
   delete node;
-  context()->NotifyNodeDeleted(node_id);
   return true;
 }
 
diff --git a/mojo/services/view_manager/view_manager_connection.h b/mojo/services/view_manager/view_manager_connection.h
index 2dc31d8..ebb7a25 100644
--- a/mojo/services/view_manager/view_manager_connection.h
+++ b/mojo/services/view_manager/view_manager_connection.h
@@ -53,7 +53,6 @@ class MOJO_VIEW_MANAGER_EXPORT ViewManagerConnection
                               const ViewId& new_view_id,
                               const ViewId& old_view_id,
                               TransportChangeId change_id);
-  void NotifyNodeDeleted(const NodeId& node, TransportChangeId change_id);
 
  private:
   typedef std::map<TransportConnectionSpecificNodeId, Node*> NodeMap;
diff --git a/mojo/services/view_manager/view_manager_connection_unittest.cc b/mojo/services/view_manager/view_manager_connection_unittest.cc
index 48c520d..dea435b 100644
--- a/mojo/services/view_manager/view_manager_connection_unittest.cc
+++ b/mojo/services/view_manager/view_manager_connection_unittest.cc
@@ -223,14 +223,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)
@@ -453,12 +445,11 @@ TEST_F(ViewManagerConnectionTest, DeleteNode) {
                            CreateNodeId(client_.id(), 1),
                            121));
     Changes changes(client_.GetAndClearChanges());
-    ASSERT_EQ(3u, changes.size());
+    ASSERT_EQ(2u, changes.size());
     EXPECT_EQ("change_id=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",
               changes[1]);
-    EXPECT_EQ("change_id=121 node=1,1 deleted", changes[2]);
   }
 }
 
@@ -516,10 +507,9 @@ TEST_F(ViewManagerConnectionTest, DeleteNodeWithView) {
                            CreateNodeId(client_.id(), 1),
                            121));
     Changes changes(client_.GetAndClearChanges());
-    ASSERT_EQ(2u, changes.size());
+    ASSERT_EQ(1u, 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]);
   }
 
   // Set view 11 on node 2.