[GCM] Adding registration ID request and tests to GCM Account Mapper

[GCM] Adding registration ID request and tests to GCM Account Mapper

Adding registration ID request and tests to GCM Account Mapper
* Adding code to get registration ID from GCM
* Making sure account tokens are only processed when reg ID is present
* Remembering only the last assigned set of account tokens
* Fixing tests that require registration ID
* Adding tests specific to registration ID setting

BUG=374969
R=zea@chromium.org

Committed: https://crrev.com/ba729da9c8327a4efc18963abd365efe409a80b5
Cr-Commit-Position: refs/heads/master@{#295890}

[fgorski, 2014-09-17 20:26:58]

PTAL,

Seems like I didn't send it out yesterday.

[Nicolas Zea, 2014-09-18 17:35:15]

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
File components/gcm_driver/gcm_account_mapper.cc (right):

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:60: // If account mapper is not
ready to handle tasks yet, save the lastest
nit: lastest -> latest

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:286: void
GCMAccountMapper::Register() {
nit: dcheck !registration_id_? Or do we want to have support for calling
register more than once?

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:307: Register();
how many times will we retry?

[Nicolas Zea, 2014-09-18 17:36:10]

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
File components/gcm_driver/gcm_account_mapper.cc (right):

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:286: void
GCMAccountMapper::Register() {
On 2014/09/18 17:35:14, Nicolas Zea wrote:
> nit: dcheck !registration_id_? Or do we want to have support for calling
> register more than once?

Come to think of it, do we have any logic to prevent multiple registration
requests happening at once?

[fgorski, 2014-09-18 18:08:53]

Addressed feedback and updated test for retrying registration ID.

PTAL

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
File components/gcm_driver/gcm_account_mapper.cc (right):

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:60: // If account mapper is not
ready to handle tasks yet, save the lastest
On 2014/09/18 17:35:14, Nicolas Zea wrote:
> nit: lastest -> latest

Done.

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:286: void
GCMAccountMapper::Register() {
On 2014/09/18 17:36:10, Nicolas Zea wrote:
> On 2014/09/18 17:35:14, Nicolas Zea wrote:
> > nit: dcheck !registration_id_? Or do we want to have support for calling
> > register more than once?
> 
> Come to think of it, do we have any logic to prevent multiple registration
> requests happening at once?

Yes, the async operation pending will be issued immediately.

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:286: void
GCMAccountMapper::Register() {
On 2014/09/18 17:35:14, Nicolas Zea wrote:
> nit: dcheck !registration_id_? Or do we want to have support for calling
> register more than once?

Done.

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:307: Register();
On 2014/09/18 17:35:14, Nicolas Zea wrote:
> how many times will we retry?

Register implements back-off, so it does not matter that much... If we don't
have a network we can keep trying. Here are alternatives:
* I can implement gcm connection observer to get an indication of when network
is present, but that would not be on the connection that registration would run
on.
* I could listen to a generic network event and only retry Register then.
* I can bail and never care about registration, or perhaps reach out for it when
I got a batch of tokens but no registration yet. (this is lazy, but should work
-> I think I like that the most.)
* We could simply ignore it and wait for a restart, but some people don't
restart that often.

In either case we are only talking about the case where there is no registration
id in the cache, so this is a very narrow case. (First time setup of the
mapper.)

[Nicolas Zea, 2014-09-19 17:41:58]

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
File components/gcm_driver/gcm_account_mapper.cc (right):

https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
components/gcm_driver/gcm_account_mapper.cc:307: Register();
On 2014/09/18 18:08:53, fgorski wrote:
> On 2014/09/18 17:35:14, Nicolas Zea wrote:
> > how many times will we retry?
> 
> Register implements back-off, so it does not matter that much... If we don't
> have a network we can keep trying. Here are alternatives:
> * I can implement gcm connection observer to get an indication of when network
> is present, but that would not be on the connection that registration would
run
> on.
> * I could listen to a generic network event and only retry Register then.
> * I can bail and never care about registration, or perhaps reach out for it
when
> I got a batch of tokens but no registration yet. (this is lazy, but should
work
> -> I think I like that the most.)
> * We could simply ignore it and wait for a restart, but some people don't
> restart that often.
> 
> In either case we are only talking about the case where there is no
registration
> id in the cache, so this is a very narrow case. (First time setup of the
> mapper.)

Does the bail approach mean retry when new tokens arrive (new refresh tokens or
access tokens?), and if that doesn't happen retry on restart? Unless we're
talking access tokens, we don't generally get refresh tokens very often, so I
suspect this will primarily rely on restart.

I'm okay if we let it keep retrying, but would prefer putting at least 30 second
delay between a failed registration attempt and a retry (backoff is reset after
all)

[fgorski, 2014-09-19 18:04:03]

On 2014/09/19 17:41:58, Nicolas Zea wrote:
>
https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
> File components/gcm_driver/gcm_account_mapper.cc (right):
> 
>
https://codereview.chromium.org/580603002/diff/1/components/gcm_driver/gcm_ac...
> components/gcm_driver/gcm_account_mapper.cc:307: Register();
> On 2014/09/18 18:08:53, fgorski wrote:
> > On 2014/09/18 17:35:14, Nicolas Zea wrote:
> > > how many times will we retry?
> > 
> > Register implements back-off, so it does not matter that much... If we don't
> > have a network we can keep trying. Here are alternatives:
> > * I can implement gcm connection observer to get an indication of when
network
> > is present, but that would not be on the connection that registration would
> run
> > on.
> > * I could listen to a generic network event and only retry Register then.
> > * I can bail and never care about registration, or perhaps reach out for it
> when
> > I got a batch of tokens but no registration yet. (this is lazy, but should
> work
> > -> I think I like that the most.)
> > * We could simply ignore it and wait for a restart, but some people don't
> > restart that often.
> > 
> > In either case we are only talking about the case where there is no
> registration
> > id in the cache, so this is a very narrow case. (First time setup of the
> > mapper.)
> 
> Does the bail approach mean retry when new tokens arrive (new refresh tokens
or
> access tokens?), and if that doesn't happen retry on restart? Unless we're
> talking access tokens, we don't generally get refresh tokens very often, so I
> suspect this will primarily rely on restart.
Yes, when a new batch of access tokens arrives and the mapper is not ready yet,
we save the tokens (l 63) and then check if the initialization completed (l 66),
if so, we try to registrer again (if there is a pending registration that is
ok),
the new one is stopped and the old one is still trying to make progress. Either
way once it finishes, we try to send the new tokens.

> 
> I'm okay if we let it keep retrying, but would prefer putting at least 30
second
> delay between a failed registration attempt and a retry (backoff is reset
after
> all)

I don't think it is worth retrying, as the tokens that might be waiting expire
at some point, we might as well wait until the next time. I can still flip it
back
though and add a delay of 30 seconds or more.

[Nicolas Zea, 2014-09-19 18:21:15]

Ok, makes sense to me now after chatting offline. LGTM

[fgorski, 2014-09-19 20:12:53]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-09-19 20:26:02]

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

[commit-bot: I haz the power, 2014-09-19 20:26:04]

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

[commit-bot: I haz the power, 2014-09-19 20:59:25]

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

[commit-bot: I haz the power, 2014-09-19 20:59:26]

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

[commit-bot: I haz the power, 2014-09-19 21:00:13]

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

[commit-bot: I haz the power, 2014-09-19 21:00:14]

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

[fgorski, 2014-09-19 21:38:23]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:38:58]

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

[commit-bot: I haz the power, 2014-09-19 22:49:09]

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

[commit-bot: I haz the power, 2014-09-19 22:49:10]

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

[fgorski, 2014-09-20 20:51:39]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-09-20 20:52:28]

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

[commit-bot: I haz the power, 2014-09-20 20:56:01]

Committed patchset #2 (id:20001) as 0e60c4702814da0ee8073f80580200219c759b63

[commit-bot: I haz the power, 2014-09-20 20:56:42]

Patchset 2 (id:??) landed as
https://crrev.com/ba729da9c8327a4efc18963abd365efe409a80b5
Cr-Commit-Position: refs/heads/master@{#295890}