Keep the GCM data intact when the user signs out of the profile

Keep the GCM data intact when the user signs out of the profile

BUG=419168
TEST=tests updated

Committed: https://crrev.com/15aecc15078ebf83ee6a19b9614d9325d5f8355d
Cr-Commit-Position: refs/heads/master@{#298343}

[jianli, 2014-09-30 23:00:53]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org

[fgorski, 2014-10-01 16:54:09]

it looks good, but I think we should better document tests.

Also do we ever call Purge in production after removing the call from
GCMProfileService? (I think we can remove it, which does not need to happen in
this patch).

https://codereview.chromium.org/617003005/diff/1/components/gcm_driver/gcm_dr...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/617003005/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver_desktop_unittest.cc:409: TEST_F(GCMDriverTest,
SignInAndSignOutOnGCMEnabled) {
I don't understand why we are stopping. Is GCM only available to signed in users
in this scenario?
Could you add a relevant comment.

[jianli, 2014-10-01 21:56:16]

On 2014/10/01 16:54:09, fgorski wrote:
> it looks good, but I think we should better document tests.
> 
> Also do we ever call Purge in production after removing the call from
> GCMProfileService? (I think we can remove it, which does not need to happen in
> this patch).

Purge is not called any more. We can remove it later.

[jianli, 2014-10-01 21:56:23]

https://codereview.chromium.org/617003005/diff/1/components/gcm_driver/gcm_dr...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/617003005/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver_desktop_unittest.cc:409: TEST_F(GCMDriverTest,
SignInAndSignOutOnGCMEnabled) {
On 2014/10/01 16:54:09, fgorski wrote:
> I don't understand why we are stopping. Is GCM only available to signed in
users
> in this scenario?
> Could you add a relevant comment.

Before we drop the sign-in enforcement and roll out GCM to all users, GCM could
only be used by the signed-in users. Once the users sign out, the GCM will be
shut down though the GCM data remains intact.

Added comment.

[fgorski, 2014-10-01 23:36:45]

lgtm

[fgorski, 2014-10-03 01:32:50]

Found interesting thing about stopping when signing out. (We should handle that
for users that fall under on the GCM Disabled).

https://codereview.chromium.org/617003005/diff/30001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/617003005/diff/30001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.cc:395: Stop();
I think this leaves that app handlers still hanging. This also causes
initialization to happen multiple times on the account mapper.

Did you consider shutting down the app handlers?
If we don't want to do it, we should probably explicitly stop account mapper.

I am trying to work out a patch with a solution to that problem, but I need to
hear from you.

[fgorski, 2014-10-03 01:32:52]

Found interesting thing about stopping when signing out. (We should handle that
for users that fall under on the GCM Disabled).

[jianli, 2014-10-06 07:07:02]

https://codereview.chromium.org/617003005/diff/30001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/617003005/diff/30001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.cc:395: Stop();
On 2014/10/03 01:32:50, fgorski wrote:
> I think this leaves that app handlers still hanging. This also causes
> initialization to happen multiple times on the account mapper.
> 
> Did you consider shutting down the app handlers?
> If we don't want to do it, we should probably explicitly stop account mapper.
> 
> I am trying to work out a patch with a solution to that problem, but I need to
> hear from you.

I don't think we want to shut down the app handlers because once the user signs
in everything should start to work.

It's better to stop account mapper, probbaly in GCMDriverDesktop::Stop.

[fgorski, 2014-10-06 16:52:30]

On 2014/10/06 07:07:02, jianli wrote:
>
https://codereview.chromium.org/617003005/diff/30001/components/gcm_driver/gc...
> File components/gcm_driver/gcm_driver_desktop.cc (right):
> 
>
https://codereview.chromium.org/617003005/diff/30001/components/gcm_driver/gc...
> components/gcm_driver/gcm_driver_desktop.cc:395: Stop();
> On 2014/10/03 01:32:50, fgorski wrote:
> > I think this leaves that app handlers still hanging. This also causes
> > initialization to happen multiple times on the account mapper.
> > 
> > Did you consider shutting down the app handlers?
> > If we don't want to do it, we should probably explicitly stop account
mapper.
> > 
> > I am trying to work out a patch with a solution to that problem, but I need
to
> > hear from you.
> 
> I don't think we want to shut down the app handlers because once the user
signs
> in everything should start to work.
> 
> It's better to stop account mapper, probbaly in GCMDriverDesktop::Stop.

That is what I ended up doing in https://codereview.chromium.org/618003002

Also, you will have to rebase this patch. OVERRIDE => override

[jianli, 2014-10-06 22:50:46]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-10-06 22:53:15]

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

[commit-bot: I haz the power, 2014-10-07 01:42:52]

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

[commit-bot: I haz the power, 2014-10-07 01:42:53]

Exceeded time limit waiting for builds to trigger.

[jianli, 2014-10-07 02:00:17]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-10-07 02:05:13]

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

[commit-bot: I haz the power, 2014-10-07 03:04:05]

Committed patchset #4 (id:50001) as dd615c88d4d3950b30cc654b88c1dde67d5cf0c6

[commit-bot: I haz the power, 2014-10-07 03:04:50]

Patchset 4 (id:??) landed as
https://crrev.com/15aecc15078ebf83ee6a19b9614d9325d5f8355d
Cr-Commit-Position: refs/heads/master@{#298343}