Make GCMProfileService own GCMDriver, instead of deriving from it

Make GCMProfileService own GCMDriver, instead of deriving from it

Also remove several tests related to testing on neutral channel signals.
Replacement tests will be added when we switch to starting and stopping
GCM on demand in the future patch.

BUG=356716
TEST=tests updated
TBR=kalman@chromium.org,pavely@chromium.org,arv@chromium.org

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

[fgorski, 2014-05-19 17:33:32]

Mostly looks good.

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

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service_unittest.cc:113:
scoped_ptr<gcm::GCMDriver> gcm_service_;
gcm_driver_

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_profile_settings_provider.cc (right):

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_profile_settings_provider.cc:12: #include
"chrome/browser/services/gcm/gcm_driver.h"
is that needed?

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_driver.cc (right):

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_driver.cc:699: void GCMDriver::EnsureStarted() {
please make sure to check on gcm_enabled_

[jianli, 2014-05-19 18:37:19]

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

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service_unittest.cc:113:
scoped_ptr<gcm::GCMDriver> gcm_service_;
On 2014/05/19 17:33:32, fgorski wrote:
> gcm_driver_

Done.

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_profile_settings_provider.cc (right):

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_profile_settings_provider.cc:12: #include
"chrome/browser/services/gcm/gcm_driver.h"
On 2014/05/19 17:33:32, fgorski wrote:
> is that needed?

Removed.

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_driver.cc (right):

https://codereview.chromium.org/286213003/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_driver.cc:699: void GCMDriver::EnsureStarted() {
On 2014/05/19 17:33:32, fgorski wrote:
> please make sure to check on gcm_enabled_

Done.

[fgorski, 2014-05-19 20:37:08]

lgtm with nits

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/gcm/gcm_api.cc:33: const char kGCMDisabled[] =
"GCM was disabled. Please try again later.";
Why not simply
"GCM is currently disabled."
or
"GCM is disabled."

This message is showing on the console. I think "Please try again later" is
redundant.

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_driver.cc:701: // Is GCM enabled?
nit: comment is redundant

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_driver.cc:705: // Can we get an account ID from
the identity provider?
nit: Comment should be:
Is the user signed in?

[Nicolas Zea, 2014-05-19 21:04:06]

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/fake_gcm_profile_service.h:9: #include <vector>
nit: newline after

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_driver.h:116: // Returns the user name if the
profile is signed in.
nit: comment what it does if the profile is not signed in (return empty string
presumably?)

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:90: driver_.reset(new
GCMDriver(
I wonder if this entire section should be in a if !defined(OS_ANDROID) for now,
just to be safe.

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:106: driver_->Shutdown();
should we also driver_.reset() here?

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:61: GCMDriver* driver() const
{ return driver_.get(); }
I'm a bit concerned about the possibility of other services calling and using
driver() before one has been created. Do you think it makes sense to always have
an unitialized driver object we could always return, and have Initialize invoke
GCMDriver::Initialize?

Alternatively, it looks like we always immediately call Initialize after
constructing the GCMProfileService object. How about we just pass the
GCMClientFactory in the constructor, and get rid of initialize altogether
(creating everything in the constructor)? This way, driver_ will always exist.

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:1027: if (gcm_profile_service) {
this should probably be if (gcm_profile_service &&
gcm_profile_service->driver())

[jianli, 2014-05-19 23:03:48]

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/gcm/gcm_api.cc:33: const char kGCMDisabled[] =
"GCM was disabled. Please try again later.";
On 2014/05/19 20:37:09, fgorski wrote:
> Why not simply
> "GCM is currently disabled."
> or
> "GCM is disabled."
> 
> This message is showing on the console. I think "Please try again later" is
> redundant. 

Done.

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/fake_gcm_profile_service.h:9: #include <vector>
On 2014/05/19 21:04:06, Nicolas Zea wrote:
> nit: newline after

Done.

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_driver.cc:701: // Is GCM enabled?
On 2014/05/19 20:37:09, fgorski wrote:
> nit: comment is redundant

Done.

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_driver.cc:705: // Can we get an account ID from
the identity provider?
On 2014/05/19 20:37:09, fgorski wrote:
> nit: Comment should be:
> Is the user signed in?

Done.

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_driver.h:116: // Returns the user name if the
profile is signed in.
On 2014/05/19 21:04:06, Nicolas Zea wrote:
> nit: comment what it does if the profile is not signed in (return empty string
> presumably?)

Done.

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:90: driver_.reset(new
GCMDriver(
On 2014/05/19 21:04:06, Nicolas Zea wrote:
> I wonder if this entire section should be in a if !defined(OS_ANDROID) for
now,
> just to be safe.

Done.

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:106: driver_->Shutdown();
On 2014/05/19 21:04:06, Nicolas Zea wrote:
> should we also driver_.reset() here?

Done.

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:61: GCMDriver* driver() const
{ return driver_.get(); }
On 2014/05/19 21:04:06, Nicolas Zea wrote:
> I'm a bit concerned about the possibility of other services calling and using
> driver() before one has been created. Do you think it makes sense to always
have
> an unitialized driver object we could always return, and have Initialize
invoke
> GCMDriver::Initialize?
> 
> Alternatively, it looks like we always immediately call Initialize after
> constructing the GCMProfileService object. How about we just pass the
> GCMClientFactory in the constructor, and get rid of initialize altogether
> (creating everything in the constructor)? This way, driver_ will always exist.

Moved Initialize logic into constructor.

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:1027: if (gcm_profile_service) {
On 2014/05/19 21:04:06, Nicolas Zea wrote:
> this should probably be if (gcm_profile_service &&
> gcm_profile_service->driver())

Done.

[jianli, 2014-05-20 17:37:22]

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

https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:90: driver_.reset(new
GCMDriver(
On 2014/05/19 23:03:48, jianli wrote:
> On 2014/05/19 21:04:06, Nicolas Zea wrote:
> > I wonder if this entire section should be in a if !defined(OS_ANDROID) for
> now,
> > just to be safe.
> 
> Done.

Indeed we still need to create GCMDriver instance on Android since there're unit
tests running. I've updated the original code to make it look better.

[fgorski, 2014-05-20 18:54:50]

On 2014/05/20 17:37:22, jianli wrote:
>
https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
> File chrome/browser/services/gcm/gcm_profile_service.cc (right):
> 
>
https://codereview.chromium.org/286213003/diff/100001/chrome/browser/services...
> chrome/browser/services/gcm/gcm_profile_service.cc:90: driver_.reset(new
> GCMDriver(
> On 2014/05/19 23:03:48, jianli wrote:
> > On 2014/05/19 21:04:06, Nicolas Zea wrote:
> > > I wonder if this entire section should be in a if !defined(OS_ANDROID) for
> > now,
> > > just to be safe.
> > 
> > Done.
> 
> Indeed we still need to create GCMDriver instance on Android since there're
unit
> tests running. I've updated the original code to make it look better.

still lgtm

[Nicolas Zea, 2014-05-20 19:53:06]

lgtm

[jianli, 2014-05-20 19:54:55]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-20 19:56:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/286213003/160001

[jianli, 2014-05-20 20:37:12]

The CQ bit was unchecked by jianli@chromium.org

[jianli, 2014-05-20 20:49:19]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-20 20:49:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/286213003/180001

[jianli, 2014-05-20 21:21:42]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-20 21:22:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/286213003/200001

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

Change committed as 271832