Make use of InvalidationService

Make use of InvalidationService

The InvalidationService was introduced r199520.  That commit added the
InvalidationService interface and several implementations of it, but
made no use of the new code.  This commit builds on that work.

Up until now, TICL invalidations were handled on the sync thread.  The
related objects were instantiated and owned by the SyncBackendHost and
SyncManager.  All requests to update the set of object registrations had
to be passed to the sync thread.  Components that wanted to receive
invalidations but were not part of sync had to route their communication
with the invalidations server through ProfileSyncService to get to the
sync thread.  Things were a bit different on Android, but the system
still tried to pretend that invalidations were owned by the sync thread.

The new InvalidationService implementation is a ProfileKeyedService that
is mostly independent from sync.  It still relies on sync to manage sign
in and fetch the appropriate auth tokens.  However, it's now much easier
for components outside of sync to communication with the invalidations
server.

The new system allows us to remove a lot of invalidations-related code
from the ProfileSyncService, SyncBackendHost and SyncManager.  Sync is
now just one of many clients of the InvalidationService.  The
SyncBackendHost is responsible for forwarding messages back and forth
between the InvalidationService and the sync thread.

TBR=sky,erg
BUG=124137
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208315

[rlarocque, 2013-05-21 20:51:35]

I think this is ready for a review.  It's the follow-up to
https://codereview.chromium.org/13991017/.

This touches quite a few files, so I'll need a couple of OWNERs reviews for some
mostly trivial changes:
- msw: c/b/chrome_to_mobile_service*
- dcheng: c/b/extensions/api/push_messaging*
- satorux: c/b/google_apis/drive_notification_manager*
- battre: c/b/prefs/browser_prefs.cc
- rsimha: sync/tools/testserver/xmppserver.py

I've also added ghc and tim as an FYI.  They don't have to review anything if
they don't want to.  

Fred: Could you review the rest of this patch (ie. most of it)?

Most of the diffs are deletions, so it's not as big as it looks.  Still, let me
know if you'd like some help.  We can delegate some of the review work to
someone else if you're too busy to review the whole thing.

https://codereview.chromium.org/15580002/diff/1/chrome/browser/prefs/browser_...
File chrome/browser/prefs/browser_prefs.cc (left):

https://codereview.chromium.org/15580002/diff/1/chrome/browser/prefs/browser_...
chrome/browser/prefs/browser_prefs.cc:298:
invalidation::InvalidatorStorage::RegisterUserPrefs(registry);
In case you're wondering, this call was moved to the InvalidationServiceFactory.

I figured that ProfileKeyedServiceBase must provide the RegisterUserPrefs()
function for a reason.  Maybe it will avoid registering the pref if an
invalidation service is never isntantiated?  I didn't really look into it.

