Make invalidations work for Chrome OS Kiosk Apps.

Make invalidations work for Chrome OS Kiosk Apps.

Kiosk Apps don't have a user logged in, so SigninManager and
ProfileOAuth2TokenService don't work. However, Kiosk Apps have access
to a device-level robot account for identity. Wire this up with the
invalidation code by breaking out the authentication-related pieces
into a separate interface and providing implementations both for the
regular Profile case as well as Kiosk Apps. The appropriate
implementation gets selected by InvalidationServiceFactory.

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

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

[Mattias Nissler (ping if slow), 2014-02-25 13:26:35]

Folks, here's an initial stab at breaking the currently rather strong ties
between the chrome/browser/invalidation code and Profile.

I have two motivations for doing this:

1. The immediate need to solve http://crbug.com/269455, which revolves around
Push Messaging API vs. Chrome OS Kiosk App mode (the infrastructure
Chromebox-for-meetings is built around). In this mode, we do have a Profile but
OAuth2 tokens don't come from ProfileOAuth2TokenService, but from
DeviceOAuth2TokenService.

2. Eventually, we'd like to be able to instantiate an InvalidationService
instance that is not tied to specific Profile, so we can get invalidations for
Chrome OS device policy (i.e. admin changes device-level settings on Google Apps
control panel, device picks this up immediately). This CL gets us a bit closer
to that goal by addressing the OAuth story; further work is needed for
URLRequestContext, InvalidatorStorage etc.

Finally, note that this CL is not complete yet. It's good enough to get the Push
Messaging API working for Kiosk Apps (I've tested that), but it probably needs
more work to handle FakeInvalidationService properly and to do the right thing
for GCMNetworkChannelDelegateImpl.

Before I spend more effort on sorting these out, I wanted to get this out to you
so you can take an initial look and tell me whether you're OK with the approach
and sanity-check whether I've missed any obvious pitfalls. Let me know what you
think.

Thanks!

[rlarocque, 2014-02-25 18:29:08]

+Pavel

This may be relevant to the componentization efforts.  It looks like this change
is headed in the right direction.  Pavel knows more about it than I do, so I'll
let him comment further.

https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
File chrome/browser/invalidation/p2p_invalidation_service.h (right):

https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/p2p_invalidation_service.h:30:
scoped_ptr<InvalidationAuthProvider> auth_provider);
This may be unnecessary.  The P2P Invalidation Service is used only for sync
integration tests.  I don't think any of them depend on authentication.

[Mattias Nissler (ping if slow), 2014-02-26 09:47:08]

Sounds like I'm not totally off track then ;)

Pavel, let me know your thoughts on this so I can move forward to solve
mentioned bug within the M35 time frame.

https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
File chrome/browser/invalidation/p2p_invalidation_service.h (right):

https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/p2p_invalidation_service.h:30:
scoped_ptr<InvalidationAuthProvider> auth_provider);
On 2014/02/25 18:29:08, rlarocque wrote:
> This may be unnecessary.  The P2P Invalidation Service is used only for sync
> integration tests.  I don't think any of them depend on authentication.

OK, good to know. I'll see whether I can get away with returning NULL from
GetInvalidationAuthProvider() then.

[pavely, 2014-02-27 02:34:55]

On 2014/02/26 09:47:08, Mattias Nissler wrote:
> Sounds like I'm not totally off track then ;)
> 
> Pavel, let me know your thoughts on this so I can move forward to solve
> mentioned bug within the M35 time frame.
> 
>
https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
> File chrome/browser/invalidation/p2p_invalidation_service.h (right):
> 
>
https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
> chrome/browser/invalidation/p2p_invalidation_service.h:30:
> scoped_ptr<InvalidationAuthProvider> auth_provider);
> On 2014/02/25 18:29:08, rlarocque wrote:
> > This may be unnecessary.  The P2P Invalidation Service is used only for sync
> > integration tests.  I don't think any of them depend on authentication.
> 
> OK, good to know. I'll see whether I can get away with returning NULL from
> GetInvalidationAuthProvider() then.

I've looked at the things you are changing, this looks right. ShowLoginUI looks
out of place, but that's fine, push_messaging_api needs it.

