[GCM] Stop/restart GCM when the state is forced off/on

[GCM] Stop/restart GCM when the state is forced off/on

BUG=348642
TEST=new tests added
R=dimich@chromium.org, fgorski@chromium.org, tim@chromium.org

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

[jianli, 2014-03-04 00:55:59]

Tim for chrome/browser/sync changes.
Filip and Dmitry for everything.

[Dmitry Titov, 2014-03-04 02:47:51]

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

https://codereview.chromium.org/183013007/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_profile_service.cc:267: void Load(const
base::WeakPtr<GCMProfileService>& service);
Note: If you rename GCMClient::Load -> Start, you will need to rename this too,
and perhaps GCMProfileService::EnsureLoaded() as well...

https://codereview.chromium.org/183013007/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/183013007/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:995: if
(experiments.gcm_channel_state != syncer::Experiments::UNSET) {
What if it goes SUPPRESSED->UNSET? It seems it won't be enabled then even if
there is an app that uses GCM...

Maybe it'd be more readable if there were 
GCMProfileService::SetEnabledState(GCMEnabledState state), since the existing
GCMEnabledState already has all the possible ways the service can operate in,
and it corresponds 1:1 to values in experiment...

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/gcm_client.h
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/gcm_client.h...
google_apis/gcm/gcm_client.h:156: // TODO(jianli): consider renaming this name
to Start.
Why not rename it now?

[jianli, 2014-03-04 05:14:25]

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

https://codereview.chromium.org/183013007/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_profile_service.cc:267: void Load(const
base::WeakPtr<GCMProfileService>& service);
On 2014/03/04 02:47:51, Dmitry Titov wrote:
> Note: If you rename GCMClient::Load -> Start, you will need to rename this
too,
> and perhaps GCMProfileService::EnsureLoaded() as well...

Yes, will rename all these if we proceed with the renaming.

https://codereview.chromium.org/183013007/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/183013007/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:995: if
(experiments.gcm_channel_state != syncer::Experiments::UNSET) {
On 2014/03/04 02:47:51, Dmitry Titov wrote:
> What if it goes SUPPRESSED->UNSET? It seems it won't be enabled then even if
> there is an app that uses GCM...

The logic will be triggered in GCMProfileService::EnsureLoaded. Each time when
gcm.register or send is called, EnsureLoaded will be called. So I do not think
there is a problem in handling SUPPRESSED to UNSET.

> 
> Maybe it'd be more readable if there were 
> GCMProfileService::SetEnabledState(GCMEnabledState state), since the existing
> GCMEnabledState already has all the possible ways the service can operate in,
> and it corresponds 1:1 to values in experiment...

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/gcm_client.h
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/gcm_client.h...
google_apis/gcm/gcm_client.h:156: // TODO(jianli): consider renaming this name
to Start.
On 2014/03/04 02:47:51, Dmitry Titov wrote:
> Why not rename it now?

Since we're going to merge this patch to M34, I would like not to do the
renaming now in order to make the patch small such that the request could be
approved.

[Dmitry Titov, 2014-03-04 18:01:45]

Thanks for clarifications! LGTM

[fgorski, 2014-03-04 20:07:49]

LGTM,
you can add a TODO for renaming and an explanation why if you don't want to move
the clear username.

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

https://codereview.chromium.org/183013007/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_profile_service.cc:581: username_.clear();
Seems that clearing username could be a part of RemoveCachedData... after all it
is an information that you are caching. And clearing coincides with calls to
RemoveCachedData.

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/engine/gcm_s...
File google_apis/gcm/engine/gcm_store_impl.h (right):

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.h:35: virtual void Close() OVERRIDE;
Seems like Load/Unload or Open/Close would be a better pairing of the 2 methods
above.

[jianli, 2014-03-04 20:52:25]

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

https://codereview.chromium.org/183013007/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_profile_service.cc:581: username_.clear();
On 2014/03/04 20:07:49, Filip Gorski wrote:
> Seems that clearing username could be a part of RemoveCachedData... after all
it
> is an information that you are caching. And clearing coincides with calls to
> RemoveCachedData.

Done.

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/engine/gcm_s...
File google_apis/gcm/engine/gcm_store_impl.h (right):

https://codereview.chromium.org/183013007/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.h:35: virtual void Close() OVERRIDE;
On 2014/03/04 20:07:49, Filip Gorski wrote:
> Seems like Load/Unload or Open/Close would be a better pairing of the 2
methods
> above.

Open means that the store is just opened for reading and no loading is done yet.
Unload is also a bit confusing since we do not mean to unload the data. We
definitely need to revisit the naming here.

[jianli, 2014-03-04 20:54:15]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-03-04 20:56:15]

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

[jianli, 2014-03-04 20:57:24]

The CQ bit was unchecked by jianli@chromium.org

[tim (not reviewing), 2014-03-04 21:19:34]

lgtm

[jianli, 2014-03-04 21:29:36]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-03-04 21:33:32]

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

[commit-bot: I haz the power, 2014-03-04 22:13:46]

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

[commit-bot: I haz the power, 2014-03-04 22:13:47]

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

[fgorski, 2014-03-04 23:00:28]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-03-04 23:04:33]

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

[commit-bot: I haz the power, 2014-03-05 00:12:43]

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

[commit-bot: I haz the power, 2014-03-05 00:12:45]

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

[jianli, 2014-03-05 00:57:27]

Committed patchset #2 manually as r254893 (presubmit successful).