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..53ab9ab01c5d477ec9de224a91c98a8dfbd3bd1c 100644
--- a/components/undo/bookmark_undo_service.cc
+++ b/components/undo/bookmark_undo_service.cc
@@ -8,7 +8,6 @@
 #include "components/bookmarks/browser/bookmark_node_data.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"
 
@@ -22,24 +21,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 +39,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 +48,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 +57,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 +81,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,
@@ -109,54 +90,35 @@ void BookmarkAddOperation::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
 class BookmarkRemoveOperation : public BookmarkUndoOperation {
  public:
   BookmarkRemoveOperation(BookmarkModel* bookmark_model,
-                          BookmarkRenumberObserver* undo_renumber_observer,
-                          const BookmarkNode* parent,
-                          int old_index,
-                          const BookmarkNode* node);
-  ~BookmarkRemoveOperation() override {}
+                          int64_t node_id);
+  ~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_;
+  int64_t node_id_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkRemoveOperation);
 };
 
 BookmarkRemoveOperation::BookmarkRemoveOperation(
     BookmarkModel* bookmark_model,
-    BookmarkRenumberObserver* undo_renumber_observer,
-    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) {
+    int64_t node_id)
+    : BookmarkUndoOperation(bookmark_model),
+      node_id_(node_id) {
 }
 
-void BookmarkRemoveOperation::Undo() {
-  DCHECK(removed_node_.is_valid());
-  BookmarkModel* model = bookmark_model();
-  const BookmarkNode* parent =
-      bookmarks::GetBookmarkNodeByID(model, parent_id_);
-  DCHECK(parent);
+BookmarkRemoveOperation::~BookmarkRemoveOperation() {
+  if (node_id_ != -1)
+    bookmark_model()->DiscardUndoableRemovedNode(node_id_);
+}
 
-  bookmarks::CloneBookmarkNode(
-      model, removed_node_.elements, parent, old_index_, false);
-  UpdateBookmarkIds(removed_node_.elements[0], parent, old_index_);
+void BookmarkRemoveOperation::Undo() {
+  bookmark_model()->UndoRemove(node_id_);
+  node_id_ = -1;
 }
 
 int BookmarkRemoveOperation::GetUndoLabelId() const {
@@ -167,31 +129,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 +143,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 +152,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 +177,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 +194,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 +205,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 +245,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 +254,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 +262,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 +271,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 +305,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) {
 }
 
 BookmarkUndoService::~BookmarkUndoService() {
 }
 
 void BookmarkUndoService::Start(BookmarkModel* model) {
+  DCHECK(!model_);
+  model_ = model;
   scoped_observer_.Add(model);
+  model->EnableUndoableRemove();
 }
 
 void BookmarkUndoService::Shutdown() {
+  DCHECK(model_);
   scoped_observer_.RemoveAll();
+  model_->DisableUndoableRemove();
 }
 
 void BookmarkUndoService::BookmarkModelLoaded(BookmarkModel* model,
@@ -431,7 +344,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,7 +352,7 @@ 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());
 }
 
@@ -448,31 +361,31 @@ void BookmarkUndoService::OnWillRemoveBookmarks(BookmarkModel* model,
                                                 int old_index,
                                                 const BookmarkNode* node) {
   scoped_ptr<UndoOperation> op(
-      new BookmarkRemoveOperation(model, this, parent, old_index, node));
+      new BookmarkRemoveOperation(model, node->id()));
   undo_manager()->AddUndoOperation(op.Pass());
 }
 
 void BookmarkUndoService::OnWillRemoveAllUserBookmarks(BookmarkModel* model) {
-  bookmarks::ScopedGroupBookmarkActions merge_removes(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)));
+          model, 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());
 }
 
@@ -484,12 +397,3 @@ void BookmarkUndoService::GroupedBookmarkChangesBeginning(
 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);
-  }
-}