sync: Inject sync/'s dependency on invalidations

sync: Inject sync/'s dependency on invalidations

Defines an InvalidationInterface class and uses it to break any direct
dependencies from sync code on syncer::Invalidation.  Despite its name,
syncer::Invalidation should belong solely to the invalidations
component, which code in the sync/ directory should not depend on.

Changes the interface in the sync engine from copying
syncer::Invalidation to managing scoped_ptr<InvalidationInterface>.
This change in memory management was required to support the use of an
abstract interface.

Removes the DroppedInvaldiationTracker.  This class was previously only
used by sync.  The small benefit provided by this class is outweighed by
the amount of glue code it would take to maintain it.

Changes tests to conform to the new interface.  Adds some test-only
implementations of InvalidationInterface and some associated classes.

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

[rlarocque, 2014-06-18 17:54:05]

Here's the change to wrap sync' use of invalidations.  Since sync/ is not
allowed to depend on components/invalidation/, and the class currently known as
syncer::Invalidation is going to move to components/invalidation, we need to
wrap it before we hand it off to sync/.

Most of this is a rename, but there a few interface changes were necessary, too.
 See the CL description for details.

This does not remove all of the dependencies from sync/ on sync/notifier and
related classes, but it does remove most of them.  I'll clean up the rest in a
second CL.

Tim: Please review.

[tim (not reviewing), 2014-06-18 21:51:35]

https://codereview.chromium.org/322333004/diff/1/chrome/browser/sync/glue/inv...
File chrome/browser/sync/glue/invalidation_wrapper.h (right):

https://codereview.chromium.org/322333004/diff/1/chrome/browser/sync/glue/inv...
chrome/browser/sync/glue/invalidation_wrapper.h:9: #include
"sync/internal_api/public/base/invalidation_interface.h"
What is invalidation_interface? I don't see it in the tree or in this CL. 

Two files side-by-side called invalidation and invalidation_interface is
confusing. Is it more of an InvalidationBase <- Invalidation relationship?

[tim (not reviewing), 2014-06-18 21:52:58]

On 2014/06/18 21:51:35, timsteele wrote:
>
https://codereview.chromium.org/322333004/diff/1/chrome/browser/sync/glue/inv...
> File chrome/browser/sync/glue/invalidation_wrapper.h (right):
> 
>
https://codereview.chromium.org/322333004/diff/1/chrome/browser/sync/glue/inv...
> chrome/browser/sync/glue/invalidation_wrapper.h:9: #include
> "sync/internal_api/public/base/invalidation_interface.h"
> What is invalidation_interface? I don't see it in the tree or in this CL. 
> 
> Two files side-by-side called invalidation and invalidation_interface is
> confusing. Is it more of an InvalidationBase <- Invalidation relationship?

Ah, I read your blurb describing the CL in the request message but not the CL
description.
You probably need to add the file to the commit.  I feel like they don't belong
side by side... but I don't know enough about them yet.

[rlarocque, 2014-06-18 21:57:09]

On 2014/06/18 21:52:58, timsteele wrote:
> On 2014/06/18 21:51:35, timsteele wrote:
> >
>
https://codereview.chromium.org/322333004/diff/1/chrome/browser/sync/glue/inv...
> > File chrome/browser/sync/glue/invalidation_wrapper.h (right):
> > 
> >
>
https://codereview.chromium.org/322333004/diff/1/chrome/browser/sync/glue/inv...
> > chrome/browser/sync/glue/invalidation_wrapper.h:9: #include
> > "sync/internal_api/public/base/invalidation_interface.h"
> > What is invalidation_interface? I don't see it in the tree or in this CL. 
> > 
> > Two files side-by-side called invalidation and invalidation_interface is
> > confusing. Is it more of an InvalidationBase <- Invalidation relationship?
> 
> Ah, I read your blurb describing the CL in the request message but not the CL
> description.
> You probably need to add the file to the commit.  I feel like they don't
belong
> side by side... but I don't know enough about them yet.

Sorry about that.  The missing files should be available in the latest upload.

