Eliminate Bookmarks dependence to bookmark_undo_utils.h.

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.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252832

[Tom Cassiotis, 2014-02-05 20:06:08]

Scott please take a look.

https://codereview.chromium.org/155913004/diff/30001/chrome/browser/undo/book...
File chrome/browser/undo/bookmark_undo_service.cc (left):

https://codereview.chromium.org/155913004/diff/30001/chrome/browser/undo/book...
chrome/browser/undo/bookmark_undo_service.cc:394: void
BookmarkUndoService::OnBookmarkRenumbered(int64 old_id, int64 new_id) {
This function was not in the same order as in the header file. I included this
small fix in this CL.

[sky, 2014-02-06 17:48:26]

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:297: void BeginGroupedChanges();
Can you make these private and the only way to use it is by way of a scoper
object that is friended? (ScopedGroupBookmarkActions (move it into bookmarks
directory)).

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model_observer.h (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model_observer.h:103: // is about to begin.
Give an example of what this entails, and one comment for the two should
suffice.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:39: void
AssertGroupedChangesObserverCount(int expected_beginning_count,
ExpectedGroupChanges...

[Tom Cassiotis, 2014-02-21 17:51:44]

Scott please take a look.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:297: void BeginGroupedChanges();
On 2014/02/06 17:48:27, sky wrote:
> Can you make these private and the only way to use it is by way of a scoper
> object that is friended? (ScopedGroupBookmarkActions (move it into bookmarks
> directory)).

Done.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model_observer.h (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model_observer.h:103: // is about to begin.
On 2014/02/06 17:48:27, sky wrote:
> Give an example of what this entails, and one comment for the two should
> suffice.

Done.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:39: void
AssertGroupedChangesObserverCount(int expected_beginning_count,
On 2014/02/06 17:48:27, sky wrote:
> ExpectedGroupChanges...

Added 'Assert' so it come out as AssertExpectedGroupChanges.  It is consistent
with the naming in bookmark_model_unittest.cc (AssertObserverCount).  Is that
okay?

Done.

[tfarina, 2014-02-21 18:46:47]

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:26: public:
please, sync your checkout.

also, please, run clang-format on it.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:39: void
AssertGroupedChangesObserverCount(int expected_beginning_count,
On 2014/02/21 17:51:45, Tom Cassiotis wrote:
> On 2014/02/06 17:48:27, sky wrote:
> > ExpectedGroupChanges...
> 
> Added 'Assert' so it come out as AssertExpectedGroupChanges.  It is consistent
> with the naming in bookmark_model_unittest.cc (AssertObserverCount).  Is that
> okay?
> 
I'd say Scott suggested Expected* because this method is using EXPECT macros
rather than ASSERT ones.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:282: ClearCounts();
I doubt you need this here. Because the test fixture will clean this up in the
initialization.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:297:
EXPECT_TRUE(CanPasteFromClipboard(model.bookmark_bar_node()));
this could be other_node() as well, no?

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:298: 
remove this empty line;

[sky, 2014-02-21 20:54:38]

LGTM

https://codereview.chromium.org/155913004/diff/370002/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right):

https://codereview.chromium.org/155913004/diff/370002/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:39: void
AssertExpectedGroupChanges(int expected_beginning_count,
Don't use assert in the name since it doesn't use assert.

[Tom Cassiotis, 2014-02-22 04:29:34]

I addressed the remaining issues.

I will delay committing until I am around for it to go through the trybots and
hit the tree.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right):

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:26: public:
On 2014/02/21 18:46:47, tfarina wrote:
> please, sync your checkout.
> 
> also, please, run clang-format on it.

Done.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:39: void
AssertGroupedChangesObserverCount(int expected_beginning_count,
On 2014/02/21 18:46:47, tfarina wrote:
> On 2014/02/21 17:51:45, Tom Cassiotis wrote:
> > On 2014/02/06 17:48:27, sky wrote:
> > > ExpectedGroupChanges...
> > 
> > Added 'Assert' so it come out as AssertExpectedGroupChanges.  It is
consistent
> > with the naming in bookmark_model_unittest.cc (AssertObserverCount).  Is
that
> > okay?
> > 
> I'd say Scott suggested Expected* because this method is using EXPECT macros
> rather than ASSERT ones.

Sorry for the misunderstanding. 

Done.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:282: ClearCounts();
On 2014/02/21 18:46:47, tfarina wrote:
> I doubt you need this here. Because the test fixture will clean this up in the
> initialization.

You are right.

Done.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:297:
EXPECT_TRUE(CanPasteFromClipboard(model.bookmark_bar_node()));
On 2014/02/21 18:46:47, tfarina wrote:
> this could be other_node() as well, no?

This function simply checks whether we can paste into the provided node. I
thought it would be better for the paste destination to be a different node than
where it was copied from.

Saying that it doesn't much matter since CanPasteFromClipboard only checks that
the passed in node is non-null.

Done.

https://codereview.chromium.org/155913004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:298: 
On 2014/02/21 18:46:47, tfarina wrote:
> remove this empty line;

Done.

[Tom Cassiotis, 2014-02-22 15:04:08]

The CQ bit was checked by tom.cassiotis@gmail.com

[commit-bot: I haz the power, 2014-02-22 15:04:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/155913004/790001

[tfarina, 2014-02-22 15:09:30]

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right):

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:29: 
where is my virtual destructor? Looks like you removed it.

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:40: //
BookmarkModelObserver:
BaseBookmarkModelObserver

[tfarina, 2014-02-22 15:09:35]

The CQ bit was unchecked by tfarina@chromium.org

[tfarina, 2014-02-22 15:11:21]

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_utils.h (right):

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_utils.h:113: class ScopedGroupBookmarkActions
{
Move this to a separate header file?

[Tom Cassiotis, 2014-02-22 20:54:02]

tfarina@ please take a look.

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_utils.h (right):

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_utils.h:113: class ScopedGroupBookmarkActions
{
On 2014/02/22 15:11:21, tfarina wrote:
> Move this to a separate header file?

Done.

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right):

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:29: 
On 2014/02/22 15:09:30, tfarina wrote:
> where is my virtual destructor? Looks like you removed it.

I made a mistake resolving the conflict.

Fixed.

https://codereview.chromium.org/155913004/diff/790001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_utils_unittest.cc:40: //
BookmarkModelObserver:
On 2014/02/22 15:09:30, tfarina wrote:
> BaseBookmarkModelObserver

Done.

[tfarina, 2014-02-22 22:25:47]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-02-22 22:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/155913004/1070001

[commit-bot: I haz the power, 2014-02-23 00:33:54]

Change committed as 252832

[Tom Cassiotis, 2014-02-23 13:21:01]

I haven't had a change of mine reverted and I'm not sure what the process of
fixing the patch and re-commit is.

I have uploaded a new patch that should address the problem with unit tests on
Android. (The undo code is excluded for Android so the undo specific
EXPECTations should also be excluded)

I can't run the Android unit tests locally and I guess the default trybots here
do not run BookmarkUtilsTest.* for Android.

I'll wait for advice before proceeding.