Multiple level undo/redo of bookmarks available through Bookmark Bar.

The BookmarkUndoService is now being constructed and is observing
user initiated changes to the bookmark model.

Undo and Redo of bookmark changes can be triggered from the
Bookmark Bar context menu. This functionality is behind the switch
enable-bookmark-undo.

Still outstanding is the multi-level undo to be accessible from
the Bookmark Manager.

BUG=126092

Review URL: https://codereview.chromium.org/70233015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@239292 0039d316-1c4b-4281-b951-d872f2087c98

chrome/app/bookmarks_strings.grdp

@@ -104,6 +104,12 @@
     <message name="IDS_BOOKMARK_BAR_RENAME_FOLDER" desc="Title of the bookmark context menu item that brings up a dialog to rename a folder">
       &amp;Rename...
     </message>
+    <message name="IDS_BOOKMARK_BAR_UNDO" desc="Menu title for undoing a bookmark operation">
+      &amp;Undo
+    </message>
+    <message name="IDS_BOOKMARK_BAR_REDO" desc="Menu title for redoing a bookmark operation">
+      &amp;Redo
+    </message>
     <message name="IDS_BOOKMARK_BAR_REMOVE" desc="Menu title for removing/unstarring a bookmark">
       &amp;Delete
     </message>
@@ -144,6 +150,12 @@
     <message name="IDS_BOOKMARK_BAR_RENAME_FOLDER" desc="In Title Case: Title of the bookmark context menu item that brings up a dialog to rename a folder">
       &amp;Rename...
     </message>
+    <message name="IDS_BOOKMARK_BAR_UNDO" desc="Menu title for undoing a bookmark operation">
+       &amp;Undo
+    </message>
+    <message name="IDS_BOOKMARK_BAR_REDO" desc="Menu title for redoing a bookmark operation">
+       &amp;Redo
+    </message>
     <message name="IDS_BOOKMARK_BAR_REMOVE" desc="In Title Case: Menu title for removing/unstarring a bookmark">
       &amp;Delete
     </message>

chrome/app/chrome_command_ids.h

@@ -336,6 +336,8 @@
 #define IDC_BOOKMARK_MANAGER 51009
 #define IDC_BOOKMARK_BAR_ALWAYS_SHOW 51010
 #define IDC_BOOKMARK_BAR_SHOW_APPS_SHORTCUT 51011
+#define IDC_BOOKMARK_BAR_UNDO 51012
+#define IDC_BOOKMARK_BAR_REDO 51013
 
 // Context menu items in the status tray
 #define IDC_STATUS_TRAY_KEEP_CHROME_RUNNING_IN_BACKGROUND 51100

chrome/browser/bookmarks/DEPS

@@ -7,6 +7,8 @@ include_rules = [
   "+chrome/browser/bookmarks",
   "+chrome/browser/favicon",
   "+chrome/browser/chrome_notification_types.h",
+  "+chrome/browser/undo/bookmark_undo_service.h",
+  "+chrome/browser/undo/bookmark_undo_service_factory.h",
 
   # TODO(tfarina): Bring this list to zero. crbug.com/144783
   # Do not add to the list of temporarily-allowed dependencies below,

chrome/browser/bookmarks/bookmark_model_factory.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 
+#include "base/command_line.h"
 #include "base/deferred_sequenced_task_runner.h"
 #include "base/memory/singleton.h"
 #include "base/values.h"
@@ -12,6 +13,9 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/startup_task_runner_service.h"
 #include "chrome/browser/profiles/startup_task_runner_service_factory.h"
+#include "chrome/browser/undo/bookmark_undo_service.h"
+#include "chrome/browser/undo/bookmark_undo_service_factory.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
 #include "components/user_prefs/pref_registry_syncable.h"
@@ -46,6 +50,13 @@ BrowserContextKeyedService* BookmarkModelFactory::BuildServiceInstanceFor(
   BookmarkModel* bookmark_model = new BookmarkModel(profile);
   bookmark_model->Load(StartupTaskRunnerServiceFactory::GetForProfile(profile)->
       GetBookmarkTaskRunner());
+#if !defined(OS_ANDROID)
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+     switches::kEnableBookmarkUndo)) {
+    bookmark_model->AddObserver(
+        BookmarkUndoServiceFactory::GetForProfile(profile));
+  }
+#endif  // !defined(OS_ANDROID)
   return bookmark_model;
 }
 

chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

@@ -50,6 +50,7 @@
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
 #include "chrome/browser/ui/tabs/pinned_tab_service_factory.h"
 #include "chrome/browser/ui/webui/ntp/ntp_resource_cache_factory.h"
+#include "chrome/browser/undo/bookmark_undo_service_factory.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
 
 #if defined(ENABLE_EXTENSIONS)
