Bookmark Undo service for multiple level undo/redo of bookmarks.

chrome/browser/undo/bookmark_undo_service.cc

Index: chrome/browser/undo/bookmark_undo_service.cc
diff --git a/chrome/browser/undo/bookmark_undo_service.cc b/chrome/browser/undo/bookmark_undo_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a1ad748532b34cb7cdbbea371f4919286144e975
--- /dev/null
+++ b/chrome/browser/undo/bookmark_undo_service.cc
@@ -0,0 +1,419 @@
+// Copyright 2013 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 "chrome/browser/undo/bookmark_undo_service.h"
+
+#include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/bookmarks/bookmark_node_data.h"
+#include "chrome/browser/bookmarks/bookmark_utils.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/undo/bookmark_undo_service_factory.h"
+#include "chrome/browser/undo/undo_manager_utils.h"
+#include "chrome/browser/undo/undo_operation.h"
+
+namespace {
+
+// BookmarkUndoOperation ------------------------------------------------------
+
+// Base class for all bookmark related UndoOperations that facilitates access to
+// the BookmarkUndoService.
+class BookmarkUndoOperation : public UndoOperation {
+ public:
+  explicit BookmarkUndoOperation(Profile* profile);
+  virtual ~BookmarkUndoOperation() {}
+
+  // If a bookmark node id is renumbered due to an undo operation, the
+  // BookmarkUndoOperation should be updated with the new id in order to
+  // properly locate the bookmark node during execution of Undo.
+  virtual void RenumberBookmarkId(int64 old_id, int64 new_id) = 0;

[sky, 2013/08/06 22:04:07]

Wouldn't BookmarkIdChanged be a better name for this?

[Tom Cassiotis, 2013/08/19 13:47:00]

I did use the 'verb' and 'event' form of the function purposely.

BookmarkUndoOperation has smaller set of responsibilities and
BookmarkUndoService knows about them so I thought the 'verb' form would be best.

While in BookmarkUndoService I used OnBookmarkRenumbered to reflect that the
class may have to do any number of things and it may be called from somewhere
where it does not have much knowledge of the class.

In the end, it was a "feels right" kind of decision. I do not have any concern
changing it if you have a strong preference for the alternative naming.

+
+  BookmarkModel* GetBookmarkModel() const;
+  BookmarkUndoService* GetBookmarkUndoService() const;
+
+ private:
+  Profile* profile_;
+};
+
+BookmarkUndoOperation::BookmarkUndoOperation(Profile* profile)
+    : profile_(profile) {
+}
+
+BookmarkModel* BookmarkUndoOperation::GetBookmarkModel() const {
+  return BookmarkModelFactory::GetForProfile(profile_);
+}
+
+BookmarkUndoService* BookmarkUndoOperation::GetBookmarkUndoService() const {
+  return BookmarkUndoServiceFactory::GetForProfile(profile_);
+}
+
+// BookmarkAddOperation -------------------------------------------------------
+
+// Handles the undo of the insertion of a bookmark or folder.
+class BookmarkAddOperation : public BookmarkUndoOperation {
+ public:
+  BookmarkAddOperation(Profile* profile, const BookmarkNode* parent, int index);
+  virtual ~BookmarkAddOperation() {}
+
+  // UndoOperation:
+  virtual void Undo() OVERRIDE;
+
+  // BookmarkUndoOperation:
+  virtual void RenumberBookmarkId(int64 old_id, int64 new_id) OVERRIDE;
+
+ private:
+  int64 parent_id_;
+  int index_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkAddOperation);
+};
+
+BookmarkAddOperation::BookmarkAddOperation(Profile* profile,
+                                           const BookmarkNode* parent,
+                                           int index)
+  : BookmarkUndoOperation(profile),
+    parent_id_(parent->id()),
+    index_(index) {
+}
+
+void BookmarkAddOperation::Undo() {
+  BookmarkModel* model = GetBookmarkModel();
+  const BookmarkNode* parent = model->GetNodeByID(parent_id_);
+  DCHECK(parent);
+
+  model->Remove(parent, index_);
+}
+
+void BookmarkAddOperation::RenumberBookmarkId(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,
+// the information for all descendant bookmark nodes is maintained.
+//
+// The BookmarkModel allows only single bookmark node to be removed.
+class BookmarkRemoveOperation : public BookmarkUndoOperation {
+ public:
+  BookmarkRemoveOperation(Profile* profile,
+                          const BookmarkNode* parent,
+                          int old_index,
+                          const BookmarkNode* node);
+  virtual ~BookmarkRemoveOperation() {}
+
+  // UndoOperation:
+  virtual void Undo() OVERRIDE;
+
+  // BookmarkUndoOperation:
+  virtual void RenumberBookmarkId(int64 old_id, int64 new_id) OVERRIDE;
+
+ private:
+  void GetNewBookmarkIds(const BookmarkNodeData::Element& element,

[sky, 2013/08/06 22:04:07]

This doesn't actually Get anything. Maybe this should be named UpdateBookmarkIds

[Tom Cassiotis, 2013/08/19 13:47:00]

Done.

+                         const BookmarkNode* parent,
+                         int index_added_at) const;
+
+  int64 parent_id_;
+  int old_index_;

[sky, 2013/08/06 22:04:07]

const?

[Tom Cassiotis, 2013/08/19 13:47:00]

Done.

+  BookmarkNodeData removed_node_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkRemoveOperation);
+};
+
+BookmarkRemoveOperation::BookmarkRemoveOperation(Profile* profile,
+                                                 const BookmarkNode* parent,
+                                                 int old_index,
+                                                 const BookmarkNode* node)
+  : BookmarkUndoOperation(profile),
+    parent_id_(parent->id()),
+    old_index_(old_index),
+    removed_node_(node) {
+}
+
+void BookmarkRemoveOperation::Undo() {
+  DCHECK(removed_node_.is_valid());
+  BookmarkModel* model = GetBookmarkModel();
+  const BookmarkNode* parent = model->GetNodeByID(parent_id_);
+  DCHECK(parent);
+
+  bookmark_utils::CloneBookmarkNode(model,

[sky, 2013/08/06 22:04:07]

One thing I'm not sure about is meta info. Do we need to persist that too?

[Tom Cassiotis, 2013/08/19 13:47:00]

Currently the only use for the meta info is for versioning for sync.

I cannot imagine a problem with syncing if we were to restore the version
number,  although that sync code is tricky and there may be some edge cases that
I have not considered.

On the other hand, not restoring the meta info works and is simpler. The undo of
a delete is treated exactly like a new bookmark being created which is a basic
sync operation. (Also, this is also how the current BMM "Undo Delete" behaves).

The risk is if the meta info is used for other purposes which would require that
it be persisted, but currently meta info is not saved to disk/synced
(BookmarkChangeProcessor::CreateBookmarkNode) so if that change were to happen
it would need to be dealt with at that time.

+                                    removed_node_.elements,
+                                    parent,
+                                    old_index_);
+  GetNewBookmarkIds(removed_node_.elements[0], parent, old_index_);
+}
+
+void BookmarkRemoveOperation::GetNewBookmarkIds(
+    const BookmarkNodeData::Element& element,
+    const BookmarkNode* parent,
+    int index_added_at) const {
+  BookmarkModel* model = GetBookmarkModel();
+  BookmarkUndoService* undo_service = GetBookmarkUndoService();
+
+  const BookmarkNode* node = parent->GetChild(index_added_at);
+  if (element.id() != node->id())
+    undo_service->OnBookmarkRenumbered(element.id(), node->id());
+
+  if (!element.is_url) {
+    for (int i = 0; i < static_cast<int>(element.children.size()); ++i)
+      GetNewBookmarkIds(element.children[i], node, 0);
+  }
+}
+
+void BookmarkRemoveOperation::RenumberBookmarkId(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(Profile* profile,
+                        const BookmarkNode* node);
+  virtual ~BookmarkEditOperation() {}
+
+  // UndoOperation:
+  virtual void Undo() OVERRIDE;
+
+  // BookmarkUndoOperation:
+  virtual void RenumberBookmarkId(int64 old_id, int64 new_id) OVERRIDE;
+
+ private:
+  int64 node_id_;
+  BookmarkNodeData original_bookmark_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkEditOperation);
+};
+
+BookmarkEditOperation::BookmarkEditOperation(Profile* profile,
+                                             const BookmarkNode* node)
+    : BookmarkUndoOperation(profile),
+      node_id_(node->id()),
+      original_bookmark_(node) {
+}
+
+void BookmarkEditOperation::Undo() {
+  DCHECK(original_bookmark_.is_valid());
+  BookmarkModel* model = GetBookmarkModel();
+  const BookmarkNode* node = model->GetNodeByID(node_id_);
+  DCHECK(node);
+
+  model->SetTitle(node, original_bookmark_.elements[0].title);
+  if (original_bookmark_.elements[0].is_url)

[sky, 2013/08/06 22:04:07]

Should this reset last modified too?

[Tom Cassiotis, 2013/08/19 13:47:00]

Did you mean date_folder_modified_?

If that property is not restored there will be some subtle ordering when search
results.

This point brought to mind that date_added_ is also not restored. That is also
not used extensively and the behavior of the current BMM undo delete does not
restore the date_added_ property however I think that it should.

So I think it would be best to restore both properties however I would like some
advice on this. I am re-using BookmarkNodeData to store the bookmark information
for delete and modification changes. This also lets me use
bookmark_utils::CloneBookmarkNode to restore deleted bookmarks.

I am unsure if it would be best to modify BookmarkNodeData and CloneBookmarkNode
to take into account date_folder_modified_ and date_added_ or have the the
bookmark undo system have it's own versions of this.

The sync system also has a version of bookmark info that it uses to persist and
transfer the data and if we were to have an undo structure that would make three
ways to store bookmark data.

[sky, 2013/08/19 15:38:00]

We don't want pasting to restore dates, only undo. Seems fine to make
BookmarkNodeData track date_last_modified, but only the undo code would actually
restore it.

[Tom Cassiotis, 2013/08/27 14:39:29]

Done.

+     model->SetURL(node, original_bookmark_.elements[0].url);
+}
+
+void BookmarkEditOperation::RenumberBookmarkId(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(Profile* profile,
+                        const BookmarkNode* old_parent,
+                        int old_index,
+                        const BookmarkNode* new_parent,
+                        int new_index);
+  virtual ~BookmarkMoveOperation() {}
+
+  // UndoOperation:
+  virtual void Undo() OVERRIDE;
+
+  // BookmarkUndoOperation:
+  virtual void RenumberBookmarkId(int64 old_id, int64 new_id) OVERRIDE;
+
+ private:
+  int64 old_parent_id_;
+  int64 new_parent_id_;
+  int old_index_;
+  int new_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkMoveOperation);
+};
+
+BookmarkMoveOperation::BookmarkMoveOperation(Profile* profile,
+                                             const BookmarkNode* old_parent,
+                                             int old_index,
+                                             const BookmarkNode* new_parent,
+                                             int new_index)
+    : BookmarkUndoOperation(profile),
+      old_parent_id_(old_parent->id()),
+      old_index_(old_index),
+      new_parent_id_(new_parent->id()),
+      new_index_(new_index) {
+}
+
+void BookmarkMoveOperation::Undo() {
+  BookmarkModel* model = GetBookmarkModel();
+  const BookmarkNode* old_parent = model->GetNodeByID(old_parent_id_);
+  const BookmarkNode* new_parent = model->GetNodeByID(new_parent_id_);
+  DCHECK(old_parent);
+  DCHECK(new_parent);
+
+  const BookmarkNode* node = new_parent->GetChild(new_index_);
+  int destination_index = old_index_;
+
+  // If the bookmark was moved up within the same parent then the destination
+  // index needs to be incremented since the old index did not account for the
+  // moved bookmark.
+  if (old_parent == new_parent && new_index_ < old_index_)
+    ++destination_index;
+
+  model->Move(node, old_parent, destination_index);
+}
+
+void BookmarkMoveOperation::RenumberBookmarkId(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
+// sorting a bookmark folder by name or the undo of that operation.  The change
+// of order is not recursive so only the order of the immediate children of the
+// folder need to be restored.
+class BookmarkReorderOperation : public BookmarkUndoOperation {
+ public:
+  BookmarkReorderOperation(Profile* profile,
+                           const BookmarkNode* parent);
+  virtual ~BookmarkReorderOperation();
+
+  // UndoOperation:
+  virtual void Undo() OVERRIDE;
+
+  // BookmarkUndoOperation:
+  virtual void RenumberBookmarkId(int64 old_id, int64 new_id) OVERRIDE;
+
+ private:
+  int64 parent_id_;
+  std::vector<int64> ordered_bookmarks_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkReorderOperation);
+};
+
+BookmarkReorderOperation::BookmarkReorderOperation(Profile* profile,
+                                                   const BookmarkNode* parent)
+    : BookmarkUndoOperation(profile),
+      parent_id_(parent->id()) {
+  ordered_bookmarks_.resize(parent->child_count());
+  for (int i = 0; i < parent->child_count(); ++i)
+    ordered_bookmarks_[i] = parent->GetChild(i)->id();
+}
+
+BookmarkReorderOperation::~BookmarkReorderOperation() {
+}
+
+void BookmarkReorderOperation::Undo() {
+  BookmarkModel* model = GetBookmarkModel();
+  const BookmarkNode* parent = model->GetNodeByID(parent_id_);
+  DCHECK(parent);
+
+  std::vector<const BookmarkNode*> ordered_nodes;
+  for (size_t i = 0; i < ordered_bookmarks_.size(); ++i)
+    ordered_nodes.push_back(model->GetNodeByID(ordered_bookmarks_[i]));
+
+  model->ReorderChildren(parent, ordered_nodes);
+}
+
+void BookmarkReorderOperation::RenumberBookmarkId(int64 old_id, int64 new_id) {
+  if (parent_id_ == old_id)

[sky, 2013/08/06 22:04:07]

Doesn't this need to check ordered_bookmarks_ ?

[Tom Cassiotis, 2013/08/19 13:47:00]

Done.

+    parent_id_ = new_id;
+}
+
+}  // namespace
+
+// BookmarkUndoService --------------------------------------------------------
+
+BookmarkUndoService::BookmarkUndoService(Profile* profile) : profile_(profile) {
+  BookmarkModelFactory::GetForProfile(profile_)->AddObserver(this);
+}
+
+BookmarkUndoService::~BookmarkUndoService() {
+  BookmarkModelFactory::GetForProfile(profile_)->RemoveObserver(this);
+}
+
+void BookmarkUndoService::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
+  std::vector<UndoOperation*> all_operations =
+      undo_manager()->GetAllUndoOperations();
+  for (std::vector<UndoOperation*>::iterator it = all_operations.begin();
+       it != all_operations.end(); ++it) {
+    static_cast<BookmarkUndoOperation*>(*it)->RenumberBookmarkId(old_id,
+                                                                 new_id);
+  }
+}
+
+void BookmarkUndoService::Loaded(BookmarkModel* model, bool ids_reassigned) {
+  undo_manager_.RemoveAllOperations();
+}
+
+void BookmarkUndoService::BookmarkModelBeingDeleted(BookmarkModel* model) {
+  undo_manager_.RemoveAllOperations();
+}
+
+void BookmarkUndoService::BookmarkNodeMoved(BookmarkModel* model,
+                                            const BookmarkNode* old_parent,
+                                            int old_index,
+                                            const BookmarkNode* new_parent,
+                                            int new_index) {
+  scoped_ptr<UndoOperation> op(new BookmarkMoveOperation(profile_,
+                                                         old_parent,
+                                                         old_index,
+                                                         new_parent,
+                                                         new_index));
+  undo_manager()->AddUndoOperation(op.Pass());
+}
+
+void BookmarkUndoService::BookmarkNodeAdded(BookmarkModel* model,
+                                            const BookmarkNode* parent,
+                                            int index) {
+  scoped_ptr<UndoOperation> op(new BookmarkAddOperation(profile_,
+                                                        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(profile_,
+                                                           parent,
+                                                           old_index,
+                                                           node));
+  undo_manager()->AddUndoOperation(op.Pass());
+}
+
+void BookmarkUndoService::OnWillRemoveAllBookmarks(BookmarkModel* model) {
+  ScopedGroupingAction merge_removes(undo_manager());
+  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(profile_,
+          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(profile_, node));
+  undo_manager()->AddUndoOperation(op.Pass());
+}
+
+void BookmarkUndoService::OnWillReorderBookmarkNode(BookmarkModel* model,
+                                                    const BookmarkNode* node) {
+  scoped_ptr<UndoOperation> op(new BookmarkReorderOperation(profile_, node));
+  undo_manager()->AddUndoOperation(op.Pass());
+}