The problem with GCMNetworkChannelDelegateImpl is that it uses
ProfileOAuth2TokenServiceRequest to jump to UI thread and that implies
ProfileOAuth2TokenService. 
In the change I'm working on now I'm heading in direction of creating long lived
object for GCMNetworkChannelDelegateImpl that lives on UI thread. This would
allow to call standard OAuth2TokenService::StartRequest so you could use
InvalidationAuthProvider. The thing to pay attention is to make sure
OAuth2TokenService pointer is still valid by the time call reaches UI thread.

I'll need to do line-by-line pass of this CL to check for small things.

[Mattias Nissler (ping if slow), 2014-02-27 09:06:15]

On 2014/02/27 02:34:55, pavely wrote:
> On 2014/02/26 09:47:08, Mattias Nissler wrote:
> > Sounds like I'm not totally off track then ;)
> > 
> > Pavel, let me know your thoughts on this so I can move forward to solve
> > mentioned bug within the M35 time frame.
> > 
> >
>
https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
> > File chrome/browser/invalidation/p2p_invalidation_service.h (right):
> > 
> >
>
https://codereview.chromium.org/179843002/diff/1/chrome/browser/invalidation/...
> > chrome/browser/invalidation/p2p_invalidation_service.h:30:
> > scoped_ptr<InvalidationAuthProvider> auth_provider);
> > On 2014/02/25 18:29:08, rlarocque wrote:
> > > This may be unnecessary.  The P2P Invalidation Service is used only for
sync
> > > integration tests.  I don't think any of them depend on authentication.
> > 
> > OK, good to know. I'll see whether I can get away with returning NULL from
> > GetInvalidationAuthProvider() then.
> 
> I've looked at the things you are changing, this looks right. ShowLoginUI
looks
> out of place, but that's fine, push_messaging_api needs it.

Yeah, I had that initial bad feeling about ShowLoginUI as well. I then
rationalized it makes sense to have it though, because an abstracted way of
obtaining auth tokens doesn't make much sense if it doesn't cover the trigger to
get the browser into a state in which it actually can issue tokens.

> 
> The problem with GCMNetworkChannelDelegateImpl is that it uses
> ProfileOAuth2TokenServiceRequest to jump to UI thread and that implies
> ProfileOAuth2TokenService. 
> In the change I'm working on now I'm heading in direction of creating long
lived
> object for GCMNetworkChannelDelegateImpl that lives on UI thread. This would
> allow to call standard OAuth2TokenService::StartRequest so you could use
> InvalidationAuthProvider. The thing to pay attention is to make sure
> OAuth2TokenService pointer is still valid by the time call reaches UI thread.

That sounds great! Do you have an ETA? If I'm not missing something,
GCM_NETWORK_CHANNEL isn't used in production code right now (what's the plan for
it btw.?). Given that, I could just get this change ready without updating
GCMNetworkChannelDelegateImpl for now, and we could then take care of
GCMNetworkChannelDelegateImpl once both of our changes are in. Would that work
for you?

> 
> I'll need to do line-by-line pass of this CL to check for small things.

Right. Let me fix up tests and stuff before that though. I'll ping the review
once the code is ready for full review.

[Mattias Nissler (ping if slow), 2014-03-07 08:30:36]

This is now ready for reviewing. Please take a look.

[pavely, 2014-03-07 23:43:28]

LGTM with nits. I'm not a commiter, you'll need to get somebody else's lgtm.

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/invalidation_auth_provider.h (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidation_auth_provider.h:45:
InvalidationAuthProvider();
I don't understand reason for making ctor protected. Is it to prohibit creating
instances of this class?

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/profile_invalidation_auth_provider.cc (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.cc:8: #include
"chrome/browser/profiles/profile.h"
I think you don't need chrome_notification_types.h and profile.h

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/profile_invalidation_auth_provider.h (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.h:12: #include
"content/public/browser/notification_registrar.h"
Do you need to include notification_registrar.h?

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.h:15: class
Profile;
I think it will compile without Profile declaration.

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.h:36: //
SigninManagerBase:
// SigninManagerBase::Observer:

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_invalidation_service_unittest.cc (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service_unittest.cc:34: NULL,
token_service_.get(), NULL, profile_.get())),
This shouldn't compile, ProfileInvalidationAuthProvider::ctor takes 3 params.

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

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_test.cc:122: profile)));
Same as other unittest. Make sure it compiles.