@@ -203,6 +204,9 @@ EnsureBrowserContextKeyedServiceFactoriesBuilt() {
   BackgroundContentsServiceFactory::GetInstance();
 #endif
   BookmarkModelFactory::GetInstance();
+#if !defined(OS_ANDROID)
+  BookmarkUndoServiceFactory::GetInstance();
+#endif
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
   captive_portal::CaptivePortalServiceFactory::GetInstance();
 #endif

chrome/browser/sync/glue/bookmark_change_processor.cc

@@ -22,6 +22,9 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/undo/bookmark_undo_service.h"
+#include "chrome/browser/undo/bookmark_undo_service_factory.h"
+#include "chrome/browser/undo/undo_manager_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "sync/internal_api/public/change_record.h"
 #include "sync/internal_api/public/read_node.h"
@@ -504,6 +507,12 @@ void BookmarkChangeProcessor::ApplyChangesFromSyncModel(
   // changes.
   model->RemoveObserver(this);
 
+  // Changes made to the bookmark model due to sync should not be undoable.
+#if !defined(OS_ANDROID)
+  ScopedSuspendUndoTracking suspend_undo(
+      BookmarkUndoServiceFactory::GetForProfile(profile_)->undo_manager());
+#endif
+
   // Notify UI intensive observers of BookmarkModel that we are about to make
   // potentially significant changes to it, so the updates may be batched. For
   // example, on Mac, the bookmarks bar displays animations when bookmark items

chrome/browser/sync/glue/bookmark_model_associator.cc

@@ -18,6 +18,9 @@
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/glue/bookmark_change_processor.h"
+#include "chrome/browser/undo/bookmark_undo_service.h"
+#include "chrome/browser/undo/bookmark_undo_service_factory.h"
+#include "chrome/browser/undo/undo_manager_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "sync/api/sync_error.h"
 #include "sync/internal_api/public/delete_journal.h"
@@ -363,6 +366,12 @@ bool BookmarkModelAssociator::GetSyncIdForTaggedNode(const std::string& tag,
 syncer::SyncError BookmarkModelAssociator::AssociateModels(
     syncer::SyncMergeResult* local_merge_result,
     syncer::SyncMergeResult* syncer_merge_result) {
+  // Since any changes to the bookmark model made here are not user initiated,
+  // these change should not be undoable and so suspend the undo tracking.
+#if !defined(OS_ANDROID)
+  ScopedSuspendUndoTracking suspend_undo(
+      BookmarkUndoServiceFactory::GetForProfile(profile_)->undo_manager());
+#endif
   syncer::SyncError error = CheckModelSyncState(local_merge_result,
                                                 syncer_merge_result);
   if (error.IsSet())

chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h"
 
+#include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/app/chrome_command_ids.h"
@@ -17,6 +18,9 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/undo/bookmark_undo_service.h"
+#include "chrome/browser/undo/bookmark_undo_service_factory.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/user_metrics.h"
@@ -85,6 +89,11 @@ void BookmarkContextMenuController::BuildMenu() {
 
   AddSeparator();
   AddItem(IDC_BOOKMARK_BAR_REMOVE, IDS_BOOKMARK_BAR_REMOVE);
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBookmarkUndo)) {
+    AddItem(IDC_BOOKMARK_BAR_UNDO, IDS_BOOKMARK_BAR_UNDO);
+    AddItem(IDC_BOOKMARK_BAR_REDO, IDS_BOOKMARK_BAR_REDO);
+  }
 
   AddSeparator();
   AddItem(IDC_BOOKMARK_BAR_ADD_NEW_BOOKMARK, IDS_BOOKMARK_BAR_ADD_NEW_BOOKMARK);
@@ -167,6 +176,22 @@ void BookmarkContextMenuController::ExecuteCommand(int id, int event_flags) {
                                     BookmarkEditor::NO_TREE);
       break;
 
+    case IDC_BOOKMARK_BAR_UNDO: {
+      content::RecordAction(
+          UserMetricsAction("BookmarkBar_ContextMenu_Undo"));
+      BookmarkUndoServiceFactory::GetForProfile(profile_)->undo_manager()->
+          Undo();
+      break;
+    }
+
+    case IDC_BOOKMARK_BAR_REDO: {
+      content::RecordAction(
+          UserMetricsAction("BookmarkBar_ContextMenu_Redo"));
+      BookmarkUndoServiceFactory::GetForProfile(profile_)->undo_manager()->
+          Redo();
+      break;
+    }
+
     case IDC_BOOKMARK_BAR_REMOVE: {
       content::RecordAction(
           UserMetricsAction("BookmarkBar_ContextMenu_Remove"));
@@ -309,6 +334,16 @@ bool BookmarkContextMenuController::IsCommandIdEnabled(int command_id) const {
     case IDC_BOOKMARK_BAR_EDIT:
       return selection_.size() == 1 && !is_root_node && can_edit;
 
+    case IDC_BOOKMARK_BAR_UNDO:
+      return can_edit &&
+          BookmarkUndoServiceFactory::GetForProfile(profile_)->
+              undo_manager()->undo_count() > 0;
+
+    case IDC_BOOKMARK_BAR_REDO:
+      return can_edit &&
+          BookmarkUndoServiceFactory::GetForProfile(profile_)->
+              undo_manager()->redo_count() > 0;
+
     case IDC_BOOKMARK_BAR_REMOVE:
       return !selection_.empty() && !is_root_node && can_edit;
 

chrome/browser/undo/bookmark_undo_service.cc

@@ -333,7 +333,6 @@ void BookmarkReorderOperation::OnBookmarkRenumbered(int64 old_id,
 // BookmarkUndoService --------------------------------------------------------
 
 BookmarkUndoService::BookmarkUndoService(Profile* profile) : profile_(profile) {
-  BookmarkModelFactory::GetForProfile(profile_)->AddObserver(this);
 }
 
 BookmarkUndoService::~BookmarkUndoService() {

chrome/browser/undo/bookmark_undo_service_factory.cc

@@ -23,8 +23,8 @@ BookmarkUndoServiceFactory* BookmarkUndoServiceFactory::GetInstance() {
 
 BookmarkUndoServiceFactory::BookmarkUndoServiceFactory()
     : BrowserContextKeyedServiceFactory(
-          "BookmarkUndoService",
-          BrowserContextDependencyManager::GetInstance()) {
+        "BookmarkUndoService",
+        BrowserContextDependencyManager::GetInstance()) {
   DependsOn(BookmarkModelFactory::GetInstance());
 }
 

chrome/browser/undo/bookmark_undo_service_test.cc

@@ -55,6 +55,7 @@ void BookmarkUndoServiceTest::TearDown() {
 TEST_F(BookmarkUndoServiceTest, AddBookmark) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   const BookmarkNode* parent = model->other_node();
   model->AddURL(parent, 0, ASCIIToUTF16("foo"), GURL("http://www.bar.com"));
@@ -74,6 +75,7 @@ TEST_F(BookmarkUndoServiceTest, AddBookmark) {
 TEST_F(BookmarkUndoServiceTest, UndoBookmarkRemove) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   const BookmarkNode* parent = model->other_node();
   model->AddURL(parent, 0, ASCIIToUTF16("foo"), GURL("http://www.bar.com"));
@@ -105,6 +107,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkRemove) {
 TEST_F(BookmarkUndoServiceTest, UndoBookmarkGroupedAction) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   const BookmarkNode* n1 = model->AddURL(model->other_node(),
                                         0,
@@ -140,6 +143,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkGroupedAction) {
 TEST_F(BookmarkUndoServiceTest, UndoBookmarkMoveWithinFolder) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   const BookmarkNode* n1 = model->AddURL(model->other_node(),
                                         0,
@@ -172,6 +176,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkMoveWithinFolder) {
 TEST_F(BookmarkUndoServiceTest, UndoBookmarkMoveToOtherFolder) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   const BookmarkNode* n1 = model->AddURL(model->other_node(),
                                         0,
@@ -213,6 +218,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkMoveToOtherFolder) {
 TEST_F(BookmarkUndoServiceTest, UndoBookmarkRenameDelete) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   const BookmarkNode* f1 = model->AddFolder(model->other_node(),
                                            0,
@@ -266,6 +272,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkRenameDelete) {
 TEST_F(BookmarkUndoServiceTest, UndoBookmarkReorder) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   const BookmarkNode* parent = model->other_node();
   model->AddURL(parent, 0, ASCIIToUTF16("foo"), GURL("http://www.foo.com"));
@@ -306,6 +313,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkReorder) {
 TEST_F(BookmarkUndoServiceTest, UndoBookmarkRemoveAll) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   // Setup bookmarks in the Other Bookmarks and the Bookmark Bar.
   const BookmarkNode* new_folder;
@@ -345,6 +353,7 @@ TEST_F(BookmarkUndoServiceTest, UndoBookmarkRemoveAll) {
 TEST_F(BookmarkUndoServiceTest, TestUpperLimit) {
   BookmarkModel* model = GetModel();
   BookmarkUndoService* undo_service = GetUndoService();
+  model->AddObserver(undo_service);
 
   // This maximum is set in undo_manager.cc
   const size_t kMaxUndoGroups = 100;

chrome/common/chrome_switches.cc

@@ -579,6 +579,9 @@ const char kEnableBenchmarking[]            = "enable-benchmarking";
 // Enables client hints, which adds hints about browser state to HTTP requests.
 const char kEnableClientHints[]             = "enable-client-hints";
 
+// Enables the multi-level undo system for bookmarks.
+const char kEnableBookmarkUndo[]            = "enable-bookmark-undo";
+
 // This applies only when the process type is "service". Enables the Cloud
 // Print Proxy component within the service process.
 const char kEnableCloudPrintProxy[]         = "enable-cloud-print-proxy";

chrome/common/chrome_switches.h

@@ -166,6 +166,7 @@ extern const char kEnableAuthNegotiatePort[];
 extern const char kEnableAutologin[];
 extern const char kEnableBenchmarking[];
 extern const char kEnableClientHints[];
+extern const char kEnableBookmarkUndo[];
 extern const char kEnableCloudPrintProxy[];
 extern const char kEnableContacts[];
 extern const char kEnableDevToolsExperiments[];