https://codereview.chromium.org/15580002/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/15580002/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:528:
signin->SetAuthenticatedUsername("test_user@gmail.com");
The invalidator can get upset if it finds the username doesn't look like an
email address.

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_notification_manager.cc (left):

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_notification_manager.cc:51:
profile_sync_service->UpdateRegisteredInvalidationIds(
The API expects that clients will not unregister themselves unless their feature
is being permanently disabled (eg. user signs out).  It would be a misuse of the
API to unregister at every shutdown.  

I've taken the liberty of removing this unregistration request.  I can put it
back if there's a good reason to keep it.

[satorux1, 2013-05-22 02:24:10]

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_notification_manager.cc (left):

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_notification_manager.cc:51:
profile_sync_service->UpdateRegisteredInvalidationIds(
On 2013/05/21 20:51:36, rlarocque wrote:
> The API expects that clients will not unregister themselves unless their
feature
> is being permanently disabled (eg. user signs out).  It would be a misuse of
the
> API to unregister at every shutdown.  
> 
> I've taken the liberty of removing this unregistration request.  I can put it
> back if there's a good reason to keep it.  

I'm a bit confused. Call to UpdateRegisteredInvalidationIds() is removed, but
call to UnregisterInvalidationHandler() remains. Do we need to keep the latter?
If we don't need the latter, Shutdown() function will be empty, which is nice.

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_notification_manager.cc (right):

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_notification_manager.cc:79:
CHECK(invalidation_service);
DCHECK? I think we rarely use CHECK.

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

[battre, 2013-05-22 07:24:40]

rubberstamp LGTM @ chrome/browser/prefs/browser_prefs.cc

[rlarocque, 2013-05-22 23:46:51]

Added responses to satorux's comments.

+Tommy for review of chrome/browser/invalidation/invalidation_service_android.*

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_notification_manager.cc (left):

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_notification_manager.cc:51:
profile_sync_service->UpdateRegisteredInvalidationIds(
On 2013/05/22 02:24:11, satorux1 wrote:
> On 2013/05/21 20:51:36, rlarocque wrote:
> > The API expects that clients will not unregister themselves unless their
> feature
> > is being permanently disabled (eg. user signs out).  It would be a misuse of
> the
> > API to unregister at every shutdown.  
> > 
> > I've taken the liberty of removing this unregistration request.  I can put
it
> > back if there's a good reason to keep it.  
> 
> I'm a bit confused. Call to UpdateRegisteredInvalidationIds() is removed, but
> call to UnregisterInvalidationHandler() remains. Do we need to keep the
latter?
> If we don't need the latter, Shutdown() function will be empty, which is nice.

I agree, it does look strange.  There's a valid reason for it, though.  

The Invalidations server (Tango) wants clients to stay registered even after
they've gone offline.  The only case where we should unregister at the server is
when we have no need to receive those notifications for the foreseeable future. 
That might be the case if the user signs out of the browser, for example.  

The registrations are managed further down the stack, inside classes that we
can't access here.  They best way to communicate to them that we want to remain
registered even though we've shut down is to *not* update the registrations
here.  

We still want to call UnregisterInvalidationHandler.  This will unregister us
with the InvalidationService, but not unregister our IDs at the Tango server. 
The InvalidationService should not maintain a pointer to this object after it is
destroyed.  I think we also DCHECK() that all clients of the InvalidationService
have called UnregisterInvalidationHandler by the time we shut down.

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_notification_manager.cc (right):

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_notification_manager.cc:79:
CHECK(invalidation_service);
On 2013/05/22 02:24:11, satorux1 wrote:
> DCHECK? I think we rarely use CHECK.
> 
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

Done.

[rlarocque, 2013-05-22 23:47:14]

Actually add nyquist@ this time.

[msw, 2013-05-23 00:00:02]

chrome/browser/chrome_to_mobile_service* LGTM with nits.

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/chrome_to_...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/chrome_to_...
chrome/browser/chrome_to_mobile_service.cc:268: if (profile_) {
Side note: Too bad ProfileKeyedBaseFactory::GetBrowserContextToUse doesn't NULL
check, otherwise GetForProfile could handle optionally NULL Profile*s.

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/chrome_to_...
chrome/browser/chrome_to_mobile_service.cc:392: profile_ ?
nit: move "profile_ ?" to the line above, outdent and combine lines below.
Bonus points for making all three GetForProfile calls look like that.

[dcheng, 2013-05-23 00:35:45]

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc
(right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:69:
InvalidationServiceFactory::GetInstance()->
If this line doesn't fit on line 68, I think I would expect to see line 70
indented an additional 4.

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/invalidati...
File chrome/browser/invalidation/fake_invalidation_service.h (right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/invalidati...
chrome/browser/invalidation/fake_invalidation_service.h:42: };
DISALLOW_COPY_AND_ASSIGN.

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

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.h:28: class
PushMessagingApiTest;
Ick. Can you file a bug to clean this up and assign it to me?

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/sync_test.cc:313:
BuildAndUseP2PInvalidationServiceForTest(GetProfile(index));
Nit: I think I'd expect a 4 space indent (rather than 6) on 312, and then 313 to
be intended four more.

https://codereview.chromium.org/15580002/diff/11001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/15580002/diff/11001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1249: // TODO(dcheng): Acknowledge
immediately for now. Fix this once the
How come this is being removed? IIRC, SyncInvalidationListener still has the
'pinging' functionality (I wonder if we should remove it at this point since no
one is using it at all, but that's a separate discussion...)

[satorux1, 2013-05-23 00:54:20]

chrome/browser/google_apis LGTM, with a request:

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_notification_manager.cc (left):

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_notification_manager.cc:51:
profile_sync_service->UpdateRegisteredInvalidationIds(
On 2013/05/22 23:46:51, rlarocque wrote:
> On 2013/05/22 02:24:11, satorux1 wrote:
> > On 2013/05/21 20:51:36, rlarocque wrote:
> > > The API expects that clients will not unregister themselves unless their
> > feature
> > > is being permanently disabled (eg. user signs out).  It would be a misuse
of
> > the
> > > API to unregister at every shutdown.  
> > > 
> > > I've taken the liberty of removing this unregistration request.  I can put
> it
> > > back if there's a good reason to keep it.  
> > 
> > I'm a bit confused. Call to UpdateRegisteredInvalidationIds() is removed,
but
> > call to UnregisterInvalidationHandler() remains. Do we need to keep the
> latter?
> > If we don't need the latter, Shutdown() function will be empty, which is
nice.
> 
> I agree, it does look strange.  There's a valid reason for it, though.  
> 
> The Invalidations server (Tango) wants clients to stay registered even after
> they've gone offline.  The only case where we should unregister at the server
is
> when we have no need to receive those notifications for the foreseeable
future. 
> That might be the case if the user signs out of the browser, for example.  
> 
> The registrations are managed further down the stack, inside classes that we
> can't access here.  They best way to communicate to them that we want to
remain
> registered even though we've shut down is to *not* update the registrations
> here.  
> 
> We still want to call UnregisterInvalidationHandler.  This will unregister us
> with the InvalidationService, but not unregister our IDs at the Tango server. 
> The InvalidationService should not maintain a pointer to this object after it
is
> destroyed.  I think we also DCHECK() that all clients of the
InvalidationService
> have called UnregisterInvalidationHandler by the time we shut down.

Thank you for the explanation. Could you leave a shorter version as a comment
here?

[rlarocque, 2013-05-23 21:43:27]

I've rebased and uploaded a new patch to address msw, satorux and dcheng's
latest comments.  PTAL.

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_notification_manager.cc (left):

https://codereview.chromium.org/15580002/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_notification_manager.cc:51:
profile_sync_service->UpdateRegisteredInvalidationIds(
On 2013/05/23 00:54:21, satorux1 wrote:
> On 2013/05/22 23:46:51, rlarocque wrote:
> > On 2013/05/22 02:24:11, satorux1 wrote:
> > > On 2013/05/21 20:51:36, rlarocque wrote:
> > > > The API expects that clients will not unregister themselves unless their
> > > feature
> > > > is being permanently disabled (eg. user signs out).  It would be a
misuse
> of
> > > the
> > > > API to unregister at every shutdown.  
> > > > 
> > > > I've taken the liberty of removing this unregistration request.  I can
put
> > it
> > > > back if there's a good reason to keep it.  
> > > 
> > > I'm a bit confused. Call to UpdateRegisteredInvalidationIds() is removed,
> but
> > > call to UnregisterInvalidationHandler() remains. Do we need to keep the
> > latter?
> > > If we don't need the latter, Shutdown() function will be empty, which is
> nice.
> > 
> > I agree, it does look strange.  There's a valid reason for it, though.  
> > 
> > The Invalidations server (Tango) wants clients to stay registered even after
> > they've gone offline.  The only case where we should unregister at the
server
> is
> > when we have no need to receive those notifications for the foreseeable
> future. 
> > That might be the case if the user signs out of the browser, for example.  
> > 
> > The registrations are managed further down the stack, inside classes that we
> > can't access here.  They best way to communicate to them that we want to
> remain
> > registered even though we've shut down is to *not* update the registrations
> > here.  
> > 
> > We still want to call UnregisterInvalidationHandler.  This will unregister
us
> > with the InvalidationService, but not unregister our IDs at the Tango
server. 
> > The InvalidationService should not maintain a pointer to this object after
it
> is
> > destroyed.  I think we also DCHECK() that all clients of the
> InvalidationService
> > have called UnregisterInvalidationHandler by the time we shut down.
> 
> Thank you for the explanation. Could you leave a shorter version as a comment
> here?
> 

Done.

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/chrome_to_...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/chrome_to_...
chrome/browser/chrome_to_mobile_service.cc:392: profile_ ?
On 2013/05/23 00:00:02, msw wrote:
> nit: move "profile_ ?" to the line above, outdent and combine lines below.
> Bonus points for making all three GetForProfile calls look like that.

Done.

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc
(right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:69:
InvalidationServiceFactory::GetInstance()->
On 2013/05/23 00:35:45, dcheng wrote:
> If this line doesn't fit on line 68, I think I would expect to see line 70
> indented an additional 4.

Done.

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/invalidati...
File chrome/browser/invalidation/fake_invalidation_service.h (right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/invalidati...
chrome/browser/invalidation/fake_invalidation_service.h:42: };
On 2013/05/23 00:35:45, dcheng wrote:
> DISALLOW_COPY_AND_ASSIGN.

Done.

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

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.h:28: class
PushMessagingApiTest;
On 2013/05/23 00:35:45, dcheng wrote:
> Ick. Can you file a bug to clean this up and assign it to me?

Done.  See crbug.com/243482.

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/15580002/diff/11001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/sync_test.cc:313:
BuildAndUseP2PInvalidationServiceForTest(GetProfile(index));
On 2013/05/23 00:35:45, dcheng wrote:
> Nit: I think I'd expect a 4 space indent (rather than 6) on 312, and then 313
to
> be intended four more.

Done.

https://codereview.chromium.org/15580002/diff/11001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/15580002/diff/11001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1249: // TODO(dcheng): Acknowledge
immediately for now. Fix this once the
On 2013/05/23 00:35:45, dcheng wrote:
> How come this is being removed? IIRC, SyncInvalidationListener still has the
> 'pinging' functionality (I wonder if we should remove it at this point since
no
> one is using it at all, but that's a separate discussion...)

The invalidations client for sync is now SyncBackendHost.  This code lives on at
sync_backend_host.cc:1447.

[dcheng, 2013-05-24 18:27:25]

LGTM for the push_messaging changes.

[akalin, 2013-05-24 22:58:46]

made a pass, found some nits. still trying to digest.

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
File chrome/browser/chrome_to_mobile_service.h (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
chrome/browser/chrome_to_mobile_service.h:204: // Invalidation service state.
Chrome To Mobile requires this service to to
remove extra 'to'

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
File chrome/browser/chrome_to_mobile_service_unittest.cc (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
chrome/browser/chrome_to_mobile_service_unittest.cc:56: :
enable_action_box_(FeatureSwitch::action_box(), true) {
revert diff (or change the destructor to match)

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/google_api...
File chrome/browser/google_apis/drive_notification_manager.cc (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.cc:52: // purpose.  See
the class comment on the InavlidationService interface for
Inavl -> Inval

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:1447: // TODO(dcheng): Acknowledge
immediately for now. Fix this once the
ah, here it is.

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:115: // The first parameter
there shoudl be the set of enabled types.  That's not
shoudl -> should

https://codereview.chromium.org/15580002/diff/35003/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/15580002/diff/35003/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1252: // See http://crbug.com/124149 for
more information.
this is safe to remove now?

https://codereview.chromium.org/15580002/diff/35003/sync/notifier/p2p_invalid...
File sync/notifier/p2p_invalidator.h (right):

https://codereview.chromium.org/15580002/diff/35003/sync/notifier/p2p_invalid...
sync/notifier/p2p_invalidator.h:121: virtual void SendInvalidation(
still needs to be virtual?

https://codereview.chromium.org/15580002/diff/35003/sync/tools/sync_client.cc
File sync/tools/sync_client.cc (right):

https://codereview.chromium.org/15580002/diff/35003/sync/tools/sync_client.cc...
sync/tools/sync_client.cc:278: Invalidator* invalidator = new
NonBlockingInvalidator(
what owns the invalidator?

https://codereview.chromium.org/15580002/diff/35003/sync/tools/testserver/xmp...
File sync/tools/testserver/xmppserver.py (right):

https://codereview.chromium.org/15580002/diff/35003/sync/tools/testserver/xmp...
sync/tools/testserver/xmppserver.py:576: if not self._authenticated:
why did this become necessary?

[rlarocque, 2013-05-29 00:37:55]

Patch updated.  PTAL.

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
File chrome/browser/chrome_to_mobile_service.h (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
chrome/browser/chrome_to_mobile_service.h:204: // Invalidation service state.
Chrome To Mobile requires this service to to
On 2013/05/24 22:58:46, akalin wrote:
> remove extra 'to'

Done.

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
File chrome/browser/chrome_to_mobile_service_unittest.cc (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/chrome_to_...
chrome/browser/chrome_to_mobile_service_unittest.cc:56: :
enable_action_box_(FeatureSwitch::action_box(), true) {
On 2013/05/24 22:58:46, akalin wrote:
> revert diff (or change the destructor to match)

Done.

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/google_api...
File chrome/browser/google_apis/drive_notification_manager.cc (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.cc:52: // purpose.  See
the class comment on the InavlidationService interface for
On 2013/05/24 22:58:46, akalin wrote:
> Inavl -> Inval

Done.

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15580002/diff/35003/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:115: // The first parameter
there shoudl be the set of enabled types.  That's not
On 2013/05/24 22:58:46, akalin wrote:
> shoudl -> should

Done.

https://codereview.chromium.org/15580002/diff/35003/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/15580002/diff/35003/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1252: // See http://crbug.com/124149 for
more information.
On 2013/05/24 22:58:46, akalin wrote:
> this is safe to remove now?

I think so.  I've moved this to SyncBackendHost for the case of sync, and the
other invalidation handling services seem to have their own variants of this
immediate ack code.

https://codereview.chromium.org/15580002/diff/35003/sync/notifier/p2p_invalid...
File sync/notifier/p2p_invalidator.h (right):

https://codereview.chromium.org/15580002/diff/35003/sync/notifier/p2p_invalid...
sync/notifier/p2p_invalidator.h:121: virtual void SendInvalidation(
On 2013/05/24 22:58:46, akalin wrote:
> still needs to be virtual?

Nope.  Fixed.

https://codereview.chromium.org/15580002/diff/35003/sync/tools/sync_client.cc
File sync/tools/sync_client.cc (right):

https://codereview.chromium.org/15580002/diff/35003/sync/tools/sync_client.cc...
sync/tools/sync_client.cc:278: Invalidator* invalidator = new
NonBlockingInvalidator(
On 2013/05/24 22:58:46, akalin wrote:
> what owns the invalidator?

In this patch, no one owns it.  I'll change this to a scoped_ptr.

Even after that change, I think it might leak some memory during shutdown.  If
we stop the IO thread before the NonBlockingInvalidator is destroyed, then I
suspect it will be unable to destroy the blocking Invalidator that it owns.

On the other hand, how does this program shut down gracefully?  I've always
assumed that Ctrl+C was the only way to stop it.

https://codereview.chromium.org/15580002/diff/35003/sync/tools/testserver/xmp...
File sync/tools/testserver/xmppserver.py (right):

https://codereview.chromium.org/15580002/diff/35003/sync/tools/testserver/xmp...
sync/tools/testserver/xmppserver.py:576: if not self._authenticated:
On 2013/05/24 22:58:46, akalin wrote:
> why did this become necessary?

This was written a while ago, so I don't remember all the details.  

I think it's related to SyncErrorTest.XmppAuthErrorTest:
  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";

  TriggerXmppAuthError();

  ASSERT_FALSE(GetClient(0)->SetupSync());

  ASSERT_EQ(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS,
            GetClient(0)->service()->GetAuthError().state());

I think the issue was that the python XMPP server only sent auth failures if
they happened when a connection was first established.  If the
self._authenticated flag is set after that point, it has no effect.

My patch probably changed the time at which the connection is established, so
the TriggerXmppAuthError() call had no effect.  Closing the connection here
makes the auth error visible once again.

[rlarocque, 2013-05-30 19:25:25]

ping.

Still awaiting approvals from rsimha, nyquist, and akalin.  See original message
for individual responsibilities.

[Raghu Simha, 2013-05-31 23:46:55]

Change to sync/tools/testserver LGTM.

[rlarocque, 2013-06-03 17:30:06]

On 2013/05/31 23:46:55, rsimha wrote:
> Change to sync/tools/testserver LGTM.

ping again.  Still waiting for Tommy and Fred.

[nyquist, 2013-06-04 18:27:20]

chrome/browser/invalidation/invalidation_service_android.* lgtm

[akalin, 2013-06-04 18:28:29]

On 2013/06/04 18:27:20, nyquist wrote:
> chrome/browser/invalidation/invalidation_service_android.* lgtm

I will look today.

[akalin, 2013-06-04 19:23:53]

LGTM after nits

https://codereview.chromium.org/15580002/diff/35003/sync/tools/sync_client.cc
File sync/tools/sync_client.cc (right):

https://codereview.chromium.org/15580002/diff/35003/sync/tools/sync_client.cc...
sync/tools/sync_client.cc:278: Invalidator* invalidator = new
NonBlockingInvalidator(
On 2013/05/29 00:37:55, rlarocque wrote:
> On 2013/05/24 22:58:46, akalin wrote:
> > what owns the invalidator?
> 
> In this patch, no one owns it.  I'll change this to a scoped_ptr.
> 
> Even after that change, I think it might leak some memory during shutdown.  If
> we stop the IO thread before the NonBlockingInvalidator is destroyed, then I
> suspect it will be unable to destroy the blocking Invalidator that it owns.
> 
> On the other hand, how does this program shut down gracefully?  I've always
> assumed that Ctrl+C was the only way to stop it.

Yeah, there's no graceful shutdown. But it's good to document ownership (via
using scoped_ptr, etc.)

https://codereview.chromium.org/15580002/diff/35003/sync/tools/testserver/xmp...
File sync/tools/testserver/xmppserver.py (right):

https://codereview.chromium.org/15580002/diff/35003/sync/tools/testserver/xmp...
sync/tools/testserver/xmppserver.py:576: if not self._authenticated:
On 2013/05/29 00:37:55, rlarocque wrote:
> On 2013/05/24 22:58:46, akalin wrote:
> > why did this become necessary?
> 
> This was written a while ago, so I don't remember all the details.  
> 
> I think it's related to SyncErrorTest.XmppAuthErrorTest:
>   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
> 
>   TriggerXmppAuthError();
> 
>   ASSERT_FALSE(GetClient(0)->SetupSync());
> 
>   ASSERT_EQ(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS,
>             GetClient(0)->service()->GetAuthError().state());
> 
> I think the issue was that the python XMPP server only sent auth failures if
> they happened when a connection was first established.  If the
> self._authenticated flag is set after that point, it has no effect.
> 
> My patch probably changed the time at which the connection is established, so
> the TriggerXmppAuthError() call had no effect.  Closing the connection here
> makes the auth error visible once again.

Can you add a comment to this effect in this file?

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/extensions...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc
(right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:25:
class MockInvalidationService : public invalidation::InvalidationService {
might be better to use a FakeInvalidationService, but feel free to punt or put a
TODO

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:708:
sync_js_controller_.HandleJsEvent(
one line?

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_harness.h (right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_harness.h:21: namespace invalidation{
space before {

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_observer.h (right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_observer.h:22: virtual void
OnSyncCycleCompleted();
newline before protected:

[rlarocque, 2013-06-04 22:38:16]

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/extensions...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc
(right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:25:
class MockInvalidationService : public invalidation::InvalidationService {
On 2013/06/04 19:23:54, akalin wrote:
> might be better to use a FakeInvalidationService, but feel free to punt or put
a
> TODO

I agree that a FakeInvalidationService would be better, but I decided to leave
it as is.  I wanted to let the authors of PushMessaging test their code as they
saw fit.

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:708:
sync_js_controller_.HandleJsEvent(
On 2013/06/04 19:23:54, akalin wrote:
> one line?

Done.

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_harness.h (right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_harness.h:21: namespace invalidation{
On 2013/06/04 19:23:54, akalin wrote:
> space before {

Done.

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_observer.h (right):

https://codereview.chromium.org/15580002/diff/66001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_observer.h:22: virtual void
OnSyncCycleCompleted();
On 2013/06/04 19:23:54, akalin wrote:
> newline before protected:

Done.

[rlarocque, 2013-06-05 17:04:54]

A DEPS issue led me to investigate drive_notification_manager_factory.cc, which
made me realized that not all services were setup to properly depend on the
InvalidationService.

I've updated them as follows:
- Added a dependency on InvalidationService from ProfileSyncService.
- Added a dependency on InvalidationService from DriveNotificationManager.  It's
dependency on ProfileSyncService was left untouched.
- Removed ChromeToMobileService's dependency on ProfileSyncService and replaced
it with a dependency on InvalidationService.

I'll ping the reviewers of those areas to double-check that these changes make
sense.

[msw, 2013-06-05 17:41:39]

Good catch. c/b/chrome_to_mobile_service* LGTM with the factory update.

[akalin, 2013-06-05 18:00:52]

still lgtm

[rlarocque, 2013-06-06 01:04:24]

In case anyone is wondering about patch set 16:

The PushMessagingApiTests were broken when r202165 added more checking to
prevent the creation of a default Profile before CreateProfile is called.

The fix for this is not obvious.  We want to ensure that the
InvalidationServiceFactory returns a FakeInvalidationService when
GetInstance(profile) is called.  But we don't have, and can't create a Profile
until later.  In fact, InvalidationService::GetInstance(profile) is called
before profile init has completed, so it's hard to setup our factory before
then.  

After talking with stevenjb, I decided to fix it by adding a global flag to the
InvalidationServiceFactory.  When the flag is set, the factory will always
return FakeInvalidationServices for any Profile.  This makes it possible to set
the flag before any Profiles are created.

[satorux1, 2013-06-06 04:05:43]

chrome/browser/google_apis LGTM

https://codereview.chromium.org/15580002/diff/121001/chrome/browser/google_ap...
File chrome/browser/google_apis/drive_notification_manager_factory.cc (right):

https://codereview.chromium.org/15580002/diff/121001/chrome/browser/google_ap...
chrome/browser/google_apis/drive_notification_manager_factory.cc:37:
DependsOn(invalidation::InvalidationServiceFactory::GetInstance());
Thank you for fixing this.

[commit-bot: I haz the power, 2013-06-06 16:49:40]

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

[rlarocque, 2013-06-06 17:13:44]

I decided to uncheck the commit box at the last minute.

This patch is incompatible with some in-progress OAuth2 work.  Because this
patch has a less urgent deadline, I've decided to delay the commit until next
week.

[rlarocque, 2013-06-14 23:48:27]

+Pavel.

Most of the OAuth2 code was borrowed from the code Pavel added to
ProfileSyncService.

I probably should figure out how to write an integration test for token refresh
for the InvalidationService.  However, we don't have a "mock Tango"
implementation, so I'm not sure how well we can simulate the token refresh
scenario.

https://codereview.chromium.org/15580002/diff/135001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/15580002/diff/135001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service.cc:261: if (state ==
syncer::INVALIDATION_CREDENTIALS_REJECTED) {
I decided against modifying SyncManager's invalidations auth error handling
code.  This change here prevents us from sending spurious auth errors to the
SyncManager.  If we get a real auth error (OnGetTokenFailure() returns
INVALID_GAIA_CREDENTIALS), then the failure will be propagated to the
SyncMangaer, which will shut down sync and display an appropriate user-visible
notification.

[pavely, 2013-06-18 23:00:38]

https://codereview.chromium.org/15580002/diff/148001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/15580002/diff/148001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service.cc:260: if (state ==
syncer::INVALIDATION_CREDENTIALS_REJECTED) {
Should this be if (state != ...  ?

Also if it is CREDENTIALS_REJECTED then there should be a codepath that calls
RequestAccessToken.

https://codereview.chromium.org/15580002/diff/148001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.h (left):

https://codereview.chromium.org/15580002/diff/148001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.h:342: scoped_ptr<Invalidator> invalidator_;
You are tearing out invalidator from SyncManagerImpl. This means that
ProfileSyncService should not request googletalk token anymore. One less problem
for me to solve for android. Thanks.

[rlarocque, 2013-06-18 23:30:30]

I've removed the 'if (state == ..'.  

I've also updated the SyncManager's handling of invalidator auth errors.  I
doubt it will have any effect, since I don't expect we'll see any auth errors
after the connection is established.  It still seems like the right thing to do,
though.

https://codereview.chromium.org/15580002/diff/148001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/15580002/diff/148001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service.cc:260: if (state ==
syncer::INVALIDATION_CREDENTIALS_REJECTED) {
On 2013/06/18 23:00:39, pavely wrote:
> Should this be if (state != ...  ?
> 
> Also if it is CREDENTIALS_REJECTED then there should be a codepath that calls
> RequestAccessToken.

Yep, that's wrong.  I had intended to remove that if condition entirely.

I had implemented a special case for CREDENTIALS_REJECTED in my previous patch,
but an e-mail discussion with the relevant team suggests that this isn't
necessary.  They said (and my testing so far confirms) that we won't receive any
auth errors when the access token expires.

Come to think of it, I suppose this does reopen the question of whether or not
the SyncManager should do anything if the invalidator experiences an auth error.
 I guess I should disable that part of the code?  It's not really clear to me
anymore under what circumstances an Invalidations auth error might occur...

[pavely, 2013-06-18 23:33:39]

lgtm

[pavely, 2013-06-18 23:33:39]

lgtm

[rlarocque, 2013-06-18 23:43:30]

On 2013/06/18 23:33:39, pavely wrote:
> lgtm

Thanks, everyone.

This is almost ready for commit.  There's just one more CL that should be
allowed to land first: https://codereview.chromium.org/15780020/.  

Once it lands, I'll rebase and try to get this CL landed in time for M29.

[rlarocque, 2013-06-20 20:42:03]

+bauerb as FYI

The latest upload rebases on top of some ManagedUser OAuth changes (r207501).  

I've reverted the DummyInvalidator and SyncBackendHost changes from that patch. 
In their place, I've added a check to TiclInvalidationService to make sure the
service never initializes itself for managed users.  The effect should be the
same.

Assuming there are no objections, and assuming the trybots cooperate, I'd still
like to land this CL in time for M29.

[commit-bot: I haz the power, 2013-06-21 20:28:08]

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

[commit-bot: I haz the power, 2013-06-21 20:43:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[rlarocque, 2013-06-21 21:02:51]

On 2013/06/21 20:43:54, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

The script believes that msw is not sufficient for an OWNERS review of
chrome/browser/chrome_to_mobile_service*.  TBRing sky instead.

The script also wants an OWNERs stamp for some trivial changes to
chrome_browser_main_extra_parts_profiles.cc.  TBRing erg for those parts.

[commit-bot: I haz the power, 2013-06-21 21:05:47]

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

[commit-bot: I haz the power, 2013-06-22 02:28:37]

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

[commit-bot: I haz the power, 2013-06-22 02:58:41]

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

[commit-bot: I haz the power, 2013-06-22 04:42:59]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, chrome_frame_net_tests,
chrome_frame_tests, chrome_frame_unittests, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-24 17:04:08]

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

[commit-bot: I haz the power, 2013-06-24 17:04:25]

Failed to apply patch for sync/internal_api/test/fake_sync_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file sync/internal_api/test/fake_sync_manager.cc
  Hunk #4 FAILED at 156.
  Hunk #5 succeeded at 246 (offset -3 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
sync/internal_api/test/fake_sync_manager.cc.rej

Patch:       sync/internal_api/test/fake_sync_manager.cc
Index: sync/internal_api/test/fake_sync_manager.cc
diff --git a/sync/internal_api/test/fake_sync_manager.cc
b/sync/internal_api/test/fake_sync_manager.cc
index
036376fbb8b37efa1c266831a4c60ed25df0b2dd..56ab0d75332c45be1a3fdfb03d4a634900a8693c
100644
--- a/sync/internal_api/test/fake_sync_manager.cc
+++ b/sync/internal_api/test/fake_sync_manager.cc
@@ -61,25 +61,6 @@ ConfigureReason FakeSyncManager::GetAndResetConfigureReason()
{
   return reason;
 }
 
-void FakeSyncManager::Invalidate(
-    const ObjectIdInvalidationMap& invalidation_map) {
-  if (!sync_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&FakeSyncManager::InvalidateOnSyncThread,
-                 base::Unretained(this), invalidation_map))) {
-    NOTREACHED();
-  }
-}
-
-void FakeSyncManager::UpdateInvalidatorState(InvalidatorState state) {
-  if (!sync_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&FakeSyncManager::UpdateInvalidatorStateOnSyncThread,
-                 base::Unretained(this), state))) {
-    NOTREACHED();
-  }
-}
-
 void FakeSyncManager::WaitForSyncThread() {
   // Post a task to |sync_task_runner_| and block until it runs.
   base::RunLoop run_loop;
@@ -103,7 +84,6 @@ void FakeSyncManager::Init(
     ExtensionsActivityMonitor* extensions_activity_monitor,
     ChangeDelegate* change_delegate,
     const SyncCredentials& credentials,
-    scoped_ptr<Invalidator> invalidator,
     const std::string& invalidator_client_id,
     const std::string& restored_key_for_bootstrapping,
     const std::string& restored_keystore_key_for_bootstrapping,
@@ -161,31 +141,6 @@ void FakeSyncManager::UpdateCredentials(const
SyncCredentials& credentials) {
   NOTIMPLEMENTED();
 }
 
-void FakeSyncManager::UpdateEnabledTypes(ModelTypeSet types) {
-  enabled_types_ = types;
-}
-
-void FakeSyncManager::RegisterInvalidationHandler(
-    InvalidationHandler* handler) {
-  registrar_.RegisterHandler(handler);
-}
-
-void FakeSyncManager::UpdateRegisteredInvalidationIds(
-    InvalidationHandler* handler,
-    const ObjectIdSet& ids) {
-  registrar_.UpdateRegisteredIds(handler, ids);
-}
-
-void FakeSyncManager::UnregisterInvalidationHandler(
-    InvalidationHandler* handler) {
-  registrar_.UnregisterHandler(handler);
-}
-
-void FakeSyncManager::AcknowledgeInvalidation(const invalidation::ObjectId& id,
-                                              const AckHandle& ack_handle) {
-  // Do nothing.
-}
-
 void FakeSyncManager::StartSyncingNormally(
       const ModelSafeRoutingInfo& routing_info) {
   // Do nothing.
@@ -201,9 +156,9 @@ void FakeSyncManager::ConfigureSyncer(
     const base::Closure& ready_task,
     const base::Closure& retry_task) {
   last_configure_reason_ = reason;
-  ModelTypeSet enabled_types = GetRoutingInfoTypes(new_routing_info);
+  enabled_types_ = GetRoutingInfoTypes(new_routing_info);
   ModelTypeSet disabled_types = Difference(
-      ModelTypeSet::All(), enabled_types);
+      ModelTypeSet::All(), enabled_types_);
   disabled_types.RemoveAll(to_ignore);
   ModelTypeSet success_types = to_download;
   success_types.RemoveAll(configure_fail_types_);
@@ -294,20 +249,17 @@ void FakeSyncManager::RefreshTypes(ModelTypeSet types) {
   last_refresh_request_types_ = types;
 }
 
-void FakeSyncManager::InvalidateOnSyncThread(
-    const ObjectIdInvalidationMap& invalidation_map) {
-  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-  registrar_.DispatchInvalidationsToHandlers(invalidation_map);
-}
-
-void FakeSyncManager::UpdateInvalidatorStateOnSyncThread(
-    InvalidatorState state) {
-  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-  registrar_.UpdateInvalidatorState(state);
+void FakeSyncManager::OnIncomingInvalidation(
+      const ObjectIdInvalidationMap& invalidation_map) {
+  // Do nothing.
 }
 
 ModelTypeSet FakeSyncManager::GetLastRefreshRequestTypes() {
   return last_refresh_request_types_;
 }
 
+void FakeSyncManager::OnInvalidatorStateChange(InvalidatorState state) {
+  // Do nothing.
+}
+
 }  // namespace syncer

[commit-bot: I haz the power, 2013-06-24 19:54:37]

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

[commit-bot: I haz the power, 2013-06-25 00:29:37]

Change committed as 208315