Commit InvalidationService implementations

Commit InvalidationService implementations

This commit introduces a new ProfileKeyedService, the
InvalidationService.  Clients may register with this service to receive
notifications from a server informing them when they need to fetch
updates to their cached state.  (In other words, they will receive
notifications when their cache has become invalid.)

There are three different implementations of this service, all of which
inherit from InvalidationFrontend.  The current provider of
invalidations, ProfileSyncService, also inherits from this interface.

The TiclInvalidationService receives notifications through the C++
version of the googlecacheinvalidation (aka. TICL) library.  It is used
on Chromium for Desktop operating systems.

The AndroidInvalidationService receives notifications by listening to
the in-process NotificationService
(content/public/browser/notification_service.h).  The code that actually
sends these notifications is a small bridge from Java-land, which is not
affected by this CL.

The P2PInvalidationService uses XMPP peer-to-peer connections to send
and receive invalidations.  This has not been used in Chromium for a
very long time.   We keep it around because some of our integration
tests rely on it.

This CL also includes an InvalidationServiceFactory to help manage the
selection and construction of these InvalidationServices.

The tests for these new services are based on a modified version of
sync/notifier/invalidator_test_template.h.  The P2PInvalidationService
is not tested at all because it is itself a test-only utility.

In addition to the new code, this CL moves around some old code to help
populate a new directory, chrome/browser/invalidation.  We're trying to
separate invalidations from sync, so the
chrome/browser/sync/invalidations directory was no longer needed.

Finally, note that this commit does not change Chromium in any way.  It
adds new classes and test for them, but the rest of the code has not
been modified to make use of them.  Those changes will happen in a
future CL.

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

[rlarocque, 2013-04-24 20:26:15]

Here's a more official review request for InvalidationService.  Most of the code
is carried over from https://codereview.chromium.org/13197004/.  

* dcheng: I'll need your OWNERS approval for push_messaging changes.  You're
welcome to review other parts of this CL, too, if you're interested.
nyquist: Please review android_invalidation_service*.
* tim: Please review everything else.
* akalin: You've been added to the reviewers list as an FYI.  You don't have to
review anything if you don't want to.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_frontend.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_frontend.h:6: #define
CHROME_BROWSER_INVALIDATION_INVALIDATION_FRONTEND_H_
Note: The next patch in the series will rename this from InvalidationFrontend to
InvalidationService.  I decided to keep the existing name for now because I
don't want to touch ProfileSyncService or any of the files that treat it as an
InvalidationFrontend in this CL.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_factory.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.h:28: //
TODO(rlarocque): Re-enable this once InvalidationFrontend can extend
These TODOs will be fixed in the next patch in this series.  I won't be able to
make this compile correctly until InvalidationFrontend (soon to be renamed
InvalidationService) inherits from ProfileKeyedService.  Doing so would confuse
the inheritance hierarchy of ProfileSyncService, so I decided not to implement
it in this CL.  In the next CL, ProfileSyncService will have nothing to do with
invalidations, so this problem becomes much easier to solve.

[rlarocque, 2013-04-29 20:25:52]

ping.

[dcheng, 2013-04-29 21:45:23]

c/b/e/a/push_messaging LGTM

[rlarocque, 2013-05-01 20:50:02]

ping again.

[tim (not reviewing), 2013-05-03 17:48:23]

