Avoid unnacessarily conversion from index to BookmarkNode pointer.

BookmarkModel::Remove() should take BookmarkNode pointer.

This will few save conversion from BookmarkNode pointer to index for calling Remove and converting index to BookmarkNode pointer to call RemoveAndDeleteNode().

This way we will able to save call's like parent->GetIndexOf(node),and again conversion to Bookmark Pointer.

BUG=330842

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

Cr-Commit-Position: refs/heads/master@{#328135}

10 files changed, 25 insertions, 26 deletions

diff --git a/components/bookmarks/browser/bookmark_expanded_state_tracker_unittest.cc b/components/bookmarks/browser/bookmark_expanded_state_tracker_unittest.cc
index 379e4ec..f8cefa9 100644
--- a/components/bookmarks/browser/bookmark_expanded_state_tracker_unittest.cc
+++ b/components/bookmarks/browser/bookmark_expanded_state_tracker_unittest.cc
@@ -88,7 +88,7 @@ TEST_F(BookmarkExpandedStateTrackerTest, SetExpandedNodes) {
   EXPECT_EQ(nodes, tracker->GetExpandedNodes());
 
   // Remove the folder, which should remove it from the list of expanded nodes.
-  model_->Remove(model_->bookmark_bar_node(), 0);
+  model_->Remove(model_->bookmark_bar_node()->GetChild(0));
   nodes.erase(n1);
   n1 = NULL;
   EXPECT_EQ(nodes, tracker->GetExpandedNodes());
diff --git a/components/bookmarks/browser/bookmark_index_unittest.cc b/components/bookmarks/browser/bookmark_index_unittest.cc
index 22efeaa..cab18e5 100644
--- a/components/bookmarks/browser/bookmark_index_unittest.cc
+++ b/components/bookmarks/browser/bookmark_index_unittest.cc
@@ -452,7 +452,7 @@ TEST_F(BookmarkIndexTest, Remove) {
   AddBookmarks(titles, urls, arraysize(titles));
 
   // Remove the node and make sure we don't get back any results.
-  model_->Remove(model_->other_node(), 0);
+  model_->Remove(model_->other_node()->GetChild(0));
   ExpectMatches("A", NULL, 0U);
 }
 
diff --git a/components/bookmarks/browser/bookmark_model.cc b/components/bookmarks/browser/bookmark_model.cc
index 10d5b2f..2e174db 100644
--- a/components/bookmarks/browser/bookmark_model.cc
+++ b/components/bookmarks/browser/bookmark_model.cc
@@ -188,12 +188,13 @@ void BookmarkModel::EndGroupedChanges() {
                     GroupedBookmarkChangesEnded(this));
 }
 
-void BookmarkModel::Remove(const BookmarkNode* parent, int index) {
-  if (!loaded_ || !IsValidIndex(parent, index, false) || is_root_node(parent)) {
+void BookmarkModel::Remove(const BookmarkNode* node) {
+  DCHECK(node);
+  if (!loaded_ || is_root_node(node)) {
     NOTREACHED();
     return;
   }
-  RemoveAndDeleteNode(AsMutable(parent->GetChild(index)));
+  RemoveAndDeleteNode(AsMutable(node));
 }
 
 void BookmarkModel::RemoveAllUserBookmarks() {
@@ -847,6 +848,7 @@ void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
   const BookmarkNode* parent = node->parent();
   DCHECK(parent);
   int index = parent->GetIndexOf(node.get());
+  DCHECK_NE(-1, index);
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     OnWillRemoveBookmarks(this, parent, index, node.get()));
diff --git a/components/bookmarks/browser/bookmark_model.h b/components/bookmarks/browser/bookmark_model.h
index 076b01b..e222790 100644
--- a/components/bookmarks/browser/bookmark_model.h
+++ b/components/bookmarks/browser/bookmark_model.h
@@ -127,9 +127,9 @@ class BookmarkModel : public KeyedService {
   // state during their own initializer, such as the NTP.
   bool IsDoingExtensiveChanges() const { return extensive_changes_ > 0; }
 
-  // Removes the node at the given |index| from |parent|. Removing a folder node
+  // Removes |node| from the model and deletes it. Removing a folder node
   // recursively removes all nodes. Observers are notified immediately.
-  void Remove(const BookmarkNode* parent, int index);
+  void Remove(const BookmarkNode* node);
 
   // Removes all the non-permanent bookmark nodes that are editable by the user.
   // Observers are only notified when all nodes have been removed. There is no
diff --git a/components/bookmarks/browser/bookmark_model_unittest.cc b/components/bookmarks/browser/bookmark_model_unittest.cc
index 1ee3554..1f71f01 100644
--- a/components/bookmarks/browser/bookmark_model_unittest.cc
+++ b/components/bookmarks/browser/bookmark_model_unittest.cc
@@ -474,7 +474,7 @@ TEST_F(BookmarkModelTest, RemoveURL) {
   model_->AddURL(root, 0, title, url);
   ClearCounts();
 
-  model_->Remove(root, 0);
+  model_->Remove(root->GetChild(0));
   ASSERT_EQ(0, root->child_count());
   AssertObserverCount(0, 0, 1, 0, 0, 1, 0, 0, 0);
   observer_details_.ExpectEquals(root, NULL, 0, -1);
@@ -497,7 +497,7 @@ TEST_F(BookmarkModelTest, RemoveFolder) {
   ClearCounts();
 
   // Now remove the folder.
-  model_->Remove(root, 0);
+  model_->Remove(root->GetChild(0));
   ASSERT_EQ(0, root->child_count());
   AssertObserverCount(0, 0, 1, 0, 0, 1, 0, 0, 0);
   observer_details_.ExpectEquals(root, NULL, 0, -1);
@@ -612,7 +612,7 @@ TEST_F(BookmarkModelTest, Move) {
 
   // And remove the folder.
   ClearCounts();
-  model_->Remove(root, 0);
+  model_->Remove(root->GetChild(0));
   AssertObserverCount(0, 0, 1, 0, 0, 1, 0, 0, 0);
   observer_details_.ExpectEquals(root, NULL, 0, -1);
   EXPECT_TRUE(model_->GetMostRecentlyAddedUserNodeForURL(url) == NULL);
@@ -728,7 +728,7 @@ TEST_F(BookmarkModelTest, MostRecentlyModifiedFolders) {
 
   // Nuke the folder and do another fetch, making sure folder isn't in the
   // returned list.
-  model_->Remove(folder->parent(), 0);
+  model_->Remove(folder->parent()->GetChild(0));
   most_recent_folders = GetMostRecentlyModifiedUserFolders(model_.get(), 1);
   ASSERT_EQ(1U, most_recent_folders.size());
   ASSERT_TRUE(most_recent_folders[0] != folder);
diff --git a/components/bookmarks/browser/bookmark_utils.cc b/components/bookmarks/browser/bookmark_utils.cc
index 9138591..28690f2 100644
--- a/components/bookmarks/browser/bookmark_utils.cc
+++ b/components/bookmarks/browser/bookmark_utils.cc
@@ -247,7 +247,7 @@ void CopyToClipboard(BookmarkModel* model,
     for (size_t i = 0; i < filtered_nodes.size(); ++i) {
       int index = filtered_nodes[i]->parent()->GetIndexOf(filtered_nodes[i]);
       if (index > -1)
-        model->Remove(filtered_nodes[i]->parent(), index);
+        model->Remove(filtered_nodes[i]);
     }
   }
 }
@@ -472,8 +472,7 @@ void DeleteBookmarkFolders(BookmarkModel* model,
     const BookmarkNode* node = GetBookmarkNodeByID(model, *iter);
     if (!node)
       continue;
-    const BookmarkNode* parent = node->parent();
-    model->Remove(parent, parent->GetIndexOf(node));
+    model->Remove(node);
   }
 }
 
@@ -496,7 +495,7 @@ void RemoveAllBookmarks(BookmarkModel* model, const GURL& url) {
     const BookmarkNode* node = bookmarks[i];
     int index = node->parent()->GetIndexOf(node);
     if (index > -1 && model->client()->CanBeEditedByUser(node))
-      model->Remove(node->parent(), index);
+      model->Remove(node);
   }
 }
 
diff --git a/components/bookmarks/managed/managed_bookmarks_tracker.cc b/components/bookmarks/managed/managed_bookmarks_tracker.cc
index 5dd1acc..17eff02 100644
--- a/components/bookmarks/managed/managed_bookmarks_tracker.cc
+++ b/components/bookmarks/managed/managed_bookmarks_tracker.cc
@@ -172,7 +172,7 @@ void ManagedBookmarksTracker::UpdateBookmarks(const BookmarkNode* folder,
 
   // Remove any extra children of |folder| that haven't been reused.
   while (folder->child_count() != folder_index)
-    model_->Remove(folder, folder_index);
+    model_->Remove(folder->GetChild(folder_index));
 }
 
 // static
diff --git a/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc b/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
index 4b53abd..c7c29f9 100644
--- a/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
@@ -595,12 +595,11 @@ TEST_F(EnhancedBookmarkModelTest, ObserverNodeRemovedEvent) {
   const BookmarkNode* folder = AddFolder();
 
   EXPECT_EQ(0, removed_calls_);
-  bookmark_model_->Remove(node->parent(), node->parent()->GetIndexOf(node));
+  bookmark_model_->Remove(node);
   EXPECT_EQ(1, removed_calls_);
   EXPECT_EQ(node, last_removed_);
 
-  bookmark_model_->Remove(folder->parent(),
-                          folder->parent()->GetIndexOf(folder));
+  bookmark_model_->Remove(folder);
   EXPECT_EQ(2, removed_calls_);
   EXPECT_EQ(folder, last_removed_);
 }
@@ -675,7 +674,7 @@ TEST_F(EnhancedBookmarkModelTest, NodeRemovedWhileResetDuplicationScheduled) {
   const BookmarkNode* node2 = AddBookmark();
   bookmark_model_->SetNodeMetaInfo(node1, "stars.id", "c_1");
   bookmark_model_->SetNodeMetaInfo(node2, "stars.id", "c_1");
-  bookmark_model_->Remove(node1->parent(), node1->parent()->GetIndexOf(node1));
+  bookmark_model_->Remove(node1);
   base::RunLoop().RunUntilIdle();
 }
 
@@ -686,8 +685,7 @@ TEST_F(EnhancedBookmarkModelTest,
   std::string remote_id = GetId(node);
   EXPECT_EQ(node, model_->BookmarkForRemoteId(remote_id));
 
-  const BookmarkNode* gp = parent->parent();
-  bookmark_model_->Remove(gp, gp->GetIndexOf(parent));
+  bookmark_model_->Remove(parent);
   EXPECT_FALSE(model_->BookmarkForRemoteId(remote_id));
 }
 
diff --git a/components/undo/bookmark_undo_service.cc b/components/undo/bookmark_undo_service.cc
index 695bacb..21a3411 100644
--- a/components/undo/bookmark_undo_service.cc
+++ b/components/undo/bookmark_undo_service.cc
@@ -84,7 +84,7 @@ void BookmarkAddOperation::Undo() {
       bookmarks::GetBookmarkNodeByID(model, parent_id_);
   DCHECK(parent);
 
-  model->Remove(parent, index_);
+  model->Remove(parent->GetChild(index_));
 }
 
 int BookmarkAddOperation::GetUndoLabelId() const {
diff --git a/components/undo/bookmark_undo_service_test.cc b/components/undo/bookmark_undo_service_test.cc
index e97988a..2e989fd 100644
--- a/components/undo/bookmark_undo_service_test.cc
+++ b/components/undo/bookmark_undo_service_test.cc
@@ -90,7 +90,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkRemove) {
 
   const BookmarkNode* parent = model->other_node();
   model->AddURL(parent, 0, ASCIIToUTF16("foo"), GURL("http://www.bar.com"));
-  model->Remove(parent, 0);
+  model->Remove(parent->GetChild(0));
 
   EXPECT_EQ(2U, undo_service->undo_manager()->undo_count());
   EXPECT_EQ(0U, undo_service->undo_manager()->redo_count());
@@ -232,7 +232,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkRenameDelete) {
                                            ASCIIToUTF16("folder"));
   model->AddURL(f1, 0, ASCIIToUTF16("foo"), GURL("http://www.foo.com"));
   model->SetTitle(f1, ASCIIToUTF16("Renamed"));
-  model->Remove(model->other_node(), 0);
+  model->Remove(model->other_node()->GetChild(0));
 
   // Undo the folder removal and ensure the folder and bookmark were restored.
   undo_service->undo_manager()->Undo();
@@ -366,7 +366,7 @@ TEST_F(BookmarkUndoServiceTest, UndoRemoveFolderWithBookmarks) {
   new_folder = model->AddFolder(parent, 0, ASCIIToUTF16("folder"));
   model->AddURL(new_folder, 0, ASCIIToUTF16("bar"), GURL("http://www.bar.com"));
 
-  model->Remove(parent, 0);
+  model->Remove(parent->GetChild(0));
 
   // Test that the undo restores the bookmark and folder.
   undo_service->undo_manager()->Undo();