[tim (not reviewing), 2014-06-18 22:41:11]

https://codereview.chromium.org/322333004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/invalidation_wrapper.h (right):

https://codereview.chromium.org/322333004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/invalidation_wrapper.h:14: class InvalidationWrapper :
public syncer::InvalidationInterface {
s/Wrapper/Adapter.

But, could components/invalidator/Invalidation.h not implement
InvalidationInterface directly, and avoid the extra adapter?

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
File sync/internal_api/public/base/invalidation_interface.cc (right):

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
sync/internal_api/public/base/invalidation_interface.cc:9: bool
InvalidationInterface::LessThan(const InvalidationInterface& a,
nit - It's a bit weird to define a non-overrideable generic sounding comparator
like "less than" on a pure virtual interface as subclasses might introduce
dimensions unknown to InvalidationInterface that could impact it. Calling it
LessThanByVersion() or moving to a separate utility might help.  But we could
live with it.

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
File sync/internal_api/public/base/invalidation_interface.h (right):

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
sync/internal_api/public/base/invalidation_interface.h:55: #endif
nit - // SYNC_INTERNAL_API_PUBLIC_BASE...

https://codereview.chromium.org/322333004/diff/20001/sync/sessions/nudge_trac...
File sync/sessions/nudge_tracker.h (right):

https://codereview.chromium.org/322333004/diff/20001/sync/sessions/nudge_trac...
sync/sessions/nudge_tracker.h:62: scoped_ptr<InvalidationInterface>
invalidation);
include invalidation_interface.h

[rlarocque, 2014-06-19 00:50:19]

Here are some responses.  I hope to get around to fixing the compile errors and
uploading a new patch tomorrow.

https://codereview.chromium.org/322333004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/invalidation_wrapper.h (right):

https://codereview.chromium.org/322333004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/invalidation_wrapper.h:14: class InvalidationWrapper :
public syncer::InvalidationInterface {
On 2014/06/18 22:41:11, timsteele wrote:
> s/Wrapper/Adapter.

Will do.

> But, could components/invalidator/Invalidation.h not implement
> InvalidationInterface directly, and avoid the extra adapter?

If we did that, then components/invalidator/ would have to depend on
chrome/browser/sync/ (definitely wrong) or sync/ (which is one of the things I'd
like to fix as part of crbug.com/259559).

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
File sync/internal_api/public/base/invalidation_interface.cc (right):

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
sync/internal_api/public/base/invalidation_interface.cc:9: bool
InvalidationInterface::LessThan(const InvalidationInterface& a,
On 2014/06/18 22:41:11, timsteele wrote:
> nit - It's a bit weird to define a non-overrideable generic sounding
comparator
> like "less than" on a pure virtual interface as subclasses might introduce
> dimensions unknown to InvalidationInterface that could impact it. Calling it
> LessThanByVersion() or moving to a separate utility might help.  But we could
> live with it.

Will rename.

I didn't like this either.  The problem is that this can't be defined on the
derived classes, otherwise it's useless to the code that's only allowed to
interact with the wrapper.  The comparator can only rely on
InvalidationInterface.

I suppose I could have put it in a _util.h file or otherwise defined it out of
this class, but I'm not sure that's much better.  I guess it depends on how you
feel about static methods.

[rlarocque, 2014-06-20 20:00:13]

Patch updated.  PTAL.

https://codereview.chromium.org/322333004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/invalidation_wrapper.h (right):

https://codereview.chromium.org/322333004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/invalidation_wrapper.h:14: class InvalidationWrapper :
public syncer::InvalidationInterface {
On 2014/06/19 00:50:19, rlarocque wrote:
> On 2014/06/18 22:41:11, timsteele wrote:
> > s/Wrapper/Adapter.
> 
> Will do.
> 
> > But, could components/invalidator/Invalidation.h not implement
> > InvalidationInterface directly, and avoid the extra adapter?
> 
> If we did that, then components/invalidator/ would have to depend on
> chrome/browser/sync/ (definitely wrong) or sync/ (which is one of the things
I'd
> like to fix as part of crbug.com/259559).

Done.

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
File sync/internal_api/public/base/invalidation_interface.cc (right):

https://codereview.chromium.org/322333004/diff/20001/sync/internal_api/public...
sync/internal_api/public/base/invalidation_interface.cc:9: bool
InvalidationInterface::LessThan(const InvalidationInterface& a,
On 2014/06/19 00:50:19, rlarocque wrote:
> On 2014/06/18 22:41:11, timsteele wrote:
> > nit - It's a bit weird to define a non-overrideable generic sounding
> comparator
> > like "less than" on a pure virtual interface as subclasses might introduce
> > dimensions unknown to InvalidationInterface that could impact it. Calling it
> > LessThanByVersion() or moving to a separate utility might help.  But we
could
> > live with it.
> 
> Will rename.
> 
> I didn't like this either.  The problem is that this can't be defined on the
> derived classes, otherwise it's useless to the code that's only allowed to
> interact with the wrapper.  The comparator can only rely on
> InvalidationInterface.
> 
> I suppose I could have put it in a _util.h file or otherwise defined it out of
> this class, but I'm not sure that's much better.  I guess it depends on how
you
> feel about static methods.

Done.

[tim (not reviewing), 2014-06-20 20:21:33]

LGTM

[tim (not reviewing), 2014-06-20 20:47:54]

https://codereview.chromium.org/322333004/diff/60001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/322333004/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.cc:384: } else {
This may end up being a little less efficient since we're nesting loops, but I
doubt we're ever talking about large lists of invalidations here or in the
DataTypeTracker.  And otherwise I don't see any problems with the approach, I
like that the OnIncomingInvalidation API and implementation is simpler.

Still LGTM

[rlarocque, 2014-06-26 17:42:07]

Woo-hoo!  Finally got green trybots!

Getting this patch to compile on all platforms was more of a struggle than I
anticipated.  Here are some of the changes that required:

- I was (wrongly) using scoped_ptr in an std::list in data_type_tracker.h.  I
switch to using some stl_util.h helpers and a few manual delete statements.

- sync_client.cc needed its own implementation of InvalidationInterface.  It's
not allowed to reference chrome/browser/sync, so it couldn't use the one defined
there.

- I had to explicitly declare the Invalidation object's copy constructor.  The
Windows compile seems to dislike the default copy constructor.  My theory on
this is that the constructor ends up being compiled into several .o's through
the invalidation.h header, and the symbol is exported in each one because of the
SYNC_EXPORT declaration.

- Finally, there is a Windows header file that #defines 'interface' to 'struct'.
 I had to rename a parameter in sync_manager.h to work around it.

Tim, if you feel any of this merits a re-review, let me know.  Otherwise I'll
try to land this patch later today.

[tim (not reviewing), 2014-06-26 17:47:34]

Would base/memory/scoped_vector.h simplify your std::list usage?  Otherwise LG

[rlarocque, 2014-06-26 17:54:02]

On 2014/06/26 17:47:34, timsteele wrote:
> Would base/memory/scoped_vector.h simplify your std::list usage?  Otherwise LG

It might.  I had assumed that it didn't support inserting or removing from a
particular position.  Now that I look at the header file, I see that it might
support my needs.  I'll give it a shot.

[rlarocque, 2014-06-27 17:38:13]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-27 17:39:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/322333004/240001

[commit-bot: I haz the power, 2014-06-27 18:43:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-06-27 18:47:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-27 18:47:40]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[rlarocque, 2014-06-27 23:44:43]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-27 23:45:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/322333004/240001

[commit-bot: I haz the power, 2014-06-27 23:53:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-06-27 23:55:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-27 23:55:47]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[rlarocque, 2014-07-08 20:07:25]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-08 20:08:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/322333004/260001

[commit-bot: I haz the power, 2014-07-08 21:33:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-08 21:57:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 21:57:15]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[rlarocque, 2014-07-08 21:59:51]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-08 22:03:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/322333004/260001

[commit-bot: I haz the power, 2014-07-08 23:38:25]

Change committed as 281884