Supporting undoing bookmark deletion without creating new ID.

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 5709533dba2d0be60f7f29ded432599ba10be75a..0e4a4330d9e063daf05932af95f4fe8903c6ec65 100644
--- a/components/bookmarks/browser/bookmark_model.cc
+++ b/components/bookmarks/browser/bookmark_model.cc
@@ -104,7 +104,8 @@ BookmarkModel::BookmarkModel(BookmarkClient* client)
       observers_(
           base::ObserverList<BookmarkModelObserver>::NOTIFY_EXISTING_ONLY),
       loaded_signal_(true, false),
-      extensive_changes_(0) {
+      extensive_changes_(0),
+      undo_delegate_(nullptr) {
   DCHECK(client_);
 }
 
@@ -205,6 +206,7 @@ void BookmarkModel::RemoveAllUserBookmarks() {
                     OnWillRemoveAllUserBookmarks(this));
 
   BeginExtensiveChanges();
+  BeginGroupedChanges();
   // Skip deleting permanent nodes. Permanent bookmark nodes are the root and
   // its immediate children. For removing all non permanent nodes just remove
   // all children of non-root permanent nodes.
@@ -218,11 +220,18 @@ void BookmarkModel::RemoveAllUserBookmarks() {
 
       for (int j = permanent_node->child_count() - 1; j >= 0; --j) {
         BookmarkNode* child_node = permanent_node->GetChild(j);
-        removed_nodes.push_back(child_node);
         RemoveNodeAndGetRemovedUrls(child_node, &removed_urls);
+        if (undo_delegate_) {

[sky, 2015/10/07 16:30:15]

Simplify this so that there is always an UndoDelegate.

[jianli, 2015/10/07 23:26:55]

Done.

+          // The undo delegate will take the ownership.
+          undo_delegate_->OnBookmarkNodeDeleted(

[sky, 2015/10/07 16:30:15]

Don't call out to the UndoDelegate while a lock is held (line 214). Also, I
think you should notify UndoDelegate after observers (just as you don on 871
below).

[jianli, 2015/10/07 23:26:55]

Done.

+              this, permanent_node->id(), j, child_node);
+        } else {
+          removed_nodes.push_back(child_node);
+        }
       }
     }
   }
+  EndGroupedChanges();
   EndExtensiveChanges();
   if (store_.get())
     store_->ScheduleSave();
@@ -859,6 +868,11 @@ void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
       BookmarkModelObserver,
       observers_,
       BookmarkNodeRemoved(this, parent, index, node.get(), removed_urls));
+
+  if (undo_delegate_) {
+    undo_delegate_->OnBookmarkNodeDeleted(
+        this, parent->id(), index, node.release());
+  }
 }
 
 void BookmarkModel::RemoveNodeFromInternalMaps(BookmarkNode* node) {
@@ -909,11 +923,9 @@ BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
   if (store_.get())
     store_->ScheduleSave();
 
-  if (node->type() == BookmarkNode::URL) {
+  {
     base::AutoLock url_lock(url_lock_);
     AddNodeToInternalMaps(node);
-  } else {
-    index_->Add(node);
   }
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
@@ -923,9 +935,13 @@ BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
 }
 
 void BookmarkModel::AddNodeToInternalMaps(BookmarkNode* node) {
-  index_->Add(node);
   url_lock_.AssertAcquired();
-  nodes_ordered_by_url_set_.insert(node);
+  if (node->is_url()) {
+    index_->Add(node);
+    nodes_ordered_by_url_set_.insert(node);
+  }
+  for (int i = 0; i < node->child_count(); ++i)
+    AddNodeToInternalMaps(node->GetChild(i));
 }
 
 bool BookmarkModel::IsValidIndex(const BookmarkNode* parent,