Supporting undoing bookmark deletion without creating new ID.

components/bookmarks/browser/bookmark_model.cc

 // Copyright 2014 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.
 
 #include "components/bookmarks/browser/bookmark_model.h"
 
 #include <algorithm>
 #include <functional>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/i18n/string_compare.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/string_util.h"
 #include "components/bookmarks/browser/bookmark_expanded_state_tracker.h"
 #include "components/bookmarks/browser/bookmark_index.h"
 #include "components/bookmarks/browser/bookmark_match.h"
 #include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "components/bookmarks/browser/bookmark_node_data.h"
 #include "components/bookmarks/browser/bookmark_storage.h"
+#include "components/bookmarks/browser/bookmark_undo_delegate.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/favicon_base/favicon_types.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/favicon_size.h"
 
 using base::Time;
 
 namespace bookmarks {
 
@@ skipping 63 matching lines @@
     : client_(client),
       loaded_(false),
       root_(GURL()),
       bookmark_bar_node_(NULL),
       other_node_(NULL),
       mobile_node_(NULL),
       next_node_id_(1),
       observers_(
           base::ObserverList<BookmarkModelObserver>::NOTIFY_EXISTING_ONLY),
       loaded_signal_(true, false),
-      extensive_changes_(0) {
+      extensive_changes_(0),
+      undo_delegate_(nullptr) {
   DCHECK(client_);
 }
 
 BookmarkModel::~BookmarkModel() {
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkModelBeingDeleted(this));
 
   if (store_.get()) {
     // The store maintains a reference back to us. We need to tell it we're gone
     // so that it doesn't try and invoke a method back on us again.
@@ skipping 75 matching lines @@
 void BookmarkModel::Remove(const BookmarkNode* node) {
   DCHECK(loaded_);
   DCHECK(node);
   DCHECK(!is_root_node(node));
   RemoveAndDeleteNode(AsMutable(node));
 }
 
 void BookmarkModel::RemoveAllUserBookmarks() {
   std::set<GURL> removed_urls;
   ScopedVector<BookmarkNode> removed_nodes;
+  std::vector<std::pair<const BookmarkNode*, int>> removed_node_data;
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     OnWillRemoveAllUserBookmarks(this));
 
   BeginExtensiveChanges();
   // 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.
   {
     base::AutoLock url_lock(url_lock_);
     for (int i = 0; i < root_.child_count(); ++i) {
       BookmarkNode* permanent_node = root_.GetChild(i);
 
       if (!client_->CanBeEditedByUser(permanent_node))
         continue;
 
       for (int j = permanent_node->child_count() - 1; j >= 0; --j) {
         BookmarkNode* child_node = permanent_node->GetChild(j);
+        RemoveNodeAndGetRemovedUrls(child_node, &removed_urls);
         removed_nodes.push_back(child_node);

[sky, 2015/10/08 15:20:00]

You shouldn't need both removed_nodes and removed_node_data, just
removed_node_data and make it contain everything you need.

[jianli, 2015/10/08 23:37:40]

Done.

-        RemoveNodeAndGetRemovedUrls(child_node, &removed_urls);
+        removed_node_data.push_back(std::make_pair(permanent_node, j));

[sky, 2015/10/08 15:20:00]

Please add test coverage here as permanent_node is wrong, you want child_node.

[jianli, 2015/10/08 23:37:40]

I've updated BookmarkModelTest.RemoveAllUserBookmarks to validate no permanent
nodes have been removed.

[sky, 2015/10/09 16:01:19]

Also add coverage that the undodelegate is in fact passed the correct nodes.

[jianli, 2015/10/09 21:51:16]

Done.

       }
     }
   }
   EndExtensiveChanges();
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkAllUserNodesRemoved(this, removed_urls));
+
+  if (undo_delegate_) {

[sky, 2015/10/08 15:20:00]

Again, always have an undo_delegate_ so that it always take ownership.
Initialize with an empty implementation.

[jianli, 2015/10/08 23:37:40]

Done.

+    BeginGroupedChanges();
+    for (size_t i = 0; i < removed_nodes.size(); ++i) {
+      undo_delegate_->OnBookmarkNodeDeleted(
+          this, this, removed_node_data[i].first, removed_node_data[i].second,
+          scoped_ptr<BookmarkNode>(removed_nodes[i]));
+    }
+    EndGroupedChanges();
+    removed_nodes.weak_clear();
+  }
 }
 
 void BookmarkModel::Move(const BookmarkNode* node,
                          const BookmarkNode* new_parent,
                          int index) {
   if (!loaded_ || !node || !IsValidIndex(new_parent, index, true) ||
       is_root_node(new_parent) || is_permanent_node(node)) {
     NOTREACHED();
     return;
   }
@@ skipping 469 matching lines @@
     case BookmarkNode::OTHER_NODE:
       return other_node_;
     case BookmarkNode::MOBILE:
       return mobile_node_;
     default:
       NOTREACHED();
       return NULL;
   }
 }
 