https://codereview.chromium.org/179843002/diff/110001/chrome/browser/invalida...
File chrome/browser/invalidation/gcm_invalidation_bridge.h (right):

https://codereview.chromium.org/179843002/diff/110001/chrome/browser/invalida...
chrome/browser/invalidation/gcm_invalidation_bridge.h:16: class Profile;
No need for Profile after this change.

[Mattias Nissler (ping if slow), 2014-03-08 01:30:07]

Thanks for the review.

rlarocque, dcheng, can you take a look?

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/invalidation_auth_provider.h (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidation_auth_provider.h:45:
InvalidationAuthProvider();
On 2014/03/07 23:43:29, pavely wrote:
> I don't understand reason for making ctor protected. Is it to prohibit
creating
> instances of this class?

You can't create them anyways (it has pure virtual member functions), so the
ctor is only useful to derived classes, and declaring it protected seemed most
consistent with that :)

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/profile_invalidation_auth_provider.cc (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.cc:8: #include
"chrome/browser/profiles/profile.h"
On 2014/03/07 23:43:29, pavely wrote:
> I think you don't need chrome_notification_types.h and profile.h

Done.

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/profile_invalidation_auth_provider.h (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.h:12: #include
"content/public/browser/notification_registrar.h"
On 2014/03/07 23:43:29, pavely wrote:
> Do you need to include notification_registrar.h?

No, this are leftovers. Removed.

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.h:15: class
Profile;
On 2014/03/07 23:43:29, pavely wrote:
> I think it will compile without Profile declaration.

Done.

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/profile_invalidation_auth_provider.h:36: //
SigninManagerBase:
On 2014/03/07 23:43:29, pavely wrote:
> // SigninManagerBase::Observer:

Done.

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_invalidation_service_unittest.cc (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service_unittest.cc:34: NULL,
token_service_.get(), NULL, profile_.get())),
On 2014/03/07 23:43:29, pavely wrote:
> This shouldn't compile, ProfileInvalidationAuthProvider::ctor takes 3 params.

Yes, I've fixed this meanwhile - failed to compile sync_tests before uploading
the code.

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

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_test.cc:122: profile)));
On 2014/03/07 23:43:29, pavely wrote:
> Same as other unittest. Make sure it compiles.

Done.

https://codereview.chromium.org/179843002/diff/110001/chrome/browser/invalida...
File chrome/browser/invalidation/gcm_invalidation_bridge.h (right):

https://codereview.chromium.org/179843002/diff/110001/chrome/browser/invalida...
chrome/browser/invalidation/gcm_invalidation_bridge.h:16: class Profile;
On 2014/03/07 23:43:29, pavely wrote:
> No need for Profile after this change.

Done.

[rlarocque, 2014-03-08 01:36:38]

lgtm

[pavely, 2014-03-08 23:35:17]

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
File chrome/browser/invalidation/invalidation_auth_provider.h (right):

https://codereview.chromium.org/179843002/diff/90001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidation_auth_provider.h:45:
InvalidationAuthProvider();
On 2014/03/08 01:30:07, Mattias Nissler wrote:
> On 2014/03/07 23:43:29, pavely wrote:
> > I don't understand reason for making ctor protected. Is it to prohibit
> creating
> > instances of this class?
> 
> You can't create them anyways (it has pure virtual member functions), so the
> ctor is only useful to derived classes, and declaring it protected seemed most
> consistent with that :)

You could just have no ctor at all, less lines for reader to read. I leave it up
to you to leave it the way it is or change it.

[Mattias Nissler (ping if slow), 2014-03-18 11:25:08]

bartfab@ to CC

dcheng@: Do you have input on this?

Status on this is that I got sidetracked again, hoping to get back to this to do
a final check whether all is good and submit it later this week or early next
week.

[Mattias Nissler (ping if slow), 2014-03-20 13:58:33]

+fgorski for chrome/browser/services/gcm
+yoz for chrome/browser/extensions/api/push_messaging because dcheng@ is
unresponsive

I've rebased and tested that this works as expected (sync and push messaging
still receive invalidations), so I'd like to land this rather sooner than later.

[fgorski, 2014-03-20 16:34:07]

jianli@ will take care of the gcm_profile_service.

