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..94335377ac5d73d30c07f7e94839ebde180bccbf
--- /dev/null
+++ b/chrome/browser/undo/bookmark_undo_service.cc
@@ -0,0 +1,394 @@
+// 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 {
+
+// Helper to get a mutable bookmark node.
+BookmarkNode* AsMutable(const BookmarkNode* node) {

[sky, 2013/07/18 14:35:41]

There should be no reason for this. If there is, it means something is wrong.

[Tom Cassiotis, 2013/07/24 15:37:56]

AsMutable is used in one place to reorder the children in
BookmarkReorderOperation::Undo.

The need for AsMutable is due to the BookmarkNodes being reinserted in
TreeNode::SetChildren and require a non-const pointer (tree_node_model.h).

We either have to remove the const-ness in either
Undo/ReorderChildren/SetChildren or be able to retrieve non-const BookmarkNodes
from BookmarkModel::GetNodeByID.

BookmarkReorderOperation::Undo - const currently removed here

BookmarkModel::ReorderChildren - the vector would need to be rebuilt

TreeNode::SetChildren - TreeNode currently doesn't do any const-removing casts
and should not be introduced here

Changing or adding a version of GetNodeByID that returns a non-const
BookmarkNode* would be my preference.

However I am weary of making this sort of change as the BookmarkModel does not
provide any public methods that return non-const BookmarkNode* and all public
functions take const BookmarkNode*. The consistency makes me feel that it was no
accident and rather by design.

I have been curious about the approach as it seems odd to me. Functions
obviously modifying the nodes take const pointers (eg BookmarkModel::SetTitle,
SetURL). Is this done because in most cases const pointers will do and so
AsMutable does not need to  proliferate into other code?

Whatever the reason it eliminates a nice benefit of const-ness which is to have
the compiler inform you that the object that you want to be const may be
modified by a function.

In the end, I'm asking for guidance to resolve this point. :)

[sky, 2013/07/26 15:29:51]

I think BookmarkModel::ReorderChildren should take a vector of const
BookmarkNodes. The implementation can cast away the const'ness (as it does for
all the other methods). Sound good?

+  return const_cast<BookmarkNode*>(node);
+}
+
+// BookmarkUndoOperation ------------------------------------------------------
+
+// Base class for all bookmark related UndoOperations that facilitates access to
+// the BookmarkUndoService.
+class BookmarkUndoOperation : public UndoOperation {

[sky, 2013/07/18 14:35:41]

The style guide indicates you should separate declaration/definition even for
classes entirely in a .cc.

[Tom Cassiotis, 2013/07/24 15:37:56]

Done.

+ public:
+  explicit BookmarkUndoOperation(BookmarkUndoService* bookmark_undo)
+      : bookmark_undo_(bookmark_undo) {
+  }
+  virtual ~BookmarkUndoOperation() {}
+
+  BookmarkModel* GetBookmarkModel() {
+    return bookmark_undo_->GetBookmarkModel();
+  }
+  BookmarkIdMap* GetBookmarkIdMap() {
+    return &bookmark_undo_->bookmark_id_map_;
+  }
+ private:
+  BookmarkUndoService* bookmark_undo_;
+};

[sky, 2013/07/18 14:35:41]

DISALLOW... and newline between 39/40

[Tom Cassiotis, 2013/07/24 15:37:56]

The reason I did not include a the DISALLOW_ line is that the class should not
be instantiated.

With the change BookmarkUndoOperation contains a pure virtual function and so
the class cannot be instantiated.

Is a DISALLOW_ still needed?

