Move gcm-independent parts of push messaging out of gcm namespace and directory

Move push messaging out of services/gcm into it's own place

Push messaging is a client of gcm and not a part of it. To improve layering it
should live in a seperate directory. This CL moves files matching
chrome/browser/services/gcm/push_messaging_* into chrome/browser/push_messaging/,
move the classes out of the gcm namespace. It also restrains GCMProfileService
to be the owner of PushMessagingServiceImpl. Instead it became a KeyedService.
By making it such it became easy to clean up the dependencies and ownership concerns
between push messaging and gcm.

BUG=402486
Committed: https://crrev.com/4596b516cf074ad33f7148d4a069b864487e9ff7
Cr-Commit-Position: refs/heads/master@{#319089}

[kbalazs, 2015-02-24 16:14:20]

b.kelemen@samsung.com changed reviewers:
+ johnme@chromium.org, peter@chromium.org, thakis@chromium.org

[kbalazs, 2015-02-24 16:14:21]

johnme@chromium.org: Please review changes in push_messaging*

peter@chromium.org: Please review changes in push_messaging*

thakis@chromium.org: Please review changes in chrome/

[Nico, 2015-02-24 17:09:40]

lgtm once peter and john are happy

[Peter Beverloo, 2015-02-24 18:08:38]

+Michael, Miguel, Filip, Jian: FYI

Hi Balazs, thank you for the patch!

Could you please change the BUG= associated with the CL? The one you refer to
has little to do with this. https://crbug.com/402486 would be more appropriate.

If we are doing this move - and I think it's a great idea, then I would like all
the Push Messaging-related files to leave //chrome/browser/services/gcm/. We've
been wanting to do this in order to achieve better layering.

This implies two further changes:
  (1) Remove our per-file OWNERS rules from //chrome/browser/services/gcm/
  (2) Instate a new OWNERS file in //chrome/browser/push_messaging/ listing the
same names.

Your goal seems to be to make it possible to use a non-GCM push service using
our implementation of the Push messaging API. Is this correct?

We won't be able to support all of that upstream - GCM is the only officially
supported push service in our tree. However, if we can figure out ways to
improve the quality and/or maintainability of the Push Messaging code whilst
improving your (downstream) ability to go your own way, then I'm definitely open
to having that debate :-). This CL would be a good first step in that direction.

https://codereview.chromium.org/955673004/diff/1/chrome/browser/push_messagin...
File chrome/browser/push_messaging/push_messaging_application_id.h (right):

https://codereview.chromium.org/955673004/diff/1/chrome/browser/push_messagin...
chrome/browser/push_messaging/push_messaging_application_id.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
Please don't modify the copyright years - we usually leave them alone when
moving files around.

https://codereview.chromium.org/955673004/diff/1/chrome/browser/push_messagin...
File chrome/browser/push_messaging/push_messaging_permission_context_factory.h
(right):

https://codereview.chromium.org/955673004/diff/1/chrome/browser/push_messagin...
chrome/browser/push_messaging/push_messaging_permission_context_factory.h:4:
#ifndef
CHROME_BROWSER_PUSH_MESSAGING_PUSH_MESSAGING_PERMISSION_CONTEXT_FACTORY_H_
nit: please don't remove the blank line above here.

https://codereview.chromium.org/955673004/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/955673004/diff/1/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:575:
'browser/push_messaging/push_messaging_application_id.h',
nit: please make sure that these files are alphabetized.

[kbalazs, 2015-02-24 18:33:25]

On 2015/02/24 18:08:38, Peter Beverloo wrote:
> +Michael, Miguel, Filip, Jian: FYI
> 
> Hi Balazs, thank you for the patch!
> 
> Could you please change the BUG= associated with the CL? The one you refer to
> has little to do with this. https://crbug.com/402486 would be more
appropriate.

Sure, that's what I meant, just messed up somehow.

> 
> If we are doing this move - and I think it's a great idea, then I would like
all
> the Push Messaging-related files to leave //chrome/browser/services/gcm/.
We've
> been wanting to do this in order to achieve better layering.

In this patch I'm moving all those files that are related to push messaging but
not specific to gcm. Do you think I should change the location for those files
that are actually gcm specific?

> 
> This implies two further changes:
>   (1) Remove our per-file OWNERS rules from //chrome/browser/services/gcm/
>   (2) Instate a new OWNERS file in //chrome/browser/push_messaging/ listing
the
> same names.