Nice patch.
Please add a browser/invalidation/OWNERS file. The reviewers of this CL +
yourself seem like a good start.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_factory.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.cc:35: #ifndef
OS_ANDROID
nit- prefer #if !defined(OS_ANDROID)

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.cc:62: #ifndef
OS_ANDROID
nit - #if !defined(OS_ANDROID)

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_factory.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.h:27: public:
nit- one space indent for public: / private:

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.h:46: //
TODO(rlarocque): Use this class, not InvalidatorStorage, to register
Use what class? The function?

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_util.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_util.cc:55: }
// namespace invalidation

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/p2p_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.cc:31: invalidator_id_ =
GenerateInvalidatorClientId();
Wouldn't hurt to DCHECK or CHECK the id is non empty. Maybe in
GenerateInvalidatorClientId, so that it's clear that function should never
return an empty Id regardless of how it's implemented down the road.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/p2p_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.h:59: std::string
invalidator_id_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:25:
invalidator_registrar_(new syncer::InvalidatorRegistrar()) { }
init profile_ to NULL.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:58: profile_ = profile;
you could also DCHECK(!profile_) before assigning.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:116: if (invalidator_)
{
Do we expect this function to be called if invalidator_ is NULL?

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:134: //
invalidator_storage will be initialized between calls to Init() and Stop().
nit - invalidator_storage_

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:162: break;
It's common practice in NotificationObserver implementations to have a default:
NOTREACHED() branch.

Also, if you use {} for one case branch, you should use the braces in all cases.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:203: return
!!invalidator_;
I personally really prefer invalidator_ != NULL for clarity, but ... no big
deal.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/ticl_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.h:74:
TiclInvalidationService(SigninManager* signin,
Rather than rely on the comment, why not have an InitForTest method that makes
the intent of this separate function very clear.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.h:101: }
// namespace invalidation

[dcheng, 2013-05-03 18:07:34]

Some random comments on stuff outside c/b/e/a/push_messaging.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/android_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/android_invalidation_service.h:27: public
content::NotificationObserver{
Nit: space.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/android_invalidation_service.h:65: };
I'm pretty sure this is non-assignable and non-copyable due to one of the
inherited classes already, but it'd be nice to make this explicit.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_util.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_util.h:17: // Genreates a
unique client ID for the invalidator.
Nit: Generates

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/p2p_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.h:55: const
syncer::ObjectIdInvalidationMap& invalidation_map);
Does this need to be virtual? I don't think InvalidationFrontend has this in the
interface, so I'm curious where it's coming from.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/ticl_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.h:41: void Init(Profile*
profile);
Out of curiosity, is there a particular reason that the construction logic is
split between the ctor and Init()? It seems like you could combine them in all
cases (and mark profile_, signin_manager_, and token_service as T* consts as
well).

[rlarocque, 2013-05-03 22:16:14]

I've fixed a bunch of issues and responded to your comments.  PTAL.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/android_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/android_invalidation_service.h:27: public
content::NotificationObserver{
On 2013/05/03 18:07:34, dcheng wrote:
> Nit: space.

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/android_invalidation_service.h:65: };
On 2013/05/03 18:07:34, dcheng wrote:
> I'm pretty sure this is non-assignable and non-copyable due to one of the
> inherited classes already, but it'd be nice to make this explicit.

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_factory.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.cc:35: #ifndef
OS_ANDROID
On 2013/05/03 17:48:23, timsteele wrote:
> nit- prefer #if !defined(OS_ANDROID)

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.cc:62: #ifndef
OS_ANDROID
On 2013/05/03 17:48:23, timsteele wrote:
> nit - #if !defined(OS_ANDROID)

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_factory.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.h:27: public:
On 2013/05/03 17:48:23, timsteele wrote:
> nit- one space indent for public: / private:

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_factory.h:46: //
TODO(rlarocque): Use this class, not InvalidatorStorage, to register
On 2013/05/03 17:48:23, timsteele wrote:
> Use what class? The function?

In chrome/browser/prefs/browser_prefs.cc, RegisterUserPrefs(), we use
invalidator::InvalidatorStoreage::RegisterUserPrefs() to register some
invalidations-related preferences.  I think it would be more appropriate to use
this factory to register those preferences.  

I've implemented that change in the "full" version of this patch, but I had to
revert it when the Cl was split in two.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/invalidation_service_util.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/invalidation_service_util.h:17: // Genreates a
unique client ID for the invalidator.
On 2013/05/03 18:07:34, dcheng wrote:
> Nit: Generates

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/p2p_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.cc:31: invalidator_id_ =
GenerateInvalidatorClientId();
On 2013/05/03 17:48:23, timsteele wrote:
> Wouldn't hurt to DCHECK or CHECK the id is non empty. Maybe in
> GenerateInvalidatorClientId, so that it's clear that function should never
> return an empty Id regardless of how it's implemented down the road.