+
+// BookmarkAddOperation -------------------------------------------------------
+
+// Handles the undo of the insertion of a bookmark or folder.
+class BookmarkAddOperation : public BookmarkUndoOperation {
+ public:
+  BookmarkAddOperation(BookmarkUndoService* bookmark_undo,
+                       const BookmarkNode* parent,
+                       int index);
+  virtual ~BookmarkAddOperation() {}
+
+  virtual void Undo() OVERRIDE;
+
+ private:
+  int64 parent_id_;

[sky, 2013/07/18 14:35:41]

Use const where you can in all these classes.

[Tom Cassiotis, 2013/07/24 15:37:56]

I have not seen any parameters or functions that I could make const.  Most of
parameters are POD and there aren't that many functions.

Fore example, Undo() could be const however it is part of the UndoObject
interface and I am not confident that all implementations will be able to adhere
to const-ness.

GetForProfile requires a non-const Profile so I'm restricted in making the
Profile const.

Am I missing something?

+  int index_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkAddOperation);
+};
+
+BookmarkAddOperation::BookmarkAddOperation(BookmarkUndoService* bookmark_undo,
+                                           const BookmarkNode* parent,
+                                           int index)
+  : BookmarkUndoOperation(bookmark_undo),
+    parent_id_(parent->id()),
+    index_(index) {
+}
+
+void BookmarkAddOperation::Undo() {
+  BookmarkModel* model = GetBookmarkModel();
+  BookmarkIdMap* id_map = GetBookmarkIdMap();
+  const BookmarkNode* parent =
+      model->GetNodeByID(id_map->GetCurrentId(parent_id_));
+  DCHECK(parent);
+
+  model->Remove(parent, index_);
+}
+
+// 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(BookmarkUndoService* undo,
+                          const BookmarkNode* parent,
+                          int old_index,
+                          const BookmarkNode* node);
+  virtual ~BookmarkRemoveOperation() {}
+
+  virtual void Undo() OVERRIDE;
+
+ private:
+  int64 parent_id_;
+  int old_index_;
+  BookmarkNodeData removed_node_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkRemoveOperation);
+};
+
+BookmarkRemoveOperation::BookmarkRemoveOperation(BookmarkUndoService* undo,
+                                                 const BookmarkNode* parent,
+                                                 int old_index,
+                                                 const BookmarkNode* node)
+  : BookmarkUndoOperation(undo),
+    parent_id_(parent->id()),
+    old_index_(old_index),
+    removed_node_(node) {
+}
+
+void BookmarkRemoveOperation::Undo() {
+  DCHECK(removed_node_.is_valid());
+  BookmarkModel* model = GetBookmarkModel();
+  BookmarkIdMap* id_map = GetBookmarkIdMap();
+  const BookmarkNode* parent =
+      model->GetNodeByID(id_map->GetCurrentId(parent_id_));
+  DCHECK(parent);
+
+  bookmark_utils::CloneBookmarkNode(model,
+                                    removed_node_.elements,
+                                    parent,
+                                    old_index_);
+  id_map->ExtractMappings(model, removed_node_.elements[0], parent, old_index_);
+}
+
+// BookmarkEditOperation ------------------------------------------------------
+
+// Handles the undo of the modification of a bookmark node.
+class BookmarkEditOperation : public BookmarkUndoOperation {
+ public:
+  BookmarkEditOperation(BookmarkUndoService* bookmark_undo,
+                        const BookmarkNode* node);
+  virtual ~BookmarkEditOperation() {}
+
+  virtual void Undo() OVERRIDE;
+
+ private:
+  int64 nodeId_;
+  BookmarkNodeData original_bookmark_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkEditOperation);
+};
+
+BookmarkEditOperation::BookmarkEditOperation(BookmarkUndoService* bookmark_undo,
+                                             const BookmarkNode* node)
+    : BookmarkUndoOperation(bookmark_undo),
+      nodeId_(node->id()),
+      original_bookmark_(node) {
+}
+
+void BookmarkEditOperation::Undo() {
+  DCHECK(original_bookmark_.is_valid());
+  BookmarkModel* model = GetBookmarkModel();
+  BookmarkIdMap* id_map = GetBookmarkIdMap();
+  const BookmarkNode* node = model->GetNodeByID(id_map->GetCurrentId(nodeId_));
+  DCHECK(node);
+
+  model->SetTitle(node, original_bookmark_.elements[0].title);
+  if (original_bookmark_.elements[0].is_url)
+     model->SetURL(node, original_bookmark_.elements[0].url);
+}
+
+// BookmarkMoveOperation ------------------------------------------------------
+
+// Handles the undo of a bookmark being moved to a new location.
+class BookmarkMoveOperation : public BookmarkUndoOperation {
+ public:
+  BookmarkMoveOperation(BookmarkUndoService* bookmark_undo,
+                        const BookmarkNode* old_parent,
+                        int old_index,
+                        const BookmarkNode* new_parent,
+                        int new_index);
+  virtual ~BookmarkMoveOperation() {}
+
+  virtual void Undo() OVERRIDE;
+
+ private:
+  int64 old_parent_id_;
+  int64 new_parent_id_;
+  int old_index_;
+  int new_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkMoveOperation);
+};
+
+BookmarkMoveOperation::BookmarkMoveOperation(BookmarkUndoService* bookmark_undo,
+                                             const BookmarkNode* old_parent,
+                                             int old_index,
+                                             const BookmarkNode* new_parent,
+                                             int new_index)
+    : BookmarkUndoOperation(bookmark_undo),
+      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();
+  BookmarkIdMap* id_map = GetBookmarkIdMap();
+  const BookmarkNode* old_parent =
+      model->GetNodeByID(id_map->GetCurrentId(old_parent_id_));
+  const BookmarkNode* new_parent =
+      model->GetNodeByID(id_map->GetCurrentId(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);
+}
+
+// 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(BookmarkUndoService* bookmark_undo,
+                           const BookmarkNode* parent);
+  virtual ~BookmarkReorderOperation();
+
+  virtual void Undo() OVERRIDE;
+
+ private:
+  int64 parent_id_;
+  std::vector<int64> ordered_bookmarks_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkReorderOperation);
+};
+
+BookmarkReorderOperation::BookmarkReorderOperation(BookmarkUndoService* undo,
+                                                   const BookmarkNode* parent)
+    : BookmarkUndoOperation(undo),
+      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();
+  BookmarkIdMap* id_map = GetBookmarkIdMap();
+  const BookmarkNode* parent =
+      model->GetNodeByID(id_map->GetCurrentId(parent_id_));
+  DCHECK(parent);
+
+  std::vector<BookmarkNode*> ordered_nodes;
+  for (size_t i = 0; i < ordered_bookmarks_.size(); ++i) {
+    const BookmarkNode* node =
+        model->GetNodeByID(id_map->GetCurrentId(ordered_bookmarks_[i]));
+    ordered_nodes.push_back(AsMutable(node));
+  }
+
+  model->ReorderChildren(parent, ordered_nodes);
+}
+
+}  // namespace
+
+// BookmarkIdMap --------------------------------------------------------------
+
+BookmarkIdMap::BookmarkIdMap() {
+}
+
+BookmarkIdMap::~BookmarkIdMap() {
+}
+
+int64 BookmarkIdMap::GetCurrentId(int64 oldIndex) {
+  int64 currentIndex = oldIndex;
+
+  std::map<int64, int64>::const_iterator it;
+  while ((it = identifier_map_.find(currentIndex)) != identifier_map_.end())
+    currentIndex = it->second;
+  return currentIndex;
+}
+
+void BookmarkIdMap::AddMapping(int64 old_index, int64 new_index) {
+  identifier_map_[old_index] = new_index;
+}
+
+void BookmarkIdMap::ExtractMappings(BookmarkModel* model,
+                                    const BookmarkNodeData::Element& element,
+                                    const BookmarkNode* parent,
+                                    int index_added_at) {
+  const BookmarkNode* node = parent->GetChild(index_added_at);
+  if (element.id() != node->id())
+    AddMapping(element.id(), node->id());
+  if (!element.is_url) {
+    for (int i = 0; i < static_cast<int>(element.children.size()); ++i)
+      ExtractMappings(model, element.children[i], node, 0);
+  }
+}
+
+// BookmarkUndoService --------------------------------------------------------
+
+BookmarkUndoService::BookmarkUndoService(Profile* profile)
+    : profile_(profile),
+      model_(NULL) {
+  BookmarkModelFactory::GetForProfile(profile_)->AddObserver(this);
+}
+
+BookmarkUndoService::BookmarkUndoService(BookmarkModel* model)
+    : profile_(NULL),
+      model_(model) {
+  model_->AddObserver(this);
+}
+
+BookmarkUndoService::~BookmarkUndoService() {
+  GetBookmarkModel()->RemoveObserver(this);
+}
+
+BookmarkModel* BookmarkUndoService::GetBookmarkModel() {
+  return profile_ ? BookmarkModelFactory::GetForProfile(profile_) : model_;
+}
+
+void BookmarkUndoService::Loaded(BookmarkModel* model, bool ids_reassigned) {
+  undo_manager_.Reset();
+}
+
+void BookmarkUndoService::BookmarkModelBeingDeleted(BookmarkModel* model) {
+  undo_manager_.Reset();
+}
+
+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(this,
+                                                         old_parent,
+                                                         old_index,
+                                                         new_parent,
+                                                         new_index));
+  GetUndoManager()->AddUndoOperation(op.Pass());
+}
+
+void BookmarkUndoService::BookmarkNodeAdded(BookmarkModel* model,
+                                            const BookmarkNode* parent,
+                                            int index) {
+  scoped_ptr<UndoOperation> op(new BookmarkAddOperation(this, parent, index));
+  GetUndoManager()->AddUndoOperation(op.Pass());
+}
+
+void BookmarkUndoService::OnWillRemoveBookmarks(BookmarkModel* model,
+                                                const BookmarkNode* parent,
+                                                int old_index,
+                                                const BookmarkNode* node) {
+  scoped_ptr<UndoOperation> op(new BookmarkRemoveOperation(this,
+                                                           parent,
+                                                           old_index,
+                                                           node));
+  GetUndoManager()->AddUndoOperation(op.Pass());
+}
+
+void BookmarkUndoService::OnWillRemoveAllBookmarks(BookmarkModel* model) {
+  ScopedGroupingAction merge_removes(GetUndoManager());
+  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(this,
+          permanent_node, j, permanent_node->GetChild(j)));
+      GetUndoManager()->AddUndoOperation(op.Pass());
+    }
+  }
+}
+
+
+void BookmarkUndoService::OnWillChangeBookmarkNode(BookmarkModel* model,
+                                                   const BookmarkNode* node) {
+  scoped_ptr<UndoOperation> op(new BookmarkEditOperation(this, node));
+  GetUndoManager()->AddUndoOperation(op.Pass());
+}
+
+void BookmarkUndoService::OnWillReorderBookmarkNode(BookmarkModel* model,
+                                                    const BookmarkNode* node) {
+  scoped_ptr<UndoOperation> op(new BookmarkReorderOperation(this, node));
+  GetUndoManager()->AddUndoOperation(op.Pass());
+}