Sure, I will take care of that as well.

> 
> Your goal seems to be to make it possible to use a non-GCM push service using
> our implementation of the Push messaging API. Is this correct?

Yes. :)

> 
> We won't be able to support all of that upstream - GCM is the only officially
> supported push service in our tree. However, if we can figure out ways to
> improve the quality and/or maintainability of the Push Messaging code whilst
> improving your (downstream) ability to go your own way, then I'm definitely
open
> to having that debate :-). This CL would be a good first step in that
direction.

Great, we are absolutely on the same page. :)

[Peter Beverloo, 2015-02-24 18:40:27]

On 2015/02/24 18:33:25, kbalazs wrote:
> On 2015/02/24 18:08:38, Peter Beverloo wrote:
> > +Michael, Miguel, Filip, Jian: FYI
> > 
> > Hi Balazs, thank you for the patch!
> > 
> > Could you please change the BUG= associated with the CL? The one you refer
to
> > has little to do with this. https://crbug.com/402486 would be more
> appropriate.
> 
> Sure, that's what I meant, just messed up somehow.
> 
> > 
> > If we are doing this move - and I think it's a great idea, then I would like
> all
> > the Push Messaging-related files to leave //chrome/browser/services/gcm/.
> We've
> > been wanting to do this in order to achieve better layering.
> 
> In this patch I'm moving all those files that are related to push messaging
but
> not specific to gcm. Do you think I should change the location for those files
> that are actually gcm specific?

Yes. Today, the push messaging implementation is tightly coupled to GCM. Having
the files in two separate locations is only hiding that fact, and making it less
obvious what code to consider when people work on the feature.

Layering wise, consider push messaging to be a *user* of GCM, not part of GCM. I
don't think that it makes sense for any of the push_messaging_* files to be in
//chrome/browser/services/gcm/, unless they actually impact Chrome's GCM
implementation, which is not the case. (Except for the Chrome GCM-level mocks
and other testing utilities, of course.)

[kbalazs, 2015-02-25 00:14:51]

There is one complication of moving all push files. Currently the
PushMessagingServiecImpl is owned by GCMProfileService which is created via
GCMProfileServiceFactory which is a BrowserContextKeyedServiceFactory. After
PMSI it is moved out from services/gcm I guess it would not make sense anymore.
Adding new members to ProfileImpl is a no-no, so I think I should add a new
factory for PMSI. If that doesn't make sense to you please tell me.

[Peter Beverloo, 2015-02-25 00:23:59]

On 2015/02/25 00:14:51, kbalazs wrote:
> There is one complication of moving all push files. Currently the
> PushMessagingServiecImpl is owned by GCMProfileService which is created via
> GCMProfileServiceFactory which is a BrowserContextKeyedServiceFactory. After
> PMSI it is moved out from services/gcm I guess it would not make sense
anymore.
> Adding new members to ProfileImpl is a no-no, so I think I should add a new
> factory for PMSI. If that doesn't make sense to you please tell me.

SGTM - thanks for checking! There's a few lifetime peculiarities, but I think
these are handled through calling PMSI::InitializeForProfile. I'm sure John can
confirm tomorrow.

[kbalazs, 2015-02-25 20:53:02]

How do you guys test this code? Is there some public app service that can be
triggered to send push messages?

[Peter Beverloo, 2015-02-25 22:24:21]

On 2015/02/25 20:53:02, kbalazs wrote:
> How do you guys test this code? Is there some public app service that can be
> triggered to send push messages?

There's the PushMessagingBrowserTest, and unit tests for the permission context
and the application Id.

The browser tests use a fake GCM profile service, which we use to identify the
commands which are being send to GCM. Incoming messages are being send directly
to the PushMessagingServiceImpl (by manually constructing a
GCMClient::IncomingMessage).

tl;dr: We never actually hit GCM. We've considered doing so, but are worried
about the flakiness it might introduce.

[kbalazs, 2015-02-25 23:27:51]

On 2015/02/25 22:24:21, Peter Beverloo wrote:
> On 2015/02/25 20:53:02, kbalazs wrote:
> > How do you guys test this code? Is there some public app service that can be
> > triggered to send push messages?
> 
> There's the PushMessagingBrowserTest, and unit tests for the permission
context
> and the application Id.
> 
> The browser tests use a fake GCM profile service, which we use to identify the
> commands which are being send to GCM. Incoming messages are being send
directly
> to the PushMessagingServiceImpl (by manually constructing a
> GCMClient::IncomingMessage).
> 
> tl;dr: We never actually hit GCM. We've considered doing so, but are worried
> about the flakiness it might introduce.

