Eliminate Bookmarks dependence to bookmark_undo_utils.h.

Introduced a mechanism through BookmarkModelObserver to inform the
bookmark undo service to group a set of bookmark changes instead of an
explicit call.

This removes the dependence to bookmark_undo_utils that was recently
introduced in
https://src.chromium.org/viewvc/chrome?view=rev&revision=241973

BUG=126092
TEST=Added a unit_tests test to ensure that the BookmarkModelObserver is notified.

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

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

chrome/browser/bookmarks/DEPS

@@ -9,7 +9,6 @@ include_rules = [
   "+chrome/browser/chrome_notification_types.h",
   "+chrome/browser/undo/bookmark_undo_service.h",
   "+chrome/browser/undo/bookmark_undo_service_factory.h",
-  "+chrome/browser/undo/bookmark_undo_utils.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.cc

@@ -284,6 +284,16 @@ void BookmarkModel::EndExtensiveChanges() {
   }
 }
 
+void BookmarkModel::BeginGroupedChanges() {
+  FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
+                    GroupedBookmarkChangesBeginning(this));
+}
+
+void BookmarkModel::EndGroupedChanges() {
+  FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
+                    GroupedBookmarkChangesEnded(this));
+}
+
 void BookmarkModel::Remove(const BookmarkNode* parent, int index) {
   if (!loaded_ || !IsValidIndex(parent, index, false) || is_root_node(parent)) {
     NOTREACHED();

chrome/browser/bookmarks/bookmark_model.h

@@ -34,6 +34,7 @@ class BookmarkModelObserver;
 class BookmarkStorage;
 struct BookmarkTitleMatch;
 class Profile;
+class ScopedGroupBookmarkActions;
 
 namespace base {
 class SequencedTaskRunner;
@@ -439,6 +440,7 @@ class BookmarkModel : public content::NotificationObserver,
   friend class BookmarkCodecTest;
   friend class BookmarkModelTest;
   friend class BookmarkStorage;
+  friend class ScopedGroupBookmarkActions;
 
   // Used to order BookmarkNodes by URL.
   class NodeURLComparator {
@@ -514,6 +516,11 @@ class BookmarkModel : public content::NotificationObserver,
   // If we're waiting on a favicon for node, the load request is canceled.
   void CancelPendingFaviconLoadRequests(BookmarkNode* node);
 
+  // Notifies the observers that a set of changes initiated by a single user
+  // action is about to happen and has completed.
+  void BeginGroupedChanges();
+  void EndGroupedChanges();
+
   // content::NotificationObserver:
   virtual void Observe(int type,
                        const content::NotificationSource& source,

chrome/browser/bookmarks/bookmark_model_observer.h

@@ -99,6 +99,15 @@ class BookmarkModelObserver {
   // Invoked when all non-permanent bookmark nodes have been removed.
   virtual void BookmarkAllNodesRemoved(BookmarkModel* model) = 0;
 
+  // Invoked before a set of model changes that is initiated by a single user
+  // action. For example, this is called a single time when pasting from the
+  // clipboard before each pasted bookmark is added to the bookmark model.
+  virtual void GroupedBookmarkChangesBeginning(BookmarkModel* model) {}
+
+  // Invoked after a set of model changes triggered by a single user action has
+  // ended.
+  virtual void GroupedBookmarkChangesEnded(BookmarkModel* model) {}
+
  protected:
   virtual ~BookmarkModelObserver() {}
 };

chrome/browser/bookmarks/bookmark_utils.cc

@@ -15,10 +15,11 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/bookmarks/scoped_group_bookmark_actions.h"
 #include "chrome/browser/history/query_parser.h"
 #include "chrome/browser/undo/bookmark_undo_service.h"
 #include "chrome/browser/undo/bookmark_undo_service_factory.h"
-#include "chrome/browser/undo/bookmark_undo_utils.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/user_metrics.h"
@@ -149,7 +150,7 @@ void CopyToClipboard(BookmarkModel* model,
 
   if (remove_nodes) {
 #if !defined(OS_ANDROID)
-    ScopedGroupBookmarkActions group_cut(model->profile());
+    ScopedGroupBookmarkActions group_cut(model);
 #endif
     for (size_t i = 0; i < filtered_nodes.size(); ++i) {
       int index = filtered_nodes[i]->parent()->GetIndexOf(filtered_nodes[i]);
@@ -172,7 +173,7 @@ void PasteFromClipboard(BookmarkModel* model,
   if (index == -1)
     index = parent->child_count();
 #if !defined(OS_ANDROID)
-  ScopedGroupBookmarkActions group_paste(model->profile());
+  ScopedGroupBookmarkActions group_paste(model);
 #endif
   CloneBookmarkNode(model, bookmark_data.elements, parent, index, true);
 }

chrome/browser/bookmarks/bookmark_utils.h

@@ -13,6 +13,7 @@
 
 class BookmarkModel;
 class BookmarkNode;
+class Profile;
 
 namespace user_prefs {
 class PrefRegistrySyncable;

chrome/browser/bookmarks/bookmark_utils_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/bookmarks/base_bookmark_model_observer.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_node_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -20,16 +21,38 @@ using std::string;
 namespace bookmark_utils {
 namespace {
 
-class BookmarkUtilsTest : public ::testing::Test {
+class BookmarkUtilsTest : public testing::Test,
+                          public BaseBookmarkModelObserver {
  public:
-  BookmarkUtilsTest() {}
+  BookmarkUtilsTest()
+      : grouped_changes_beginning_count_(0), grouped_changes_ended_count_(0) {}
   virtual ~BookmarkUtilsTest() {}
 
   virtual void TearDown() OVERRIDE {
     ui::Clipboard::DestroyClipboardForCurrentThread();
   }
 
+  void ExpectGroupedChangeCount(int expected_beginning_count,
+                                int expected_ended_count) {
+    EXPECT_EQ(grouped_changes_beginning_count_, expected_beginning_count);
+    EXPECT_EQ(grouped_changes_ended_count_, expected_ended_count);
+  }
+
+  // BaseBookmarkModelObserver:
+  virtual void BookmarkModelChanged() OVERRIDE {}
+
+  virtual void GroupedBookmarkChangesBeginning(BookmarkModel* model) OVERRIDE {
+    ++grouped_changes_beginning_count_;
+  }
+
+  virtual void GroupedBookmarkChangesEnded(BookmarkModel* model) OVERRIDE {
+    ++grouped_changes_ended_count_;
+  }
+
  private:
+  int grouped_changes_beginning_count_;
+  int grouped_changes_ended_count_;
+
   // Clipboard requires a message loop.
   base::MessageLoopForUI loop_;
 
@@ -242,6 +265,31 @@ TEST_F(BookmarkUtilsTest, CopyPaste) {
   EXPECT_FALSE(CanPasteFromClipboard(model.bookmark_bar_node()));
 }
 
+TEST_F(BookmarkUtilsTest, CutToClipboard) {
+  BookmarkModel model(NULL);
+  model.AddObserver(this);
+
+  base::string16 title(ASCIIToUTF16("foo"));
+  GURL url("http://foo.com");
+  const BookmarkNode* n1 = model.AddURL(model.other_node(), 0, title, url);
+  const BookmarkNode* n2 = model.AddURL(model.other_node(), 1, title, url);
+
+  // Cut the nodes to the clipboard.
+  std::vector<const BookmarkNode*> nodes;
+  nodes.push_back(n1);
+  nodes.push_back(n2);
+  CopyToClipboard(&model, nodes, true);
+
+  // Make sure the nodes were removed.
+  EXPECT_EQ(0, model.other_node()->child_count());
+
+  // Make sure observers were notified the set of changes should be grouped.
+  ExpectGroupedChangeCount(1, 1);
+
+  // And make sure we can paste from the clipboard.
+  EXPECT_TRUE(CanPasteFromClipboard(model.other_node()));
+}
+
 TEST_F(BookmarkUtilsTest, GetParentForNewNodes) {
   BookmarkModel model(NULL);
   // This tests the case where selection contains one item and that item is a

chrome/browser/bookmarks/scoped_group_bookmark_actions.cc

+// Copyright 2014 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/bookmarks/scoped_group_bookmark_actions.h"
+
+#include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
+
+ScopedGroupBookmarkActions::ScopedGroupBookmarkActions(Profile* profile)
+    : model_(NULL) {
+  if (profile)
+    model_ = BookmarkModelFactory::GetForProfile(profile);
+  if (model_)
+    model_->BeginGroupedChanges();
+}
+
+ScopedGroupBookmarkActions::ScopedGroupBookmarkActions(BookmarkModel* model)
+    : model_(model) {
+  if (model_)
+    model_->BeginGroupedChanges();
+}
+
+ScopedGroupBookmarkActions::~ScopedGroupBookmarkActions() {
+  if (model_)
+    model_->EndGroupedChanges();
+}

chrome/browser/bookmarks/scoped_group_bookmark_actions.h

+// Copyright 2014 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_BOOKMARKS_SCOPED_GROUP_BOOKMARK_ACTIONS_H_
+#define CHROME_BROWSER_BOOKMARKS_SCOPED_GROUP_BOOKMARK_ACTIONS_H_
+
+#include "base/macros.h"
+
+class BookmarkModel;
+class Profile;
+
+// Scopes the grouping of a set of changes into one undoable action.
+class ScopedGroupBookmarkActions {
+ public:
+  explicit ScopedGroupBookmarkActions(Profile* profile);
+  explicit ScopedGroupBookmarkActions(BookmarkModel* model);
+  ~ScopedGroupBookmarkActions();
+
+ private:
+  BookmarkModel* model_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedGroupBookmarkActions);
+};
+
+#endif  // CHROME_BROWSER_BOOKMARKS_SCOPED_GROUP_BOOKMARK_ACTIONS_H_

chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/bookmarks/bookmark_node_data.h"
 #include "chrome/browser/bookmarks/bookmark_stats.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
+#include "chrome/browser/bookmarks/scoped_group_bookmark_actions.h"
 #include "chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api_constants.h"
 #include "chrome/browser/extensions/api/bookmarks/bookmark_api_constants.h"
 #include "chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h"
@@ -25,7 +26,6 @@
 #include "chrome/browser/ui/bookmarks/bookmark_drag_drop.h"
 #include "chrome/browser/undo/bookmark_undo_service.h"
 #include "chrome/browser/undo/bookmark_undo_service_factory.h"
-#include "chrome/browser/undo/bookmark_undo_utils.h"
 #include "chrome/common/extensions/api/bookmark_manager_private.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/user_prefs.h"

chrome/browser/ui/bookmarks/bookmark_drag_drop.cc

@@ -8,9 +8,9 @@
 #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/bookmarks/scoped_group_bookmark_actions.h"
 #include "chrome/browser/undo/bookmark_undo_service.h"
 #include "chrome/browser/undo/bookmark_undo_service_factory.h"
-#include "chrome/browser/undo/bookmark_undo_utils.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
 
 namespace chrome {

chrome/browser/undo/bookmark_undo_service.cc

@@ -8,10 +8,10 @@
 #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/bookmarks/scoped_group_bookmark_actions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/undo/bookmark_renumber_observer.h"
 #include "chrome/browser/undo/bookmark_undo_service_factory.h"
-#include "chrome/browser/undo/bookmark_undo_utils.h"
 #include "chrome/browser/undo/undo_operation.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -391,16 +391,6 @@ 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)->OnBookmarkRenumbered(old_id,
-                                                                   new_id);
-  }
-}
-
 void BookmarkUndoService::BookmarkModelLoaded(BookmarkModel* model,
                                               bool ids_reassigned) {
   undo_manager_.RemoveAllOperations();
@@ -466,3 +456,23 @@ void BookmarkUndoService::OnWillReorderBookmarkNode(BookmarkModel* model,
   scoped_ptr<UndoOperation> op(new BookmarkReorderOperation(profile_, node));
   undo_manager()->AddUndoOperation(op.Pass());
 }
+
+void BookmarkUndoService::GroupedBookmarkChangesBeginning(
+    BookmarkModel* model) {
+  undo_manager()->StartGroupingActions();
+}
+
+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 (std::vector<UndoOperation*>::iterator it = all_operations.begin();
+       it != all_operations.end(); ++it) {
+    static_cast<BookmarkUndoOperation*>(*it)->OnBookmarkRenumbered(old_id,
+                                                                   new_id);
+  }
+}
+

chrome/browser/undo/bookmark_undo_service.h

@@ -54,6 +54,8 @@ class BookmarkUndoService : public BaseBookmarkModelObserver,
                                         const BookmarkNode* node) OVERRIDE;
   virtual void OnWillReorderBookmarkNode(BookmarkModel* model,
                                          const BookmarkNode* node) OVERRIDE;
+  virtual void GroupedBookmarkChangesBeginning(BookmarkModel* model) OVERRIDE;
+  virtual void GroupedBookmarkChangesEnded(BookmarkModel* model) OVERRIDE;
 
   // BookmarkRenumberObserver:
   virtual void OnBookmarkRenumbered(int64 old_id, int64 new_id) OVERRIDE;

chrome/browser/undo/bookmark_undo_utils.cc

@@ -33,18 +33,3 @@ ScopedSuspendBookmarkUndo::~ScopedSuspendBookmarkUndo() {
   if (undo_manager)
     undo_manager->ResumeUndoTracking();
 }
-
-// ScopedGroupBookmarkActions -------------------------------------------------
-
-ScopedGroupBookmarkActions::ScopedGroupBookmarkActions(Profile* profile)
-    : profile_(profile) {
-  UndoManager *undo_manager = GetUndoManager(profile_);
-  if (undo_manager)
-    undo_manager->StartGroupingActions();
-}
-
-ScopedGroupBookmarkActions::~ScopedGroupBookmarkActions() {
-  UndoManager *undo_manager = GetUndoManager(profile_);
-  if (undo_manager)
-    undo_manager->EndGroupingActions();
-}

chrome/browser/undo/bookmark_undo_utils.h

@@ -24,18 +24,4 @@ class ScopedSuspendBookmarkUndo {
   DISALLOW_COPY_AND_ASSIGN(ScopedSuspendBookmarkUndo);
 };
 
-// ScopedGroupBookmarkActions -------------------------------------------------
-
-// Scopes the grouping of a set of changes into one undoable action.
-class ScopedGroupBookmarkActions {
- public:
-  explicit ScopedGroupBookmarkActions(Profile* profile);
-  ~ScopedGroupBookmarkActions();
-
- private:
-  Profile* profile_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedGroupBookmarkActions);
-};
-
 #endif  // CHROME_BROWSER_UNDO_BOOKMARK_UNDO_UTILS_H_

chrome/chrome_browser.gypi

@@ -333,6 +333,8 @@
         'browser/bookmarks/bookmark_utils.h',
         'browser/bookmarks/enhanced_bookmarks_features.cc',
         'browser/bookmarks/enhanced_bookmarks_features.h',
+        'browser/bookmarks/scoped_group_bookmark_actions.cc',
+        'browser/bookmarks/scoped_group_bookmark_actions.h',
         'browser/browser_about_handler.cc',
         'browser/browser_about_handler.h',
         'browser/browser_process.cc',
@@ -3264,6 +3266,8 @@
             'browser/accessibility/accessibility_extension_api_constants.cc',
             'browser/accessibility/invert_bubble_prefs.cc',
             'browser/auto_launch_trial.cc',
+            'browser/bookmarks/scoped_group_bookmark_actions.cc',
+            'browser/bookmarks/scoped_group_bookmark_actions.h',
             'browser/certificate_viewer.cc',
             'browser/chrome_browser_main_posix.cc',
             'browser/chrome_browser_main_posix.h',