[jianli, 2014-03-20 16:47:30]

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:546: DCHECK(!profile ||
!profile->IsOffTheRecord());
GCMProfileService depends on profile. Even you make this DCHECK work for the
case without profile, you will get the crash later on when profile is accessed.

[Mattias Nissler (ping if slow), 2014-03-20 16:51:25]

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:546: DCHECK(!profile ||
!profile->IsOffTheRecord());
On 2014/03/20 16:47:30, jianli wrote:
> GCMProfileService depends on profile. Even you make this DCHECK work for the
> case without profile, you will get the crash later on when profile is
accessed.

I added this just to get FakeGCMProfileService in
gcm_invalidation_bridge_unittest.cc going, which needs to call the ctor but
doesn't actually rely on having a valid Profile.

I prefer tests to work with out the heavy-weight dependency to TestingProfile
where possible, but I'm not going to complain if you tell me to spin up a
TestingProfile just for getting past this DCHECK :)

[jianli, 2014-03-20 16:56:28]

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:546: DCHECK(!profile ||
!profile->IsOffTheRecord());
On 2014/03/20 16:51:26, Mattias Nissler wrote:
> On 2014/03/20 16:47:30, jianli wrote:
> > GCMProfileService depends on profile. Even you make this DCHECK work for the
> > case without profile, you will get the crash later on when profile is
> accessed.
> 
> I added this just to get FakeGCMProfileService in
> gcm_invalidation_bridge_unittest.cc going, which needs to call the ctor but
> doesn't actually rely on having a valid Profile.
> 
> I prefer tests to work with out the heavy-weight dependency to TestingProfile
> where possible, but I'm not going to complain if you tell me to spin up a
> TestingProfile just for getting past this DCHECK :)

Skipping the check in GCMProfileService constructor sounds more like hack to me.
I think it is better to create a TestingProfile in order to make the test be
close to the real scenarion as much as possible.

[Mattias Nissler (ping if slow), 2014-03-20 17:07:37]

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/179843002/diff/190001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:546: DCHECK(!profile ||
!profile->IsOffTheRecord());
On 2014/03/20 16:56:28, jianli wrote:
> On 2014/03/20 16:51:26, Mattias Nissler wrote:
> > On 2014/03/20 16:47:30, jianli wrote:
> > > GCMProfileService depends on profile. Even you make this DCHECK work for
the
> > > case without profile, you will get the crash later on when profile is
> > accessed.
> > 
> > I added this just to get FakeGCMProfileService in
> > gcm_invalidation_bridge_unittest.cc going, which needs to call the ctor but
> > doesn't actually rely on having a valid Profile.
> > 
> > I prefer tests to work with out the heavy-weight dependency to
TestingProfile
> > where possible, but I'm not going to complain if you tell me to spin up a
> > TestingProfile just for getting past this DCHECK :)
> 
> Skipping the check in GCMProfileService constructor sounds more like hack to
me.
> I think it is better to create a TestingProfile in order to make the test be
> close to the real scenarion as much as possible.

I'd agree if we were talking about an integration test that exercises
GCMInvalidationBridge in context, but this is a unit test that is using a *fake*
implementation of GCMProfileService. Thus, there can't be any expectation of it
resembling any real-life scenario.

Anyways, I'm not going to fight over this. I'll upload a CL that restores the
TestingProfile boilerplate in a second.

[Mattias Nissler (ping if slow), 2014-03-20 17:24:18]

OK, new version is up.

chrome/browser/services/gcm is no longer part of the CL.

[dcheng, 2014-03-20 17:33:35]

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:14: #include
"chrome/browser/browser_process.h"
What is this include for?

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:36: class
ObfuscatedGaiaIdFetcher;
Do we need this forward declare at all, since we include the header on line 14?

[Mattias Nissler (ping if slow), 2014-03-20 17:40:08]

Comments addressed.

dcheng@: PTAL

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:14: #include
"chrome/browser/browser_process.h"
On 2014/03/20 17:33:36, dcheng wrote:
> What is this include for?

Development artifact. Removed.

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:36: class
ObfuscatedGaiaIdFetcher;
On 2014/03/20 17:33:36, dcheng wrote:
> Do we need this forward declare at all, since we include the header on line
14?

Nope, good point. I had just mechanically alphabetized declaration order.