Ah, I missed it. Actually the test has to go through a massive refactor in
accordance with the change.

[Peter Beverloo, 2015-02-25 23:55:05]

I like this a lot Balazs, thank you so much for doing this!! The explicit
dependency is much better than the if-checks in PushMessagingServiceImpl.

Sorry that it turns out more involved than you'd expected.

If it would help you, we can do this in multiple patches if you'd like? A first
one solely for moving the files, with a follow-up for the ownership and
dependency changes. Otherwise I'm happy to continue with a single, larger patch
as well, it's still reviewable.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (left):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:694:
gcm::PushMessagingServiceImpl::InitializeForProfile(this);
I think we need to keep this.

The reason that we're running some code when the profile gets initialized is
that the GCM client in Chrome doesn't actually establish a connection unless
there's an "application" (in this case, PushMessagingServiceImpl) in a state
where it can receive messages.

InitializeForProfile ensures this by reading a preference containing the number
of active registrations for said profile. If there are any, we force creation of
the PushMessagingServiceImpl and increase the push registration count, which
then adds that instance as a handler on the GCM Driver. This, in turn, may
establish a connection to GCM in case there wasn't one.

If I'm reading your refactoring correct, right now we innitialize
PushMessagingServiceImpl lazily when someone requests an instance through the
factory. This would break in non-test scenarios.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_permission_context_factory.h
(right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_permission_context_factory.h:4:
#ifndef
CHROME_BROWSER_PUSH_MESSAGING_PUSH_MESSAGING_PERMISSION_CONTEXT_FACTORY_H_
nit: blank line above here & copyright year

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.cc:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
nit: no more (c)

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.cc:16: // GCM is
not supported in incognito mode and thus push api is neither.
I don't think GCM cares anymore about incognito mode. Could you use this comment
instead?

  // The Push API is not currently supported in incognito mode. See
  // https://crbug.com/401439.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31:
"PushMessagingProfileService",
nit: two more spaces of indent here.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_factory.h (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.h:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
nit: we don't use (c) anymore.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:159: // Shutdown
should come before and it removes us from the list of app
nit: s/Shutdown/Shutdown()/ to clarify that it's about the Shutdown() method,
and not the concept of shutting down.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:737:
kPushMessagingApplicationIdPrefix);
nit: this would fit on a single line

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.h:159:
gcm::GCMProfileService* gcm_profile_service_;
Could you add a comment that it's safe to hold on to this (weak) pointer because
the factory defines a dependency?

  // PushMessagingServiceFactory defines a dependency on the GCMProfileService,
  // guaranteeing that GCMProfileService will outlive this class.

[kbalazs, 2015-02-27 22:40:36]

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (left):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:694:
gcm::PushMessagingServiceImpl::InitializeForProfile(this);
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> I think we need to keep this.
> 
> The reason that we're running some code when the profile gets initialized is
> that the GCM client in Chrome doesn't actually establish a connection unless
> there's an "application" (in this case, PushMessagingServiceImpl) in a state
> where it can receive messages.
> 
> InitializeForProfile ensures this by reading a preference containing the
number
> of active registrations for said profile. If there are any, we force creation
of
> the PushMessagingServiceImpl and increase the push registration count, which
> then adds that instance as a handler on the GCM Driver. This, in turn, may
> establish a connection to GCM in case there wasn't one.
> 
> If I'm reading your refactoring correct, right now we innitialize
> PushMessagingServiceImpl lazily when someone requests an instance through the
> factory. This would break in non-test scenarios.

Yes, I missed it. I put it back.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_permission_context_factory.h
(right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_permission_context_factory.h:4:
#ifndef
CHROME_BROWSER_PUSH_MESSAGING_PUSH_MESSAGING_PERMISSION_CONTEXT_FACTORY_H_
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> nit: blank line above here & copyright year

Done.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.cc:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> nit: no more (c)

Done.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.cc:16: // GCM is
not supported in incognito mode and thus push api is neither.
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> I don't think GCM cares anymore about incognito mode. Could you use this
comment
> instead?
> 
>   // The Push API is not currently supported in incognito mode. See
>   // https://crbug.com/401439.

Done.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31:
"PushMessagingProfileService",
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> nit: two more spaces of indent here.

Done.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_factory.h (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_factory.h:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> nit: we don't use (c) anymore.

Done.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:159: // Shutdown
should come before and it removes us from the list of app
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> nit: s/Shutdown/Shutdown()/ to clarify that it's about the Shutdown() method,
> and not the concept of shutting down.

Done.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:737:
kPushMessagingApplicationIdPrefix);
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> nit: this would fit on a single line

Done.

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/955673004/diff/20001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.h:159:
gcm::GCMProfileService* gcm_profile_service_;
On 2015/02/25 23:55:05, Peter Beverloo wrote:
> Could you add a comment that it's safe to hold on to this (weak) pointer
because
> the factory defines a dependency?
> 
>   // PushMessagingServiceFactory defines a dependency on the
GCMProfileService,
>   // guaranteeing that GCMProfileService will outlive this class.

Done.

[kbalazs, 2015-02-27 22:56:05]

The test was a bit tricky. Previously SetTestingFactoryAndUse was used to
override the gcm profile service factory to create FakeGCMProfileService. The
problem is that it requires the profile (BrowserContext) to be ready. But when
it is ready it is too late as we create the PushMessagingServiceImpl during
ProfileImpl initialization (and by the way PushMessagingMessageFilter also
triggers creation when it caches GetPushEndpoint() from it's constructor). So I
added one more static hook into GCMProfileServiceFactory to override the factory
method from the beginning for all profiles.

[kbalazs, 2015-03-03 01:10:10]

The test needs more work. Problem is that some tests use
KeyedService::SetTestingFactory which shuts down the service but it does that
without handling the dependency graph (i.e. it does not shuts down dependent
services before). Therefore my assumption that if the push factory depends on
the gcm one implies that the gcm driver cannot be shut down before push service
does not always hold.

[Peter Beverloo, 2015-03-03 15:54:18]

Perhaps it's OK to only emit a warning, mentioning that behavior in tests is
expected to be slightly different? (Unless we can accurately determine that we
are in a test.)

I've asked John to take a look from a lifetime POV.

https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:51: virtual
FakeGCMProfileService* AsFakeGCMProfileService();
I'm not a huge fan of this. Could we perhaps just static_cast<> if we really
have to?

https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:23:
global_testing_factory = NULL;
nit: g_testing_factory

https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:66: }
nit: no need for {}

[johnme, 2015-03-03 17:59:19]

On 2015/03/03 01:10:10, kbalazs wrote:
> The test needs more work. Problem is that some tests use
> KeyedService::SetTestingFactory which shuts down the service but it does that
> without handling the dependency graph (i.e. it does not shuts down dependent
> services before). Therefore my assumption that if the push factory depends on
> the gcm one implies that the gcm driver cannot be shut down before push
service
> does not always hold.

Good point. I think it's fine to break the assumption that GCMDriver cannot be
shut down before PushMessagingServiceImpl (see concrete suggestion below). I
think the only consequence would be if
PushMessagingServiceImpl::InitializeForProfile had added app handlers to the
GCMDriver, then it got swapped out with a fake one by a test, then those app
handlers wouldn't get re-created. But this only happens during tests, and we
always have a clean profile in our current push tests, so
kPushMessagingRegistrationCount will always be zero and app handlers won't be
added until a registration attempt is made.

https://codereview.chromium.org/955673004/diff/100001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/955673004/diff/100001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:770:
gcm::GCMDriver* driver = gcm_profile_service_->driver();
To resolve the dependency lifetimes issue, you could use
gcm::GCMProfileServiceFactory::GetForProfile(profile_) here instead, and remove
the gcm_profile_service_ member variable.

It'll have negligible overhead, since this method only gets called during
relatively rare (and expensive) events like Register and Unregister.

[kbalazs, 2015-03-03 22:45:11]

On 2015/03/03 17:59:19, johnme wrote:
> On 2015/03/03 01:10:10, kbalazs wrote:
> > The test needs more work. Problem is that some tests use
> > KeyedService::SetTestingFactory which shuts down the service but it does
that
> > without handling the dependency graph (i.e. it does not shuts down dependent
> > services before). Therefore my assumption that if the push factory depends
on
> > the gcm one implies that the gcm driver cannot be shut down before push
> service
> > does not always hold.
> 
> Good point. I think it's fine to break the assumption that GCMDriver cannot be
> shut down before PushMessagingServiceImpl (see concrete suggestion below). I
> think the only consequence would be if
> PushMessagingServiceImpl::InitializeForProfile had added app handlers to the
> GCMDriver, then it got swapped out with a fake one by a test, then those app
> handlers wouldn't get re-created. But this only happens during tests, and we
> always have a clean profile in our current push tests, so
> kPushMessagingRegistrationCount will always be zero and app handlers won't be
> added until a registration attempt is made.
> 
>
https://codereview.chromium.org/955673004/diff/100001/chrome/browser/push_mes...
> File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):
> 
>
https://codereview.chromium.org/955673004/diff/100001/chrome/browser/push_mes...
> chrome/browser/push_messaging/push_messaging_service_impl.cc:770:
> gcm::GCMDriver* driver = gcm_profile_service_->driver();
> To resolve the dependency lifetimes issue, you could use
> gcm::GCMProfileServiceFactory::GetForProfile(profile_) here instead, and
remove
> the gcm_profile_service_ member variable.

That's a great idea. It seems like it solves all problems.

[kbalazs, 2015-03-03 22:54:30]

On 2015/03/03 15:54:18, Peter Beverloo wrote:
> Perhaps it's OK to only emit a warning, mentioning that behavior in tests is
> expected to be slightly different? (Unless we can accurately determine that we
> are in a test.)

I think there is no need for warning with the latest patch. We can just expect
the gcm driver is there. It can only be shut down temporally during unit tests
but we won't even observe that.

> 
> I've asked John to take a look from a lifetime POV.
> 
>
https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
> File chrome/browser/services/gcm/gcm_profile_service.h (right):
> 
>
https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
> chrome/browser/services/gcm/gcm_profile_service.h:51: virtual
> FakeGCMProfileService* AsFakeGCMProfileService();
> I'm not a huge fan of this. Could we perhaps just static_cast<> if we really
> have to?

Sure, I wanted to be paranoid we don't have to.

> 
>
https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
> File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):
> 
>
https://codereview.chromium.org/955673004/diff/100001/chrome/browser/services...
> chrome/browser/services/gcm/gcm_profile_service_factory.cc:23:
> global_testing_factory = NULL;
> nit: g_testing_factory