+void BookmarkModel::RestoreRemovedNode(const BookmarkNode* parent,
+                                       int index,
+                                       scoped_ptr<BookmarkNode> scoped_node) {
+  BookmarkNode* node = scoped_node.release();
+  AsMutable(parent)->Add(node, index);

[sky, 2015/10/08 15:20:00]

Use AddNode for the common parts.

[jianli, 2015/10/08 23:37:40]

Done.

+
+  if (store_.get())
+    store_->ScheduleSave();
+
+  {
+    base::AutoLock url_lock(url_lock_);
+    AddNodeToInternalMaps(node);
+  }
+
+  // We might be restoring a folder node that have already contained a set of
+  // child nodes. We need to notify all of them.
+  FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
+                    BookmarkNodeAdded(this, parent, index));
+  NotifyNodeAddedForAllDescendents(node);
+}
+
+void BookmarkModel::NotifyNodeAddedForAllDescendents(const BookmarkNode* node) {
+  for (int i = 0; i < node->child_count(); ++i) {
+    FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
+                      BookmarkNodeAdded(this, node, i));
+    NotifyNodeAddedForAllDescendents(node->GetChild(i));
+  }
+}
+
 bool BookmarkModel::IsBookmarkedNoLock(const GURL& url) {
   BookmarkNode tmp_node(url);
   return (nodes_ordered_by_url_set_.find(&tmp_node) !=
           nodes_ordered_by_url_set_.end());
 }
 
 void BookmarkModel::RemoveNode(BookmarkNode* node,
                                std::set<GURL>* removed_urls) {
   if (!loaded_ || !node || is_permanent_node(node)) {
     NOTREACHED();
@@ skipping 121 matching lines @@
     RemoveNodeAndGetRemovedUrls(node.get(), &removed_urls);
   }
 
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(
       BookmarkModelObserver,
       observers_,
       BookmarkNodeRemoved(this, parent, index, node.get(), removed_urls));
+
+  if (undo_delegate_) {
+    undo_delegate_->OnBookmarkNodeDeleted(
+          this, this, parent, index, node.Pass());
+  }
 }
 
 void BookmarkModel::RemoveNodeFromInternalMaps(BookmarkNode* node) {
   index_->Remove(node);
   // NOTE: this is called in such a way that url_lock_ is already held. As
   // such, this doesn't explicitly grab the lock.
   url_lock_.AssertAcquired();
   NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(node);
   DCHECK(i != nodes_ordered_by_url_set_.end());
   // i points to the first node with the URL, advance until we find the
@@ skipping 30 matching lines @@
 }
 
 BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
                                      int index,
                                      BookmarkNode* node) {
   parent->Add(node, index);
 
   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_,
                     BookmarkNodeAdded(this, parent, index));
 
   return node;
 }
 
 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,
                                  int index,
                                  bool allow_end) {
   return (parent && parent->is_folder() &&
           (index >= 0 && (index < parent->child_count() ||
                           (allow_end && index == parent->child_count()))));
 }
 
@@ skipping 103 matching lines @@
   return scoped_ptr<BookmarkLoadDetails>(new BookmarkLoadDetails(
       bb_node,
       other_node,
       mobile_node,
       client_->GetLoadExtraNodesCallback(),
       new BookmarkIndex(client_, accept_languages),
       next_node_id_));
 }
 
 }  // namespace bookmarks