Fix object ownership in ui/base/models.

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 <memory>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/macros.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.
 //
 // TreeNodes do NOT maintain a pointer back to the model. As such, if you
 // are using TreeNodes with a TreeNodeModel you will need to notify the observer
 // yourself any time you make any change directly to the TreeNodes. For example,
 // if you directly invoke set_title on a node it does not notify the observer,
 // you will need to do it yourself. This includes the following methods: Add,
 // Remove and set_title. TreeNodeModel provides cover methods that mutate the
 // TreeNodes and notify the observer. If you are using TreeNodes with a
 // TreeNodeModel use the cover methods to save yourself the headache.
 //
 // The following example creates a TreeNode with two children and then
 // creates a TreeNodeModel from it:
 //
-// TreeNodeWithValue<int>* root = new TreeNodeWithValue<int>();
-// root->Add(new TreeNodeWithValue<int>(ASCIIToUTF16("child 1"), 0));
-// root->Add(new TreeNodeWithValue<int>(ASCIIToUTF16("child 2"), 1));
-// TreeNodeModel<TreeNodeWithValue<int> > model(root);
+// std::unique_ptr<TreeNodeWithValue<int>> root =
+//     base::MakeUnique<TreeNodeWithValue<int>>();
+// root->Add(
+//     base::MakeUnique<TreeNodeWithValue<int>>(ASCIIToUTF16("child 1"), 0));
+// root->Add(
+//     base::MakeUnique<TreeNodeWithValue<int>>(ASCIIToUTF16("child 2"), 1));
+// TreeNodeModel<TreeNodeWithValue<int>> model(std::move(root));
 //
 // Two variants of TreeNode are provided here:
 //
 // . TreeNode itself is intended for subclassing. It has one type parameter
 //   that corresponds to the type of the node. When subclassing use your class
-//   name as the type parameter, eg:
+//   name as the type parameter, e.g.:
 //   class MyTreeNode : public TreeNode<MyTreeNode> .
 // . TreeNodeWithValue is a trivial subclass of TreeNode that has one type
 //   type parameter: a value type that is associated with the node.
 //
 // Which you use depends upon the situation. If you want to subclass and add
 // methods, then use TreeNode. If you don't need any extra methods and just
 // want to associate a value with each node, then use TreeNodeWithValue.
 //
 // Regardless of which TreeNode you use, if you are using the nodes with a
 // TreeView take care to notify the observer when mutating the nodes.
 
 // TreeNode -------------------------------------------------------------------
 
 template <class NodeType>
 class TreeNode : public TreeModelNode {
  public:
-  TreeNode() : parent_(NULL) {}
+  TreeNode() : parent_(nullptr) {}
 
   explicit TreeNode(const base::string16& title)
-      : title_(title), parent_(NULL) {}
+      : title_(title), parent_(nullptr) {}
 
-  ~TreeNode() override { base::STLDeleteElements(&children_); }
+  ~TreeNode() override {}
 
-  // Adds |node| as a child of this node, at |index|.
-  virtual void Add(NodeType* node, int index) {
+  // Adds |node| as a child of this node, at |index|. Returns a raw pointer to
+  // the node.
+  virtual NodeType* Add(std::unique_ptr<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);
+    DCHECK(!node->parent_);
     node->parent_ = static_cast<NodeType*>(this);
-    children_.insert(children_.begin() + index, node);
+    NodeType* node_ptr = node.get();
+    children_.insert(children_.begin() + index, std::move(node));
+    return node_ptr;
   }
 
-  // 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);
+  // Removes |node| from this node and returns it.
+  virtual std::unique_ptr<NodeType> Remove(NodeType* node) {
+    auto i = std::find_if(children_.begin(), children_.end(),

[sky, 2016/09/29 22:06:48]

Same code exists on 141-144. Move to a common function?

[Avi (use Gerrit), 2016/09/29 22:59:58]

FYI the two callers are of different const-ness. Either we implement the
function twice, or we const_cast. (Unless I'm missing something.)

I'm const_casting; let me know what you think.

[Avi (use Gerrit), 2016/09/30 00:16:48]

aaaaaand I can't const_cast. I'm stuck with a const iterator to use on
vector::erase, and linux won't let me erase with a const iterator!
https://groups.google.com/a/chromium.org/d/msg/cxx/FJvc7odo2Us/o5fiZBr4CwAJ

I have to put this back; sorry.

[sky, 2016/09/30 03:07:05]

<rant>
I like the thought of containers with std::unique_ptr, but IMO unique_ptr makes
for a level of awkwardness that ScopedVector doesn't have. find is a good
example of the extra level of awkwardness.
</rant>

+                          [node](const std::unique_ptr<NodeType>& ptr) {
+                            return ptr.get() == node;
+                          });
     DCHECK(i != children_.end());
-    node->parent_ = NULL;
+    node->parent_ = nullptr;
+    std::unique_ptr<NodeType> ptr = std::move(*i);
     children_.erase(i);
-    return node;
+    return ptr;
   }
 
-  // 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_.clear();
-  }
+  // Removes all the children from this node.
+  void DeleteAll() { 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));
-  }
-
-  // Returns the parent node, or NULL if this is the root node.
+  // Returns the parent node, or nullptr if this is the root node.
   const NodeType* parent() const { return parent_; }
   NodeType* parent() { return parent_; }
 
   // Returns true if this is the root node.