I was able to remove this.

[Peter Beverloo, 2015-03-03 23:04:57]

Awesome! Thanks for doing this! :-)

lgtm

https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:116:
PushMessagingServiceFactory::GetForProfile(profile));
PushMessagingServiceFactory::GetForProfile() returns a
PushMessagingServiceImpl*. Perhaps you meant for that to return a
content::PushMessagingService* instead?

[kbalazs, 2015-03-03 23:57:26]

https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:116:
PushMessagingServiceFactory::GetForProfile(profile));
On 2015/03/03 23:04:56, Peter Beverloo wrote:
> PushMessagingServiceFactory::GetForProfile() returns a
> PushMessagingServiceImpl*. Perhaps you meant for that to return a
> content::PushMessagingService* instead?

I was doing that before but I changed my mind, but forgot to remove this cast.
I'll take care of that before landing after the bots turn green.

[kbalazs, 2015-03-04 16:10:40]

The CQ bit was checked by b.kelemen@samsung.com

[kbalazs, 2015-03-04 16:10:41]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
peter@chromium.org
Link to the patchset: https://codereview.chromium.org/955673004/#ps140001
(title: "remove redundant cast")

[commit-bot: I haz the power, 2015-03-04 16:12:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/955673004/140001

[commit-bot: I haz the power, 2015-03-04 16:22:40]

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

[commit-bot: I haz the power, 2015-03-04 16:22:41]

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

[kbalazs, 2015-03-04 16:24:41]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2015-03-04 16:25:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/955673004/140001

[commit-bot: I haz the power, 2015-03-04 16:32:41]

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

[commit-bot: I haz the power, 2015-03-04 16:32:42]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Peter Beverloo, 2015-03-04 17:43:57]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-03-04 17:45:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/955673004/140001

[commit-bot: I haz the power, 2015-03-04 18:43:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-04 18:44:32]

Patchset 8 (id:??) landed as
https://crrev.com/4596b516cf074ad33f7148d4a069b864487e9ff7
Cr-Commit-Position: refs/heads/master@{#319089}