Fix object ownership in ui/base/models.

components/bookmarks/browser/bookmark_model.cc

Index: components/bookmarks/browser/bookmark_model.cc
diff --git a/components/bookmarks/browser/bookmark_model.cc b/components/bookmarks/browser/bookmark_model.cc
index 8d5b238a0c9a1d8cb90266e87f85b2d268dfb6d8..c77c7171f978a663dd55eda93af86b2e19ff409c 100644
--- a/components/bookmarks/browser/bookmark_model.cc
+++ b/components/bookmarks/browser/bookmark_model.cc
@@ -13,6 +13,7 @@
 #include "base/i18n/string_compare.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/string_util.h"
@@ -51,11 +52,11 @@ class VisibilityComparator {
  public:
   explicit VisibilityComparator(BookmarkClient* client) : client_(client) {}
 
-  // Returns true if |n1| preceeds |n2|.
-  bool operator()(const BookmarkPermanentNode* n1,
-                  const BookmarkPermanentNode* n2) {
-    bool n1_visible = client_->IsPermanentNodeVisible(n1);
-    bool n2_visible = client_->IsPermanentNodeVisible(n2);
+  // Returns true if |n1| precedes |n2|.
+  bool operator()(const std::unique_ptr<BookmarkPermanentNode>& n1,
+                  const std::unique_ptr<BookmarkPermanentNode>& n2) {
+    bool n1_visible = client_->IsPermanentNodeVisible(n1.get());
+    bool n2_visible = client_->IsPermanentNodeVisible(n2.get());
     return n1_visible != n2_visible && n1_visible;
   }
 
@@ -69,8 +70,9 @@ class SortComparator {
  public:
   explicit SortComparator(icu::Collator* collator) : collator_(collator) {}
 
-  // Returns true if |n1| preceeds |n2|.
-  bool operator()(const BookmarkNode* n1, const BookmarkNode* n2) {
+  // Returns true if |n1| precedes |n2|.
+  bool operator()(const std::unique_ptr<BookmarkNode>& n1,
+                  const std::unique_ptr<BookmarkNode>& n2) {
     if (n1->type() == n2->type()) {
       // Types are the same, compare the names.
       if (!collator_)
@@ -217,12 +219,9 @@ void BookmarkModel::Remove(const BookmarkNode* node) {
 void BookmarkModel::RemoveAllUserBookmarks() {
   std::set<GURL> removed_urls;
   struct RemoveNodeData {
-    RemoveNodeData(const BookmarkNode* parent, int index, BookmarkNode* node)
-        : parent(parent), index(index), node(node) {}
-
     const BookmarkNode* parent;
     int index;
-    BookmarkNode* node;
+    std::unique_ptr<BookmarkNode> node;
   };
   std::vector<RemoveNodeData> removed_node_data_list;
 
@@ -242,10 +241,9 @@ void BookmarkModel::RemoveAllUserBookmarks() {
         continue;
 
       for (int j = permanent_node->child_count() - 1; j >= 0; --j) {
-        BookmarkNode* child_node = AsMutable(permanent_node->GetChild(j));
-        RemoveNodeAndGetRemovedUrls(child_node, &removed_urls);
-        removed_node_data_list.push_back(
-            RemoveNodeData(permanent_node, j, child_node));
+        std::unique_ptr<BookmarkNode> node = RemoveNodeAndGetRemovedUrls(
+            AsMutable(permanent_node->GetChild(j)), &removed_urls);
+        removed_node_data_list.push_back({permanent_node, j, std::move(node)});
       }
     }
   }
@@ -257,10 +255,10 @@ void BookmarkModel::RemoveAllUserBookmarks() {
                     BookmarkAllUserNodesRemoved(this, removed_urls));
 
   BeginGroupedChanges();
-  for (const auto& removed_node_data : removed_node_data_list) {
-    undo_delegate()->OnBookmarkNodeRemoved(
-        this, removed_node_data.parent, removed_node_data.index,
-        std::unique_ptr<BookmarkNode>(removed_node_data.node));
+  for (auto& removed_node_data : removed_node_data_list) {
+    undo_delegate()->OnBookmarkNodeRemoved(this, removed_node_data.parent,
+                                           removed_node_data.index,
+                                           std::move(removed_node_data.node));
   }
   EndGroupedChanges();
 }
@@ -293,8 +291,12 @@ void BookmarkModel::Move(const BookmarkNode* node,
 
   if (old_parent == new_parent && index > old_index)
     index--;
+
+  BookmarkNode* mutable_old_parent = AsMutable(old_parent);
+  std::unique_ptr<BookmarkNode> owned_node =
+      mutable_old_parent->Remove(AsMutable(node));
   BookmarkNode* mutable_new_parent = AsMutable(new_parent);
-  mutable_new_parent->Add(AsMutable(node), index);
+  mutable_new_parent->Add(std::move(owned_node), index);
 
   if (store_.get())
     store_->ScheduleSave();
@@ -594,7 +596,8 @@ const BookmarkNode* BookmarkModel::AddFolderWithMetaInfo(
     return NULL;
   }
 
-  BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(), GURL());
+  std::unique_ptr<BookmarkNode> new_node =
+      base::MakeUnique<BookmarkNode>(generate_next_node_id(), GURL());
   new_node->set_date_folder_modified(Time::Now());
   // Folders shouldn't have line breaks in their titles.
   new_node->SetTitle(title);
@@ -602,7 +605,7 @@ const BookmarkNode* BookmarkModel::AddFolderWithMetaInfo(
   if (meta_info)
     new_node->SetMetaInfoMap(*meta_info);
 
-  return AddNode(AsMutable(parent), index, new_node);
+  return AddNode(AsMutable(parent), index, std::move(new_node));
 }
 
 const BookmarkNode* BookmarkModel::AddURL(const BookmarkNode* parent,
@@ -635,14 +638,15 @@ const BookmarkNode* BookmarkModel::AddURLWithCreationTimeAndMetaInfo(
   if (creation_time > parent->date_folder_modified())
     SetDateFolderModified(parent, creation_time);
 
-  BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(), url);
+  std::unique_ptr<BookmarkNode> new_node =
+      base::MakeUnique<BookmarkNode>(generate_next_node_id(), url);
   new_node->SetTitle(title);
   new_node->set_date_added(creation_time);
   new_node->set_type(BookmarkNode::URL);
   if (meta_info)
     new_node->SetMetaInfoMap(*meta_info);
 
-  return AddNode(AsMutable(parent), index, new_node);
+  return AddNode(AsMutable(parent), index, std::move(new_node));
 }
 
 void BookmarkModel::SortChildren(const BookmarkNode* parent) {
@@ -679,14 +683,18 @@ void BookmarkModel::ReorderChildren(
 
   // Ensure that all children in |parent| are in |ordered_nodes|.
   DCHECK_EQ(static_cast<size_t>(parent->child_count()), ordered_nodes.size());
-  for (size_t i = 0; i < ordered_nodes.size(); ++i)
-    DCHECK_EQ(parent, ordered_nodes[i]->parent());
+  for (const BookmarkNode* node : ordered_nodes)
+    DCHECK_EQ(parent, node->parent());
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     OnWillReorderBookmarkNode(this, parent));
 
-  AsMutable(parent)->SetChildren(
-      *(reinterpret_cast<const std::vector<BookmarkNode*>*>(&ordered_nodes)));
+  BookmarkNode* mutable_parent = AsMutable(parent);
+  for (size_t i = 0; i < ordered_nodes.size() - 1; ++i) {

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

if ordered_nodes is empty this code is problematic.

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

Done.

+    std::unique_ptr<BookmarkNode> node =
+        mutable_parent->Remove(AsMutable(ordered_nodes[i]));

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

This implementation is way more expensive than the old.

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

Yes. The only thought I have on how to do better is to pass the vector into the
TreeNode and have it rearrange the children. WDYT?

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

Actually I have an idea.

+    mutable_parent->Add(std::move(node), i);
+  }
 
   if (store_.get())
     store_->ScheduleSave();
@@ -752,16 +760,15 @@ const BookmarkPermanentNode* BookmarkModel::PermanentNode(
   }
 }
 
-void BookmarkModel::RestoreRemovedNode(
-    const BookmarkNode* parent,
-    int index,
-    std::unique_ptr<BookmarkNode> scoped_node) {
-  BookmarkNode* node = scoped_node.release();
-  AddNode(AsMutable(parent), index, node);
+void BookmarkModel::RestoreRemovedNode(const BookmarkNode* parent,
+                                       int index,
+                                       std::unique_ptr<BookmarkNode> node) {
+  BookmarkNode* node_ptr = node.get();
+  AddNode(AsMutable(parent), index, std::move(node));
 
   // We might be restoring a folder node that have already contained a set of
   // child nodes. We need to notify all of them.
-  NotifyNodeAddedForAllDescendents(node);
+  NotifyNodeAddedForAllDescendents(node_ptr);
 }
 
 void BookmarkModel::NotifyNodeAddedForAllDescendents(const BookmarkNode* node) {
@@ -826,14 +833,21 @@ void BookmarkModel::DoneLoading(std::unique_ptr<BookmarkLoadDetails> details) {
     if (store_.get())
       store_->ScheduleSave();
   }
-  bookmark_bar_node_ = details->release_bb_node();
-  other_node_ = details->release_other_folder_node();
-  mobile_node_ = details->release_mobile_folder_node();
-  index_.reset(details->release_index());
+  std::unique_ptr<BookmarkPermanentNode> owned_bb_node =
+      details->owned_bb_node();
+  std::unique_ptr<BookmarkPermanentNode> owned_other_folder_node =
+      details->owned_other_folder_node();
+  std::unique_ptr<BookmarkPermanentNode> owned_mobile_folder_node =
+      details->owned_mobile_folder_node();
+  index_ = details->owned_index();
+
+  bookmark_bar_node_ = owned_bb_node.get();

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

Can you set these members when you add them below?

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

Not sure what you mean. Have something like

  root_children.push_back(std::move(owned_bb_node));
  bookmark_bar_node_ = root_children.back().get();
  root_children.push_back(std::move(owned_other_folder_node));
  other_node_ = root_children.back().get();
  root_children.push_back(std::move(owned_mobile_folder_node));
  mobile_node_ = root_children.back().get();

? That doesn't read as much cleaner to me, but if you prefer...

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

My mistake. What you have is fine.

+  other_node_ = owned_other_folder_node.get();
+  mobile_node_ = owned_mobile_folder_node.get();
 
   // Get any extra nodes and take ownership of them at the |root_|.
-  std::vector<BookmarkPermanentNode*> extra_nodes;
-  details->release_extra_nodes(&extra_nodes);
+  std::vector<std::unique_ptr<BookmarkPermanentNode>> extra_nodes =
+      details->owned_extra_nodes();
 
   // TODO(robliao): Remove ScopedTracker below once https://crbug.com/467179
   // is fixed.
@@ -843,12 +857,12 @@ void BookmarkModel::DoneLoading(std::unique_ptr<BookmarkLoadDetails> details) {
   // WARNING: order is important here, various places assume the order is
   // constant (but can vary between embedders with the initial visibility
   // of permanent nodes).
-  std::vector<BookmarkPermanentNode*> root_children;
-  root_children.push_back(bookmark_bar_node_);
-  root_children.push_back(other_node_);
-  root_children.push_back(mobile_node_);
-  for (size_t i = 0; i < extra_nodes.size(); ++i)
-    root_children.push_back(extra_nodes[i]);
+  std::vector<std::unique_ptr<BookmarkPermanentNode>> root_children;
+  root_children.push_back(std::move(owned_bb_node));
+  root_children.push_back(std::move(owned_other_folder_node));
+  root_children.push_back(std::move(owned_mobile_folder_node));
+  std::move(extra_nodes.begin(), extra_nodes.end(),
+            std::back_inserter(root_children));
 
   // TODO(robliao): Remove ScopedTracker below once https://crbug.com/467179
   // is fixed.
@@ -859,7 +873,7 @@ void BookmarkModel::DoneLoading(std::unique_ptr<BookmarkLoadDetails> details) {
                    root_children.end(),
                    VisibilityComparator(client_.get()));
   for (size_t i = 0; i < root_children.size(); ++i)
-    root_.Add(root_children[i], static_cast<int>(i));
+    root_.Add(std::move(root_children[i]), static_cast<int>(i));
 
   root_.SetMetaInfoMap(details->model_meta_info_map());
   root_.set_sync_transaction_version(details->model_sync_transaction_version());
@@ -889,21 +903,21 @@ void BookmarkModel::DoneLoading(std::unique_ptr<BookmarkLoadDetails> details) {
                     BookmarkModelLoaded(this, details->ids_reassigned()));
 }
 
-void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
-  std::unique_ptr<BookmarkNode> node(delete_me);
+void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* node_ptr) {
+  std::unique_ptr<BookmarkNode> node;
 
-  const BookmarkNode* parent = node->parent();
+  const BookmarkNode* parent = node_ptr->parent();
   DCHECK(parent);
-  int index = parent->GetIndexOf(node.get());
+  int index = parent->GetIndexOf(node_ptr);
   DCHECK_NE(-1, index);
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
-                    OnWillRemoveBookmarks(this, parent, index, node.get()));
+                    OnWillRemoveBookmarks(this, parent, index, node_ptr));
 
   std::set<GURL> removed_urls;
   {
     base::AutoLock url_lock(url_lock_);
-    RemoveNodeAndGetRemovedUrls(node.get(), &removed_urls);
+    node = RemoveNodeAndGetRemovedUrls(node_ptr, &removed_urls);
   }
 
   if (store_.get())
@@ -931,16 +945,17 @@ void BookmarkModel::RemoveNodeFromInternalMaps(BookmarkNode* node) {
   nodes_ordered_by_url_set_.erase(i);
 }
 
-void BookmarkModel::RemoveNodeAndGetRemovedUrls(BookmarkNode* node,
-                                                std::set<GURL>* removed_urls) {
+std::unique_ptr<BookmarkNode> BookmarkModel::RemoveNodeAndGetRemovedUrls(
+    BookmarkNode* node_ptr,
+    std::set<GURL>* removed_urls) {
   // NOTE: this method should be always called with |url_lock_| held.
   // This method does not explicitly acquires a lock.
   url_lock_.AssertAcquired();
   DCHECK(removed_urls);
-  BookmarkNode* parent = node->parent();
+  BookmarkNode* parent = node_ptr->parent();
   DCHECK(parent);
-  parent->Remove(node);
-  RemoveNode(node, removed_urls);
+  std::unique_ptr<BookmarkNode> node = parent->Remove(node_ptr);
+  RemoveNode(node_ptr, removed_urls);
   // RemoveNode adds an entry to removed_urls for each node of type URL. As we
   // allow duplicates we need to remove any entries that are still bookmarked.
   for (std::set<GURL>::iterator i = removed_urls->begin();
@@ -955,25 +970,28 @@ void BookmarkModel::RemoveNodeAndGetRemovedUrls(BookmarkNode* node,
       ++i;
     }
   }
+
+  return node;
 }
 
 BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
                                      int index,
-                                     BookmarkNode* node) {
-  parent->Add(node, index);
+                                     std::unique_ptr<BookmarkNode> node) {
+  BookmarkNode* node_ptr = node.get();
+  parent->Add(std::move(node), index);
 
   if (store_.get())
     store_->ScheduleSave();
 
   {
     base::AutoLock url_lock(url_lock_);
-    AddNodeToInternalMaps(node);
+    AddNodeToInternalMaps(node_ptr);
   }
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeAdded(this, parent, index));
 
-  return node;
+  return node_ptr;
 }
 
 void BookmarkModel::AddNodeToInternalMaps(BookmarkNode* node) {