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

chrome/browser/undo/bookmark_undo_service.h

Index: chrome/browser/undo/bookmark_undo_service.h
diff --git a/chrome/browser/undo/bookmark_undo_service.h b/chrome/browser/undo/bookmark_undo_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..da64cd850a909a2af68f0b1f1a2e8cd738cf7d91
--- /dev/null
+++ b/chrome/browser/undo/bookmark_undo_service.h
@@ -0,0 +1,100 @@
+// 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.
+
+#ifndef CHROME_BROWSER_UNDO_BOOKMARK_UNDO_SERVICE_H_
+#define CHROME_BROWSER_UNDO_BOOKMARK_UNDO_SERVICE_H_
+
+#include <map>
+
+#include "chrome/browser/bookmarks/base_bookmark_model_observer.h"
+#include "chrome/browser/bookmarks/bookmark_node_data.h"
+#include "chrome/browser/undo/undo_manager.h"
+#include "components/browser_context_keyed_service/browser_context_keyed_service.h"
+
+class Profile;
+namespace {
+class BookmarkUndoOperation;
+}  // namespace
+
+// BookmarkIdMap -------------------------------------------------------
+
+// Keeps a record of the bookmark identifiers as they change as a result of
+// user actions and the undo system.
+//
+// For example, an existing bookmark is modified and then deleted, the undo of
+// the deletion would recreate the bookmark but with a different id.  In order
+// for the undo of the earlier modification to occur the new id should be
+// associated with the old id and be retrieved to undo the modification.
+class BookmarkIdMap {

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

I think I like notifying existing BookmarkUndoOperations of id changes. They can
then update indices immediately. That way there is no long lived map that is
never cleared around growing unbounded.

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

Since the UndoOperation and UndoService should know nothing about bookmarks and
bookmark ids, this change took a bit of effort.

To accomplish this I added UndoManager::ForEachUndoOperation that takes a
functor  and executes this on all UndoGroups and UndoOperations.  It would have
been cleaner if C++11 features but <functions> is not available to us at this
time.

The examples I saw of functors in the code did not try to use this for
isolation, the closest would be the event dispatchers.  I hope that the approach
is acceptable.

Done.

[Tom Cassiotis, 2013/07/25 13:02:03]

Alternatively, I can add a new function in UndoOperation that takes a new base
class 'UndoOperationModifier'.  In this way I can change the functor to derive
from UndoObjectModifier and pass that along.

It would eliminate the need for a template function and thus removing the inline
functions in UndoManager.

I think this is a more understandable approach, what do you think?

+ public:
+  BookmarkIdMap();
+  virtual ~BookmarkIdMap();
+
+  // Retrieve the valid bookmark id given a possibly old id.
+  int64 GetCurrentId(int64 old_index);
+
+  // Associate an old bookmark id with a new id.
+  void AddMapping(int64 old_index, int64 new_index);
+
+  // Recursively identify changed ids after bookmarks have been pasted.
+  void ExtractMappings(BookmarkModel* model,
+                       const BookmarkNodeData::Element& element,
+                       const BookmarkNode* parent,
+                       int index_added_at);
+ private:
+  std::map<int64, int64> identifier_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkIdMap);
+};
+
+// BookmarkUndoService --------------------------------------------------------
+
+// BookmarkUndoService is owned by the profile, and is responsible for observing
+// changes to the BookmarkModel in order to provide an undo for those changes.
+class BookmarkUndoService : public BaseBookmarkModelObserver,
+                            public BrowserContextKeyedService {
+ public:
+  explicit BookmarkUndoService(Profile* profile);
+  // This constructor is made available for tests.
+  explicit BookmarkUndoService(BookmarkModel* model);

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

Can you nuke this and use a TestingProfile?

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

TestingProfile simplified the code.  However, I am getting warnings about
temporary paths.  I did find that other tests with that use the TestingProfile
also report the same, so it is not my use of it but it may be specific to my
machine.

[656:9700:0724/091848:510020121:WARNING:file_util_win.cc(293)] Failed to get
temporary file name in C:\Users\USERNAME\AppData\Local\Temp\scoped_dir656_2526:
The directory name is invalid.

Should I do more digging?

Done.

+  virtual ~BookmarkUndoService();
+
+  UndoManager* GetUndoManager() { return &undo_manager_; }

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

undo_manager()

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

Done.

+
+ private:
+  friend class ::BookmarkUndoOperation;

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

I'm not a fan of this. I would rather see BookmarkUndoOperation take pointers to
the model and IdMap.

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

The removal of the bookmark id map made this easier.

Done.

+
+  BookmarkModel* GetBookmarkModel();
+
+  // BaseBookmarkModelObserver:
+  virtual void BookmarkModelChanged() OVERRIDE {}
+  virtual void Loaded(BookmarkModel* model, bool ids_reassigned) OVERRIDE;
+  virtual void BookmarkModelBeingDeleted(BookmarkModel* model) OVERRIDE;
+  virtual void BookmarkNodeMoved(BookmarkModel* model,
+                                 const BookmarkNode* old_parent,
+                                 int old_index,
+                                 const BookmarkNode* new_parent,
+                                 int new_index) OVERRIDE;
+  virtual void BookmarkNodeAdded(BookmarkModel* model,
+                                 const BookmarkNode* parent,
+                                 int index) OVERRIDE;
+  virtual void OnWillRemoveBookmarks(BookmarkModel* model,
+                                     const BookmarkNode* parent,
+                                     int old_index,
+                                     const BookmarkNode* node) OVERRIDE;
+  virtual void OnWillRemoveAllBookmarks(BookmarkModel* model) OVERRIDE;
+  virtual void OnWillChangeBookmarkNode(BookmarkModel* model,
+                                        const BookmarkNode* node) OVERRIDE;
+  virtual void OnWillReorderBookmarkNode(BookmarkModel* model,
+                                         const BookmarkNode* node) OVERRIDE;
+
+  Profile* profile_;
+  BookmarkModel* model_;
+  UndoManager undo_manager_;
+  BookmarkIdMap bookmark_id_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkUndoService);
+};
+
+#endif  // CHROME_BROWSER_UNDO_BOOKMARK_UNDO_SERVICE_H_