ui/base/models: rewrite TreeNode without using ScopedVector

ui/base/models/tree_node_model.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef UI_BASE_MODELS_TREE_NODE_MODEL_H_
 #define UI_BASE_MODELS_TREE_NODE_MODEL_H_
 
 #include <stddef.h>
 
 #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"
 
 namespace ui {
 
 // TreeNodeModel and TreeNodes provide an implementation of TreeModel around
 // TreeNodes.
 //
 // TreeNodes own their children, so that deleting a node deletes all
 // descendants.
@@ skipping 34 matching lines @@
 // TreeNode -------------------------------------------------------------------
 
 template <class NodeType>
 class TreeNode : public TreeModelNode {
  public:
   TreeNode() : parent_(NULL) {}
 
   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) {
     DCHECK(node);
     DCHECK_GE(index, 0);
     DCHECK_LE(index, child_count());
     // If |node| has a parent, remove it from its parent.
     NodeType* parent = node->parent_;
     if (parent)
       parent->Remove(node);
     node->parent_ = static_cast<NodeType*>(this);
     children_.insert(children_.begin() + index, node);
   }
 
   // Removes |node| from this node and returns it. It's up to the caller to
   // delete it.
   virtual NodeType* Remove(NodeType* node) {
     typename std::vector<NodeType*>::iterator i =
         std::find(children_.begin(), children_.end(), node);
     DCHECK(i != children_.end());
     node->parent_ = NULL;
-    children_.weak_erase(i);
+    children_.erase(i);
     return node;
   }
 
   // Removes all the children from this node. This does NOT delete the nodes.
   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
   // contained in |children| into this node as children.
   void SetChildren(const std::vector<NodeType*>& children) {
     RemoveAll();
     for (size_t i = 0; i < children.size(); ++i)
       Add(children[i], static_cast<int>(i));
   }
 
@@ skipping 48 matching lines @@
   // ancestor.
   bool HasAncestor(const NodeType* ancestor) const {
     if (ancestor == this)
       return true;
     if (!ancestor)
       return false;
     return parent_ ? parent_->HasAncestor(ancestor) : false;
   }
 
  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.
   base::string16 title_;
 
   // This node's parent.
   NodeType* parent_;
 
   // This node's children.
-  ScopedVector<NodeType> children_;
+  std::vector<NodeType*> children_;
 
   DISALLOW_COPY_AND_ASSIGN(TreeNode);
 };
 
 // TreeNodeWithValue ----------------------------------------------------------
 
 template <class ValueType>
 class TreeNodeWithValue : public TreeNode< TreeNodeWithValue<ValueType> > {
  public:
   TreeNodeWithValue() {}
@@ skipping 105 matching lines @@
 
   // The root.
   scoped_ptr<NodeType> root_;
 
   DISALLOW_COPY_AND_ASSIGN(TreeNodeModel);
 };
 
 }  // namespace ui
 
 #endif  // UI_BASE_MODELS_TREE_NODE_MODEL_H_