ui/base/models: rewrite TreeNode without using ScopedVector

ui/base/models/tree_node_model.h

Index: ui/base/models/tree_node_model.h
diff --git a/ui/base/models/tree_node_model.h b/ui/base/models/tree_node_model.h
index 9606dc8119c5b9e71233f49d4a9f9265197efa98..279667b2a406d7f2bbeb507299b9763f3470875c 100644
--- a/ui/base/models/tree_node_model.h
+++ b/ui/base/models/tree_node_model.h
@@ -10,12 +10,11 @@
 #include <algorithm>
 #include <vector>
 
-#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
+#include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "ui/base/models/tree_model.h"
 
@@ -70,7 +69,9 @@ class TreeNode : public TreeModelNode {
   explicit TreeNode(const base::string16& title)
       : title_(title), parent_(NULL) {}
 
-  ~TreeNode() override {}
+  ~TreeNode() override {
+    STLDeleteElements(&children_);
+  }
 
   // Adds |node| as a child of this node, at |index|.
   virtual void Add(NodeType* node, int index) {
@@ -92,7 +93,7 @@ class TreeNode : public TreeModelNode {
         std::find(children_.begin(), children_.end(), node);
     DCHECK(i != children_.end());
     node->parent_ = NULL;
-    children_.weak_erase(i);
+    children_.erase(i);
     return node;
   }
 
@@ -100,7 +101,7 @@ class TreeNode : public TreeModelNode {
   void RemoveAll() {
     for (size_t i = 0; i < children_.size(); ++i)
       children_[i]->parent_ = NULL;
-    children_.weak_clear();
+    children_.clear();
   }
 
   // Removes all existing children without deleting the nodes and adds all nodes
@@ -169,7 +170,7 @@ class TreeNode : public TreeModelNode {
   }
 
  protected:
-  std::vector<NodeType*>& children() { return children_.get(); }
+  std::vector<NodeType*>& children() { return children_; }

[tfarina, 2016/01/21 14:24:45]

I had to use std::vector<NodeType*> because of this accessor. I wish we didn't
have it all!

[sky, 2016/01/21 18:53:54]

I disagree. It's an implementation detail as to how this class stores the
underlying children and this accessor is meant to return the children. It
doesn't make sense to return a vector<scoped_ptr<NodeType>>& here.

[vabr (Chromium), 2016/01/22 13:57:51]

Does that mean that the accessor is meant to abstract the way the children are
stored? It gives full access exactly to the storage, which does not seem like an
abstraction.

It's not clear to me how the caller is supposed to use the accessor. Because it
has no const in the return type, and no comment about permitted usage, it seems
like it is meant to modify the storage of children. But then I don't understand
why it would not make sense to expose the storage as a vector of scoped_ptrs.

[sky, 2016/01/22 16:42:09]

Good point. I missed that it isn't const. Callers are allowed to mutate to their
hearts content.

[vabr (Chromium), 2016/01/22 16:44:49]

Right, in which case it seems like exposing the scoped_ptrs in the vector would
make it easier for the callers not to mess up ownership accidentally. Would you
agree with that?

 
  private:
   // Title displayed in the tree.
@@ -179,7 +180,7 @@ class TreeNode : public TreeModelNode {
   NodeType* parent_;
 
   // This node's children.
-  ScopedVector<NodeType> children_;
+  std::vector<NodeType*> children_;
 
   DISALLOW_COPY_AND_ASSIGN(TreeNode);
 };