-  bool is_root() const { return parent_ == NULL; }
+  bool is_root() const { return parent_ == nullptr; }
 
   // Returns the number of children.
   int child_count() const { return static_cast<int>(children_.size()); }
 
   // Returns true if this node has no children.
   bool empty() const { return children_.empty(); }
 
   // Returns the number of all nodes in the subtree rooted at this node,
   // including this node.
   int GetTotalNodeCount() const {
     int count = 1;  // Start with one to include the node itself.
-    for (size_t i = 0; i < children_.size(); ++i)
-      count += children_[i]->GetTotalNodeCount();
+    for (const auto& child : children_)
+      count += child->GetTotalNodeCount();
     return count;
   }
 
   // Returns the node at |index|.
   const NodeType* GetChild(int index) const {
     DCHECK_GE(index, 0);
     DCHECK_LT(index, child_count());
-    return children_[index];
+    return children_[index].get();
   }
   NodeType* GetChild(int index) {
     return const_cast<NodeType*>(
         static_cast<const NodeType&>(*this).GetChild(index));
   }
 
   // Returns the index of |node|, or -1 if |node| is not a child of this.
   int GetIndexOf(const NodeType* node) const {
     DCHECK(node);
-    typename std::vector<NodeType*>::const_iterator i =
-        std::find(children_.begin(), children_.end(), node);
+    auto i = std::find_if(children_.begin(), children_.end(),
+                          [node](const std::unique_ptr<NodeType>& ptr) {
+                            return ptr.get() == node;
+                          });
     return i != children_.end() ? static_cast<int>(i - children_.begin()) : -1;
   }
 
   // Sets the title of the node.
   virtual void SetTitle(const base::string16& title) { title_ = title; }
 
   // TreeModelNode:
   const base::string16& GetTitle() const override { return title_; }
 
   // Returns true if this == ancestor, or one of this nodes parents is
   // 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_; }
+  std::vector<std::unique_ptr<NodeType>>& children() { return children_; }
 
  private:
   // Title displayed in the tree.
   base::string16 title_;
 
   // This node's parent.
   NodeType* parent_;
 
   // This node's children.
-  std::vector<NodeType*> children_;
+  std::vector<std::unique_ptr<NodeType>> children_;
 
   DISALLOW_COPY_AND_ASSIGN(TreeNode);
 };
 
 // TreeNodeWithValue ----------------------------------------------------------
 
 template <class ValueType>
-class TreeNodeWithValue : public TreeNode< TreeNodeWithValue<ValueType> > {
+class TreeNodeWithValue : public TreeNode<TreeNodeWithValue<ValueType>> {
  public:
   TreeNodeWithValue() {}
 
   explicit TreeNodeWithValue(const ValueType& value)
       : ParentType(base::string16()), value(value) {}
 
   TreeNodeWithValue(const base::string16& title, const ValueType& value)
       : ParentType(title), value(value) {}
 
   ValueType value;
 
  private:
-  typedef TreeNode< TreeNodeWithValue<ValueType> > ParentType;
+  using ParentType = TreeNode<TreeNodeWithValue<ValueType>>;
 
   DISALLOW_COPY_AND_ASSIGN(TreeNodeWithValue);
 };
 
 // TreeNodeModel --------------------------------------------------------------
 
 // TreeModel implementation intended to be used with TreeNodes.
 template <class NodeType>
 class TreeNodeModel : public TreeModel {
  public:
-  // Creates a TreeNodeModel with the specified root node. The root is owned
-  // by the TreeNodeModel.
-  explicit TreeNodeModel(NodeType* root) : root_(root) {}
+  // Creates a TreeNodeModel with the specified root node.
+  explicit TreeNodeModel(std::unique_ptr<NodeType> root)
+      : root_(std::move(root)) {}
   virtual ~TreeNodeModel() override {}
 
   NodeType* AsNode(TreeModelNode* model_node) {
     return static_cast<NodeType*>(model_node);
   }
 
-  void Add(NodeType* parent, NodeType* node, int index) {
+  NodeType* Add(NodeType* parent, std::unique_ptr<NodeType> node, int index) {
     DCHECK(parent && node);
-    parent->Add(node, index);
+    NodeType* node_ptr = parent->Add(std::move(node), index);
     NotifyObserverTreeNodesAdded(parent, index, 1);
+    return node_ptr;
   }
 
-  NodeType* Remove(NodeType* parent, NodeType* node) {
+  std::unique_ptr<NodeType> Remove(NodeType* parent, NodeType* node) {
     DCHECK(parent);
     int index = parent->GetIndexOf(node);
-    NodeType* delete_node = parent->Remove(node);
+    std::unique_ptr<NodeType> owned_node = parent->Remove(node);
     NotifyObserverTreeNodesRemoved(parent, index, 1);
-    return delete_node;
+    return owned_node;
   }
 
   void NotifyObserverTreeNodesAdded(NodeType* parent, int start, int count) {
     FOR_EACH_OBSERVER(TreeModelObserver,
                       observer_list_,
                       TreeNodesAdded(this, parent, start, count));
   }
 
   void NotifyObserverTreeNodesRemoved(NodeType* parent, int start, int count) {
     FOR_EACH_OBSERVER(TreeModelObserver,
@@ skipping 53 matching lines @@
 
   // The root.
   std::unique_ptr<NodeType> root_;
 
   DISALLOW_COPY_AND_ASSIGN(TreeNodeModel);
 };
 
 }  // namespace ui
 
 #endif  // UI_BASE_MODELS_TREE_NODE_MODEL_H_