[dcheng, 2014-03-20 17:42:05]

On 2014/03/20 17:40:08, Mattias Nissler wrote:
> Comments addressed.
> 
> dcheng@: PTAL
> 
>
https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
> File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
(right):
> 
>
https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
> chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:14:
#include
> "chrome/browser/browser_process.h"
> On 2014/03/20 17:33:36, dcheng wrote:
> > What is this include for?
> 
> Development artifact. Removed.
> 
>
https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
> File chrome/browser/extensions/api/push_messaging/push_messaging_api.h
(right):
> 
>
https://codereview.chromium.org/179843002/diff/210001/chrome/browser/extensio...
> chrome/browser/extensions/api/push_messaging/push_messaging_api.h:36: class
> ObfuscatedGaiaIdFetcher;
> On 2014/03/20 17:33:36, dcheng wrote:
> > Do we need this forward declare at all, since we include the header on line
> 14?
> 
> Nope, good point. I had just mechanically alphabetized declaration order.

c/b/e/api/push_messaging/* LGTM

[xiyuan, 2014-03-20 20:07:42]

c/b/chromeos/chrome_browser_main_chromeos.cc LGTM

[Mattias Nissler (ping if slow), 2014-03-20 20:08:54]

The CQ bit was checked by mnissler@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/179843002/230001

[commit-bot: I haz the power, 2014-03-20 20:10:22]

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

[commit-bot: I haz the power, 2014-03-20 20:10:22]

Failed to apply patch for
chrome/browser/invalidation/gcm_invalidation_bridge.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/invalidation/gcm_invalidation_bridge.cc
  Hunk #1 FAILED at 7.
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/invalidation/gcm_invalidation_bridge.cc.rej

Patch:       chrome/browser/invalidation/gcm_invalidation_bridge.cc
Index: chrome/browser/invalidation/gcm_invalidation_bridge.cc
diff --git a/chrome/browser/invalidation/gcm_invalidation_bridge.cc
b/chrome/browser/invalidation/gcm_invalidation_bridge.cc
index
d9a7e5149c4c95ca5aae0699edc22c011436f884..76df06d00449722c89d46acc2984cec4d61b9f98
100644
--- a/chrome/browser/invalidation/gcm_invalidation_bridge.cc
+++ b/chrome/browser/invalidation/gcm_invalidation_bridge.cc
@@ -7,8 +7,8 @@
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "chrome/browser/invalidation/gcm_invalidation_bridge.h"
+#include "chrome/browser/invalidation/invalidation_auth_provider.h"
 #include "chrome/browser/services/gcm/gcm_profile_service.h"
-#include "chrome/browser/services/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/signin/profile_oauth2_token_service.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
@@ -147,20 +147,17 @@ void GCMInvalidationBridge::Core::OnIncomingMessage(
   message_callback_.Run(message, echo_token);
 }
 
-GCMInvalidationBridge::GCMInvalidationBridge(Profile* profile)
+GCMInvalidationBridge::GCMInvalidationBridge(
+    gcm::GCMProfileService* gcm_profile_service,
+    InvalidationAuthProvider* auth_provider)
     : OAuth2TokenService::Consumer("gcm_network_channel"),
-      profile_(profile),
-      subscribed_for_incoming_messages_(false),
+      gcm_profile_service_(gcm_profile_service),
+      auth_provider_(auth_provider),
       weak_factory_(this) {}
 
 GCMInvalidationBridge::~GCMInvalidationBridge() {
-  if (subscribed_for_incoming_messages_) {
-    gcm::GCMProfileService* gcm_profile_service =
-        gcm::GCMProfileServiceFactory::GetForProfile(profile_);
-    DCHECK(gcm_profile_service);
-
-    gcm_profile_service->RemoveAppHandler(kInvalidationsAppId);
-  }
+  if (subscribed_for_incoming_messages_)
+    gcm_profile_service_->RemoveAppHandler(kInvalidationsAppId);
 }
 
 scoped_ptr<syncer::GCMNetworkChannelDelegate>
@@ -194,15 +191,11 @@ void GCMInvalidationBridge::RequestToken(
                    error,
                    access_token));
   }
-  ProfileOAuth2TokenService* token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
   request_token_callback_ = callback;
-  SigninManagerBase* signin_manager =
-      SigninManagerFactory::GetForProfile(profile_);
-  std::string account_id = signin_manager->GetAuthenticatedAccountId();
   OAuth2TokenService::ScopeSet scopes;
   scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope);
-  access_token_request_ = token_service->StartRequest(account_id, scopes,
this);
+  access_token_request_ = auth_provider_->GetTokenService()->StartRequest(
+      auth_provider_->GetAccountId(), scopes, this);
 }
 
 void GCMInvalidationBridge::OnGetTokenSuccess(
@@ -239,29 +232,23 @@ void GCMInvalidationBridge::OnGetTokenFailure(
 }
 
 void GCMInvalidationBridge::InvalidateToken(const std::string& token) {
-  ProfileOAuth2TokenService* token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
   DCHECK(CalledOnValidThread());
-  SigninManagerBase* signin_manager =
-      SigninManagerFactory::GetForProfile(profile_);
-  std::string account_id = signin_manager->GetAuthenticatedAccountId();
   OAuth2TokenService::ScopeSet scopes;
   scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope);
-  token_service->InvalidateToken(account_id, scopes, token);
+  auth_provider_->GetTokenService()->InvalidateToken(
+      auth_provider_->GetAccountId(), scopes, token);
 }
 
 void GCMInvalidationBridge::Register(
     syncer::GCMNetworkChannelDelegate::RegisterCallback callback) {
   DCHECK(CalledOnValidThread());
   // No-op if GCMClient is disabled.
-  gcm::GCMProfileService* gcm_profile_service =
-      gcm::GCMProfileServiceFactory::GetForProfile(profile_);
-  if (gcm_profile_service == NULL)
+  if (gcm_profile_service_ == NULL)
     return;
 
   std::vector<std::string> sender_ids;
   sender_ids.push_back(kInvalidationsSenderId);
-  gcm_profile_service->Register(
+  gcm_profile_service_->Register(
       kInvalidationsAppId,
       sender_ids,
       base::Bind(&GCMInvalidationBridge::RegisterFinished,
@@ -285,13 +272,11 @@ void GCMInvalidationBridge::RegisterFinished(
 
 void GCMInvalidationBridge::SubscribeForIncomingMessages() {
   // No-op if GCMClient is disabled.
-  gcm::GCMProfileService* gcm_profile_service =
-      gcm::GCMProfileServiceFactory::GetForProfile(profile_);
-  if (gcm_profile_service == NULL)
+  if (gcm_profile_service_ == NULL)
     return;
 
   DCHECK(!subscribed_for_incoming_messages_);
-  gcm_profile_service->AddAppHandler(kInvalidationsAppId, this);
+  gcm_profile_service_->AddAppHandler(kInvalidationsAppId, this);
   subscribed_for_incoming_messages_ = true;
 }

[Mattias Nissler (ping if slow), 2014-03-20 21:10:00]

The CQ bit was checked by mnissler@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:11:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/179843002/250001

[commit-bot: I haz the power, 2014-03-20 23:36:09]

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

[commit-bot: I haz the power, 2014-03-20 23:36:10]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Mattias Nissler (ping if slow), 2014-03-21 09:34:34]

The CQ bit was checked by mnissler@chromium.org

[commit-bot: I haz the power, 2014-03-21 09:34:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/179843002/250001

[commit-bot: I haz the power, 2014-03-21 11:22:36]

Change committed as 258522

[Mattias Nissler (ping if slow), 2014-03-21 14:30:56]

Re-landing after revert with the line in gcm_invalidation_bridge.cc that got
dropped accidentally in a merge (leading to uninitialized memory access)
restored.

[Mattias Nissler (ping if slow), 2014-03-21 14:31:03]

The CQ bit was checked by mnissler@chromium.org

[commit-bot: I haz the power, 2014-03-21 14:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/179843002/270001

[commit-bot: I haz the power, 2014-03-21 15:05:11]

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

[commit-bot: I haz the power, 2014-03-21 15:05:12]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[Mattias Nissler (ping if slow), 2014-03-21 15:21:39]

The CQ bit was checked by mnissler@chromium.org

[commit-bot: I haz the power, 2014-03-21 15:21:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/179843002/270001

[commit-bot: I haz the power, 2014-03-21 17:46:48]

Change committed as 258623