The comment for GenerateInvalidatorClientId() is currently "// Generates a
unique client ID for the invalidator."

I think that implies that it will always return a non-empty string.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/p2p_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.h:55: const
syncer::ObjectIdInvalidationMap& invalidation_map);
On 2013/05/03 18:07:34, dcheng wrote:
> Does this need to be virtual? I don't think InvalidationFrontend has this in
the
> interface, so I'm curious where it's coming from.

You might be right.  SendInvalidation() used to be part of the Invalidator's
interface.  I must have forgot to update the signature when I copied and pasted
it.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.h:59: std::string
invalidator_id_;
On 2013/05/03 17:48:23, timsteele wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:25:
invalidator_registrar_(new syncer::InvalidatorRegistrar()) { }
On 2013/05/03 17:48:23, timsteele wrote:
> init profile_ to NULL.

Fixed with a refactoring.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:58: profile_ = profile;
On 2013/05/03 17:48:23, timsteele wrote:
> you could also DCHECK(!profile_) before assigning.

Fixed with a refactoring.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:116: if (invalidator_)
{
On 2013/05/03 17:48:23, timsteele wrote:
> Do we expect this function to be called if invalidator_ is NULL?

I'm not sure, but I think it's possible.  We'll reset the invalidator_ if the
user signs out.  It's possible that some service will not have received the
notification of the signout in time, or just doesn't handle it correctly.  

I'd rather just handle it here by not crashing, rather than expecting all our
clients to handle signout very carefully.  Although I usually care a lot about
correctness, I'm willing to make an exception in this case.  We'll delete the
invalidator's storage, so it doesn't matter if the invalidations are acked or
not.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:134: //
invalidator_storage will be initialized between calls to Init() and Stop().
On 2013/05/03 17:48:23, timsteele wrote:
> nit - invalidator_storage_

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:162: break;
On 2013/05/03 17:48:23, timsteele wrote:
> It's common practice in NotificationObserver implementations to have a
default:
> NOTREACHED() branch.
> 
> Also, if you use {} for one case branch, you should use the braces in all
cases.

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.cc:203: return
!!invalidator_;
On 2013/05/03 17:48:23, timsteele wrote:
> I personally really prefer invalidator_ != NULL for clarity, but ... no big
> deal.

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/ticl_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.h:41: void Init(Profile*
profile);
On 2013/05/03 18:07:34, dcheng wrote:
> Out of curiosity, is there a particular reason that the construction logic is
> split between the ctor and Init()? It seems like you could combine them in all
> cases (and mark profile_, signin_manager_, and token_service as T* consts as
> well).

I think that's probably historical reasons.  Taking into account Tim's comment,
I'll see if I can split the initialization code between the constructor, Init()
and InitForTest() in a more sensible way.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.h:74:
TiclInvalidationService(SigninManager* signin,
On 2013/05/03 17:48:23, timsteele wrote:
> Rather than rely on the comment, why not have an InitForTest method that makes
> the intent of this separate function very clear.

Done.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/ticl_invalidation_service.h:101: }
On 2013/05/03 17:48:23, timsteele wrote:
> // namespace invalidation

Done.

[tim (not reviewing), 2013-05-03 22:23:50]

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/p2p_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.cc:31: invalidator_id_ =
GenerateInvalidatorClientId();
On 2013/05/03 22:16:14, rlarocque wrote:
> On 2013/05/03 17:48:23, timsteele wrote:
> > Wouldn't hurt to DCHECK or CHECK the id is non empty. Maybe in
> > GenerateInvalidatorClientId, so that it's clear that function should never
> > return an empty Id regardless of how it's implemented down the road.
> 
> The comment for GenerateInvalidatorClientId() is currently "// Generates a
> unique client ID for the invalidator."
> 
> I think that implies that it will always return a non-empty string.

It seems sloppy to ignore the return value of Encode and not make an assertion
anywhere that we're ignoring it because we're passing something in that we
expect to never fail encoding.

https://codereview.chromium.org/13991017/diff/52001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/52001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:55: void
TiclInvalidationService::InitForTest(syncer::Invalidator* invalidator) {
You need to assign this somewhere?
Also, maybe InitAndStart for test is more accurate given your comment.

[rlarocque, 2013-05-03 22:49:33]

Uploaded another update.

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
File chrome/browser/invalidation/p2p_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/2001/chrome/browser/invalidatio...
chrome/browser/invalidation/p2p_invalidation_service.cc:31: invalidator_id_ =
GenerateInvalidatorClientId();
On 2013/05/03 22:23:50, timsteele wrote:
> On 2013/05/03 22:16:14, rlarocque wrote:
> > On 2013/05/03 17:48:23, timsteele wrote:
> > > Wouldn't hurt to DCHECK or CHECK the id is non empty. Maybe in
> > > GenerateInvalidatorClientId, so that it's clear that function should never
> > > return an empty Id regardless of how it's implemented down the road.
> > 
> > The comment for GenerateInvalidatorClientId() is currently "// Generates a
> > unique client ID for the invalidator."
> > 
> > I think that implies that it will always return a non-empty string.
> 
> It seems sloppy to ignore the return value of Encode and not make an assertion
> anywhere that we're ignoring it because we're passing something in that we
> expect to never fail encoding.

I added the DCHECK.  I still don't understand how it could possibly fail,
though.  This is the same code we've been using for years to generate sync's
unique client ID.

https://codereview.chromium.org/13991017/diff/52001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/13991017/diff/52001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:55: void
TiclInvalidationService::InitForTest(syncer::Invalidator* invalidator) {
On 2013/05/03 22:23:50, timsteele wrote:
> You need to assign this somewhere?
> Also, maybe InitAndStart for test is more accurate given your comment.

Start() is private method.  This class is responsible for starting itself when
appropriate.  I'm not sure that the caller needs to know that we start
automatically when we call InitForTest(), since it doesn't have any control over
when we start in the normal init case, either.  

You're right that the invalidator should be assigned.  And it looks like I don't
actually need the InvalidatorStorage to be initialized.

[tim (not reviewing), 2013-05-03 23:17:26]

Changes I reviewed LGTM. (Not speaking for Daniel). It'd be good for Fred to
take a look when he gets back, but you could always address his comments in a
follow up.

[rlarocque, 2013-05-03 23:22:38]

On 2013/05/03 23:17:26, timsteele wrote:
> Changes I reviewed LGTM. (Not speaking for Daniel). It'd be good for Fred to
> take a look when he gets back, but you could always address his comments in a
> follow up.

I'll wait for Fred.  Committing this non-urgent and very large patch on the
Friday before a branch would be a bad idea anyway.

[akalin, 2013-05-08 00:43:31]

Nice patch, LGTM!

Typo in CL description (daiton -> dation)

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/android_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/android_invalidation_service.h:39:
syncer::InvalidationHandler* handler) OVERRIDE;
compiler_specific.h for OVERRIDE

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_frontend.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_frontend.h:46: // shut down; clients
must ensure that they unregister themselves before then.
Add TODO to extend ProfileKeyedService?

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_frontend_test_template.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_frontend_test_template.h:92:
id2(ipc::invalidation::ObjectSource::CHROME_SYNC, "PREFERENCE"),
Might be a nicely symbolic to use other ObjectSource constants and IDs here
instead of having them all be sync-related

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_factory.cc (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_factory.cc:52: #if
!defined(OS_ANDROID)
i think testing the other way would be clearer:

#if defined(OS_ANDROID)
  AndroidInvalidation...
#else
  ...
#endif
  return service;

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_util.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_util.h:20: }
  // namespace invalidation

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.h:91:
scoped_ptr<syncer::InvalidatorRegistrar> invalidator_registrar_;
include scoped_ptr.h for scoped_ptr

[akalin, 2013-05-08 00:43:51]

On 2013/05/08 00:43:31, akalin wrote:
> Nice patch, LGTM!
> 
> Typo in CL description (daiton -> dation)

Rather, caiton -> cation

[rlarocque, 2013-05-08 01:22:28]

Thanks for the review!  Only two more approvals required.  Tommy, could you take
a look at those Android files?

+bauerb for OWNERS review of chrome/browser/prefs/browser_prefs.cc

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/android_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/android_invalidation_service.h:39:
syncer::InvalidationHandler* handler) OVERRIDE;
On 2013/05/08 00:43:31, akalin wrote:
> compiler_specific.h for OVERRIDE

Done.

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_frontend.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_frontend.h:46: // shut down; clients
must ensure that they unregister themselves before then.
On 2013/05/08 00:43:31, akalin wrote:
> Add TODO to extend ProfileKeyedService?

Done.

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_frontend_test_template.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_frontend_test_template.h:92:
id2(ipc::invalidation::ObjectSource::CHROME_SYNC, "PREFERENCE"),
On 2013/05/08 00:43:31, akalin wrote:
> Might be a nicely symbolic to use other ObjectSource constants and IDs here
> instead of having them all be sync-related

Unfortunately, the AndroidInvalidationService still deals only in ModelTypes.  

As far as I know that's not really necessary.  There's a TODO about fixing this
with your name on it.  I think it would be a simple fix, but it's not something
I'm prepared to deal with in this CL.

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_factory.cc (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_factory.cc:52: #if
!defined(OS_ANDROID)
On 2013/05/08 00:43:31, akalin wrote:
> i think testing the other way would be clearer:
> 
> #if defined(OS_ANDROID)
>   AndroidInvalidation...
> #else
>   ...
> #endif
>   return service;

Done.

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_util.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_util.h:20: }
On 2013/05/08 00:43:31, akalin wrote:
>   // namespace invalidation

Done.

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.h (right):

https://codereview.chromium.org/13991017/diff/54001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.h:91:
scoped_ptr<syncer::InvalidatorRegistrar> invalidator_registrar_;
On 2013/05/08 00:43:31, akalin wrote:
> include scoped_ptr.h for scoped_ptr

Done.

[Bernhard Bauer, 2013-05-08 07:11:03]

c/b/p LGTM.

[nyquist, 2013-05-08 22:44:29]

lgtm for Android parts

[commit-bot: I haz the power, 2013-05-08 22:52:26]

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

[commit-bot: I haz the power, 2013-05-08 23:21:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-09 00:08:50]

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

[commit-bot: I haz the power, 2013-05-09 00:46:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-10 17:35:27]

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

[commit-bot: I haz the power, 2013-05-10 19:26:18]

Change committed as 199520

[Joao da Silva, 2013-05-15 13:01:59]

Hey Richard, we (Chrome enterprise) are planning to eventually use these
interfaces, and a question popped up while reading the code; please see inline.

Thanks!
- Joao

https://chromiumcodereview.appspot.com/13991017/diff/108006/chrome/browser/in...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://chromiumcodereview.appspot.com/13991017/diff/108006/chrome/browser/in...
chrome/browser/invalidation/ticl_invalidation_service.cc:51:
chrome::NOTIFICATION_GOOGLE_SIGNED_OUT,
Question: if there are multiple profiles and one of them signs out, then the
TiclInvalidationService for all profiles will see this notification, and all of
them will stop; should this registration be scoped to Source<Profile>(profile_)?

[rlarocque, 2013-05-15 20:29:39]

https://chromiumcodereview.appspot.com/13991017/diff/108006/chrome/browser/in...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://chromiumcodereview.appspot.com/13991017/diff/108006/chrome/browser/in...
chrome/browser/invalidation/ticl_invalidation_service.cc:51:
chrome::NOTIFICATION_GOOGLE_SIGNED_OUT,
On 2013/05/15 13:02:00, Joao da Silva wrote:
> Question: if there are multiple profiles and one of them signs out, then the
> TiclInvalidationService for all profiles will see this notification, and all
of
> them will stop; should this registration be scoped to
Source<Profile>(profile_)?

You're right, the registration should apply to notifications from this profile
only.

It's also not the only bug related to sign in and sign out.  I'm working on a
patch to fix those issues, and I'll be sure to add this fix to the list.

Thanks for pointing this out.