Supporting undoing bookmark deletion without creating new ID.

components/undo/bookmark_undo_service.cc

Index: components/undo/bookmark_undo_service.cc
diff --git a/components/undo/bookmark_undo_service.cc b/components/undo/bookmark_undo_service.cc
index 28fd451d7b080d5f6fe434629ab07f14c538ee20..c3bdb7faaccd907699d68c2f60fd1b62cb984e09 100644
--- a/components/undo/bookmark_undo_service.cc
+++ b/components/undo/bookmark_undo_service.cc
@@ -6,15 +6,16 @@
 
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node_data.h"
+#include "components/bookmarks/browser/bookmark_undo_provider.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/bookmarks/browser/scoped_group_bookmark_actions.h"
-#include "components/undo/bookmark_renumber_observer.h"
 #include "components/undo/undo_operation.h"
 #include "grit/components_strings.h"
 
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 using bookmarks::BookmarkNodeData;
+using bookmarks::BookmarkUndoProvider;
 
 namespace {
 
@@ -22,24 +23,16 @@ namespace {
 
 // Base class for all bookmark related UndoOperations that facilitates access to
 // the BookmarkUndoService.
-class BookmarkUndoOperation : public UndoOperation,
-                              public BookmarkRenumberObserver {
+class BookmarkUndoOperation : public UndoOperation {
  public:
-  BookmarkUndoOperation(BookmarkModel* bookmark_model,
-                        BookmarkRenumberObserver* undo_renumber_observer)
-      : bookmark_model_(bookmark_model),
-        undo_renumber_observer_(undo_renumber_observer) {}
+  explicit BookmarkUndoOperation(BookmarkModel* bookmark_model)
+      : bookmark_model_(bookmark_model) {}
   ~BookmarkUndoOperation() override {}
 
   BookmarkModel* bookmark_model() { return bookmark_model_; }
 
-  BookmarkRenumberObserver* undo_renumber_observer() {
-    return undo_renumber_observer_;
-  }
-
  private:
   BookmarkModel* bookmark_model_;
-  BookmarkRenumberObserver* undo_renumber_observer_;
 };
 
 // BookmarkAddOperation -------------------------------------------------------
@@ -48,7 +41,6 @@ class BookmarkUndoOperation : public UndoOperation,
 class BookmarkAddOperation : public BookmarkUndoOperation {
  public:
   BookmarkAddOperation(BookmarkModel* bookmark_model,
-                       BookmarkRenumberObserver* undo_renumber_observer,
                        const BookmarkNode* parent,
                        int index);
   ~BookmarkAddOperation() override {}
@@ -58,9 +50,6 @@ class BookmarkAddOperation : public BookmarkUndoOperation {
   int GetUndoLabelId() const override;
   int GetRedoLabelId() const override;
 
-  // BookmarkRenumberObserver:
-  void OnBookmarkRenumbered(int64 old_id, int64 new_id) override;
-
  private:
   int64 parent_id_;
   const int index_;
@@ -70,10 +59,9 @@ class BookmarkAddOperation : public BookmarkUndoOperation {
 
 BookmarkAddOperation::BookmarkAddOperation(
     BookmarkModel* bookmark_model,
-    BookmarkRenumberObserver* undo_renumber_observer,
     const BookmarkNode* parent,
     int index)
-    : BookmarkUndoOperation(bookmark_model, undo_renumber_observer),
+    : BookmarkUndoOperation(bookmark_model),
       parent_id_(parent->id()),
       index_(index) {
 }
@@ -95,11 +83,6 @@ int BookmarkAddOperation::GetRedoLabelId() const {
   return IDS_BOOKMARK_BAR_REDO_DELETE;
 }
 
-void BookmarkAddOperation::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
-  if (parent_id_ == old_id)
-    parent_id_ = new_id;
-}
-
 // BookmarkRemoveOperation ----------------------------------------------------
 
 // Handles the undo of the deletion of a bookmark node. For a bookmark folder,
@@ -108,55 +91,51 @@ void BookmarkAddOperation::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
 // The BookmarkModel allows only single bookmark node to be removed.
 class BookmarkRemoveOperation : public BookmarkUndoOperation {
  public:
-  BookmarkRemoveOperation(BookmarkModel* bookmark_model,
-                          BookmarkRenumberObserver* undo_renumber_observer,
+  BookmarkRemoveOperation(BookmarkModel* model,
+                          BookmarkUndoProvider* undo_provider,
                           const BookmarkNode* parent,
-                          int old_index,
-                          const BookmarkNode* node);
-  ~BookmarkRemoveOperation() override {}
+                          int index,
+                          scoped_ptr<BookmarkNode> node);
+  ~BookmarkRemoveOperation() override;
 
   // UndoOperation:
   void Undo() override;
   int GetUndoLabelId() const override;
   int GetRedoLabelId() const override;
 
-  // BookmarkRenumberObserver:
-  void OnBookmarkRenumbered(int64 old_id, int64 new_id) override;
-
  private:
-  void UpdateBookmarkIds(const BookmarkNodeData::Element& element,
-                         const BookmarkNode* parent,
-                         int index_added_at);
-
-  int64 parent_id_;
-  const int old_index_;
-  BookmarkNodeData removed_node_;
+  BookmarkUndoProvider* undo_provider_;
+  int64_t parent_node_id_;

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

const on this and the next.

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

Done.

+  int index_;
+  scoped_ptr<BookmarkNode> node_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkRemoveOperation);
 };
 
 BookmarkRemoveOperation::BookmarkRemoveOperation(
-    BookmarkModel* bookmark_model,
-    BookmarkRenumberObserver* undo_renumber_observer,
+    BookmarkModel* model,
+    BookmarkUndoProvider* undo_provider,
     const BookmarkNode* parent,
-    int old_index,
-    const BookmarkNode* node)
-    : BookmarkUndoOperation(bookmark_model, undo_renumber_observer),
-      parent_id_(parent->id()),
-      old_index_(old_index),
-      removed_node_(node) {
+    int index,
+    scoped_ptr<BookmarkNode> node)
+    : BookmarkUndoOperation(model),
+      undo_provider_(undo_provider),
+      parent_node_id_(parent->id()),
+      index_(index),
+      node_(node.Pass()) {
+}
+
+BookmarkRemoveOperation::~BookmarkRemoveOperation() {
 }
 
 void BookmarkRemoveOperation::Undo() {
-  DCHECK(removed_node_.is_valid());
-  BookmarkModel* model = bookmark_model();
-  const BookmarkNode* parent =
-      bookmarks::GetBookmarkNodeByID(model, parent_id_);
-  DCHECK(parent);
+  DCHECK(node_.get());
 
-  bookmarks::CloneBookmarkNode(
-      model, removed_node_.elements, parent, old_index_, false);
-  UpdateBookmarkIds(removed_node_.elements[0], parent, old_index_);
+  const BookmarkNode* parent = bookmarks::GetBookmarkNodeByID(
+      bookmark_model(), parent_node_id_);
+  if (!parent)
+    return;
+  undo_provider_->RestoreRemovedNode(parent, index_, node_.Pass());
 }
 
 int BookmarkRemoveOperation::GetUndoLabelId() const {
@@ -167,31 +146,12 @@ int BookmarkRemoveOperation::GetRedoLabelId() const {
   return IDS_BOOKMARK_BAR_REDO_ADD;
 }
 
-void BookmarkRemoveOperation::UpdateBookmarkIds(
-    const BookmarkNodeData::Element& element,
-    const BookmarkNode* parent,
-    int index_added_at) {
-  const BookmarkNode* node = parent->GetChild(index_added_at);
-  if (element.id() != node->id())
-    undo_renumber_observer()->OnBookmarkRenumbered(element.id(), node->id());
-  if (!element.is_url) {
-    for (int i = 0; i < static_cast<int>(element.children.size()); ++i)
-      UpdateBookmarkIds(element.children[i], node, i);
-  }
-}
-
-void BookmarkRemoveOperation::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
-  if (parent_id_ == old_id)
-    parent_id_ = new_id;
-}
-
 // BookmarkEditOperation ------------------------------------------------------
 
 // Handles the undo of the modification of a bookmark node.
 class BookmarkEditOperation : public BookmarkUndoOperation {
  public:
   BookmarkEditOperation(BookmarkModel* bookmark_model,
-                        BookmarkRenumberObserver* undo_renumber_observer,
                         const BookmarkNode* node);
   ~BookmarkEditOperation() override {}
 
@@ -200,9 +160,6 @@ class BookmarkEditOperation : public BookmarkUndoOperation {
   int GetUndoLabelId() const override;
   int GetRedoLabelId() const override;
 
-  // BookmarkRenumberObserver:
-  void OnBookmarkRenumbered(int64 old_id, int64 new_id) override;
-
  private:
   int64 node_id_;
   BookmarkNodeData original_bookmark_;
@@ -212,9 +169,8 @@ class BookmarkEditOperation : public BookmarkUndoOperation {
 
 BookmarkEditOperation::BookmarkEditOperation(
     BookmarkModel* bookmark_model,
-    BookmarkRenumberObserver* undo_renumber_observer,
     const BookmarkNode* node)
-    : BookmarkUndoOperation(bookmark_model, undo_renumber_observer),
+    : BookmarkUndoOperation(bookmark_model),
       node_id_(node->id()),
       original_bookmark_(node) {
 }
@@ -238,18 +194,12 @@ int BookmarkEditOperation::GetRedoLabelId() const {
   return IDS_BOOKMARK_BAR_REDO_EDIT;
 }
 
-void BookmarkEditOperation::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
-  if (node_id_ == old_id)
-    node_id_ = new_id;
-}
-
 // BookmarkMoveOperation ------------------------------------------------------
 
 // Handles the undo of a bookmark being moved to a new location.
 class BookmarkMoveOperation : public BookmarkUndoOperation {
  public:
   BookmarkMoveOperation(BookmarkModel* bookmark_model,
-                        BookmarkRenumberObserver* undo_renumber_observer,
                         const BookmarkNode* old_parent,
                         int old_index,
                         const BookmarkNode* new_parent,
@@ -261,9 +211,6 @@ class BookmarkMoveOperation : public BookmarkUndoOperation {
   // UndoOperation:
   void Undo() override;
 
-  // BookmarkRenumberObserver:
-  void OnBookmarkRenumbered(int64 old_id, int64 new_id) override;
-
  private:
   int64 old_parent_id_;
   int64 new_parent_id_;
@@ -275,12 +222,11 @@ class BookmarkMoveOperation : public BookmarkUndoOperation {
 
 BookmarkMoveOperation::BookmarkMoveOperation(
     BookmarkModel* bookmark_model,
-    BookmarkRenumberObserver* undo_renumber_observer,
     const BookmarkNode* old_parent,
     int old_index,
     const BookmarkNode* new_parent,
     int new_index)
-    : BookmarkUndoOperation(bookmark_model, undo_renumber_observer),
+    : BookmarkUndoOperation(bookmark_model),
       old_parent_id_(old_parent->id()),
       new_parent_id_(new_parent->id()),
       old_index_(old_index),
@@ -316,13 +262,6 @@ int BookmarkMoveOperation::GetRedoLabelId() const {
   return IDS_BOOKMARK_BAR_REDO_MOVE;
 }
 
-void BookmarkMoveOperation::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
-  if (old_parent_id_ == old_id)
-    old_parent_id_ = new_id;
-  if (new_parent_id_ == old_id)
-    new_parent_id_ = new_id;
-}
-
 // BookmarkReorderOperation ---------------------------------------------------
 
 // Handle the undo of reordering of bookmarks that can happen as a result of
@@ -332,7 +271,6 @@ void BookmarkMoveOperation::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
 class BookmarkReorderOperation : public BookmarkUndoOperation {
  public:
   BookmarkReorderOperation(BookmarkModel* bookmark_model,
-                           BookmarkRenumberObserver* undo_renumber_observer,
                            const BookmarkNode* parent);
   ~BookmarkReorderOperation() override;
 
@@ -341,9 +279,6 @@ class BookmarkReorderOperation : public BookmarkUndoOperation {
   int GetUndoLabelId() const override;
   int GetRedoLabelId() const override;
 
-  // BookmarkRenumberObserver:
-  void OnBookmarkRenumbered(int64 old_id, int64 new_id) override;
-
  private:
   int64 parent_id_;
   std::vector<int64> ordered_bookmarks_;
@@ -353,9 +288,8 @@ class BookmarkReorderOperation : public BookmarkUndoOperation {
 
 BookmarkReorderOperation::BookmarkReorderOperation(
     BookmarkModel* bookmark_model,
-    BookmarkRenumberObserver* undo_renumber_observer,
     const BookmarkNode* parent)
-    : BookmarkUndoOperation(bookmark_model, undo_renumber_observer),
+    : BookmarkUndoOperation(bookmark_model),
       parent_id_(parent->id()) {
   ordered_bookmarks_.resize(parent->child_count());
   for (int i = 0; i < parent->child_count(); ++i)
@@ -388,32 +322,28 @@ int BookmarkReorderOperation::GetRedoLabelId() const {
   return IDS_BOOKMARK_BAR_REDO_REORDER;
 }
 
-void BookmarkReorderOperation::OnBookmarkRenumbered(int64 old_id,
-                                                    int64 new_id) {
-  if (parent_id_ == old_id)
-    parent_id_ = new_id;
-  for (size_t i = 0; i < ordered_bookmarks_.size(); ++i) {
-    if (ordered_bookmarks_[i] == old_id)
-      ordered_bookmarks_[i] = new_id;
-  }
-}
-
 }  // namespace
 
 // BookmarkUndoService --------------------------------------------------------
 
-BookmarkUndoService::BookmarkUndoService() : scoped_observer_(this) {
+BookmarkUndoService::BookmarkUndoService()
+    : model_(NULL), scoped_observer_(this) {

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

NULL->nullptr

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

Done.

 }
 
 BookmarkUndoService::~BookmarkUndoService() {
 }
 
 void BookmarkUndoService::Start(BookmarkModel* model) {
+  DCHECK(!model_);
+  model_ = model;
   scoped_observer_.Add(model);
+  model->set_undo_delegate(this);
 }
 
 void BookmarkUndoService::Shutdown() {
+  DCHECK(model_);
   scoped_observer_.RemoveAll();
+  model_->set_undo_delegate(nullptr);
 }
 
 void BookmarkUndoService::BookmarkModelLoaded(BookmarkModel* model,
@@ -431,7 +361,7 @@ void BookmarkUndoService::BookmarkNodeMoved(BookmarkModel* model,
                                             const BookmarkNode* new_parent,
                                             int new_index) {
   scoped_ptr<UndoOperation> op(new BookmarkMoveOperation(
-      model, this, old_parent, old_index, new_parent, new_index));
+      model, old_parent, old_index, new_parent, new_index));
   undo_manager()->AddUndoOperation(op.Pass());
 }
 
@@ -439,40 +369,19 @@ void BookmarkUndoService::BookmarkNodeAdded(BookmarkModel* model,
                                             const BookmarkNode* parent,
                                             int index) {
   scoped_ptr<UndoOperation> op(
-      new BookmarkAddOperation(model, this, parent, index));
+      new BookmarkAddOperation(model, parent, index));
   undo_manager()->AddUndoOperation(op.Pass());
 }
 
-void BookmarkUndoService::OnWillRemoveBookmarks(BookmarkModel* model,
-                                                const BookmarkNode* parent,
-                                                int old_index,
-                                                const BookmarkNode* node) {
-  scoped_ptr<UndoOperation> op(
-      new BookmarkRemoveOperation(model, this, parent, old_index, node));
-  undo_manager()->AddUndoOperation(op.Pass());
-}
-
-void BookmarkUndoService::OnWillRemoveAllUserBookmarks(BookmarkModel* model) {
-  bookmarks::ScopedGroupBookmarkActions merge_removes(model);
-  for (int i = 0; i < model->root_node()->child_count(); ++i) {
-    const BookmarkNode* permanent_node = model->root_node()->GetChild(i);
-    for (int j = permanent_node->child_count() - 1; j >= 0; --j) {
-      scoped_ptr<UndoOperation> op(new BookmarkRemoveOperation(
-          model, this, permanent_node, j, permanent_node->GetChild(j)));
-      undo_manager()->AddUndoOperation(op.Pass());
-    }
-  }
-}
-
 void BookmarkUndoService::OnWillChangeBookmarkNode(BookmarkModel* model,
                                                    const BookmarkNode* node) {
-  scoped_ptr<UndoOperation> op(new BookmarkEditOperation(model, this, node));
+  scoped_ptr<UndoOperation> op(new BookmarkEditOperation(model, node));
   undo_manager()->AddUndoOperation(op.Pass());
 }
 
 void BookmarkUndoService::OnWillReorderBookmarkNode(BookmarkModel* model,
                                                     const BookmarkNode* node) {
-  scoped_ptr<UndoOperation> op(new BookmarkReorderOperation(model, this, node));
+  scoped_ptr<UndoOperation> op(new BookmarkReorderOperation(model, node));
   undo_manager()->AddUndoOperation(op.Pass());
 }
 
@@ -485,11 +394,12 @@ void BookmarkUndoService::GroupedBookmarkChangesEnded(BookmarkModel* model) {
   undo_manager()->EndGroupingActions();
 }
 
-void BookmarkUndoService::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
-  std::vector<UndoOperation*> all_operations =
-      undo_manager()->GetAllUndoOperations();
-  for (UndoOperation* op : all_operations) {
-    static_cast<BookmarkUndoOperation*>(op)
-        ->OnBookmarkRenumbered(old_id, new_id);
-  }
+void BookmarkUndoService::OnBookmarkNodeDeleted(BookmarkModel* model,
+                                                BookmarkUndoProvider* provider,
+                                                const BookmarkNode* parent,
+                                                int index,
+                                                scoped_ptr<BookmarkNode> node) {
+  scoped_ptr<UndoOperation> op(
+      new BookmarkRemoveOperation(model, provider, parent, index, node.Pass()));
+  undo_manager()->AddUndoOperation(op.Pass());
 }