[GCM] Check-in with signed in accounts associates device to user

Adding a step to the check-in process that ensures the correct account information is present before device
check-in happen, in order to maintain a relationship
between signed in accounts and the device.

Behavior of the check-in is not symmetric:
* Adding an account converges slowly - newly added
account will be associated to device with next periodic
check, to avoid checking in too often.
* Removing account triggers check-in immediately to ensure
users privacy.


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

[fgorski, 2014-07-10 20:20:08]

self review

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.h:133: // The check-in info for the user.
Returned by the server.
update documentation

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.h:206: // Callback for setting last
checkin time in the |gcm_store_|.
update documentation

[fgorski, 2014-07-10 20:21:04]

PTAL.

This patch introduces sending OAuth2 tokens and email address of logged in
accounts to GCM, to related devices to users.

[Nicolas Zea, 2014-07-10 20:57:33]

https://codereview.chromium.org/378643002/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/378643002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.cc:16: const char
kGCMGroupServerScope[] = "https://www.googleapis.com/auth/gcm";
Why is this changing?

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:338: // scheduling a periodc checkin
for devices with no signed in users
nit: periodic

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:438: if (account_removed ||
Do we need the account_removed boolean if we can compare the
last_checkin_accounts_count with account_tokens.size()?

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:439:
device_checkin_info_.last_checkin_accounts_count >
I also wonder if there's  risk of issues if an account was removed and then one
was added back. Last checkin accounts count might match the current token size.
Is that possible?

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.h (right):

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.h:59: // GCMDriver implementation:
should these two sections be combined (this and the one above)?

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.h:78: bool account_removed);
nit: style guide prefers using an enum to a bool in a public method like this
(e.g. enum AccountStatus {ACCOUNT_REMOVED, NO_ACCOUNTS_REMOVED})

https://codereview.chromium.org/378643002/diff/60001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.h (right):

https://codereview.chromium.org/378643002/diff/60001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:44: const std::map<std::string,
std::string> account_tokens,
pass by const ref

[fgorski, 2014-07-11 22:51:22]

PTAL.


Nicolas, if you think it is ok to store a list of checked in accounts, I can
simplify the logic and never expose account status in the stack.

https://codereview.chromium.org/378643002/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/378643002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.cc:16: const char
kGCMGroupServerScope[] = "https://www.googleapis.com/auth/gcm";
On 2014/07/10 20:57:32, Nicolas Zea wrote:
> Why is this changing?

I am correcting the scope here, now that I was able to manually test end to end.

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:338: // scheduling a periodc checkin
for devices with no signed in users
On 2014/07/10 20:57:32, Nicolas Zea wrote:
> nit: periodic

Done.

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:438: if (account_removed ||
On 2014/07/10 20:57:32, Nicolas Zea wrote:
> Do we need the account_removed boolean if we can compare the
> last_checkin_accounts_count with account_tokens.size()?

If the same account is first removed and then added after a few minutes, it will
take 2 days to get the link between account and device back, but it will
eventually fix itself. We are making the process more sensitive to account
removal here.

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:439:
device_checkin_info_.last_checkin_accounts_count >
On 2014/07/10 20:57:32, Nicolas Zea wrote:
> I also wonder if there's  risk of issues if an account was removed and then
one
> was added back. Last checkin accounts count might match the current token
size.
> Is that possible?

This approach can run into issues where the count is the same because you added
one and removed another, hence we are using the accounts_removed flag.

If some accounts can be removed when the device is not looking, we generally
fall back to the number comparison.

There is another way to do the whole thing, which requires storing a complete
list of accounts and making a more thorough comparison when setting a new list
of accounts. I can implement it that way. We won't require the account_removed
flag then.

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.h:133: // The check-in info for the user.
Returned by the server.
On 2014/07/10 20:20:08, fgorski wrote:
> update documentation

Done.

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.h:206: // Callback for setting last
checkin time in the |gcm_store_|.
On 2014/07/10 20:20:08, fgorski wrote:
> update documentation

Done.

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.h (right):

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.h:59: // GCMDriver implementation:
On 2014/07/10 20:57:32, Nicolas Zea wrote:
> should these two sections be combined (this and the one above)?

Done.

https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.h:78: bool account_removed);
On 2014/07/10 20:57:32, Nicolas Zea wrote:
> nit: style guide prefers using an enum to a bool in a public method like this
> (e.g. enum AccountStatus {ACCOUNT_REMOVED, NO_ACCOUNTS_REMOVED})

Done.

https://codereview.chromium.org/378643002/diff/60001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.h (right):

https://codereview.chromium.org/378643002/diff/60001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:44: const std::map<std::string,
std::string> account_tokens,
On 2014/07/10 20:57:32, Nicolas Zea wrote:
> pass by const ref

Done.

[Nicolas Zea, 2014-07-11 22:55:07]

On 2014/07/11 22:51:22, fgorski wrote:
> PTAL.
> 
> 
> Nicolas, if you think it is ok to store a list of checked in accounts, I can
> simplify the logic and never expose account status in the stack.

I think that makes sense. We're not storing tokens or anything, just account ids
associated with the last checkin.

> 
>
https://codereview.chromium.org/378643002/diff/60001/chrome/browser/services/...
> File chrome/browser/services/gcm/gcm_account_tracker.cc (right):
> 
>
https://codereview.chromium.org/378643002/diff/60001/chrome/browser/services/...
> chrome/browser/services/gcm/gcm_account_tracker.cc:16: const char
> kGCMGroupServerScope[] = "https://www.googleapis.com/auth/gcm";
> On 2014/07/10 20:57:32, Nicolas Zea wrote:
> > Why is this changing?
> 
> I am correcting the scope here, now that I was able to manually test end to
end.
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> File components/gcm_driver/gcm_client_impl.cc (right):
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_client_impl.cc:338: // scheduling a periodc checkin
> for devices with no signed in users
> On 2014/07/10 20:57:32, Nicolas Zea wrote:
> > nit: periodic
> 
> Done.
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_client_impl.cc:438: if (account_removed ||
> On 2014/07/10 20:57:32, Nicolas Zea wrote:
> > Do we need the account_removed boolean if we can compare the
> > last_checkin_accounts_count with account_tokens.size()?
> 
> If the same account is first removed and then added after a few minutes, it
will
> take 2 days to get the link between account and device back, but it will
> eventually fix itself. We are making the process more sensitive to account
> removal here.
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_client_impl.cc:439:
> device_checkin_info_.last_checkin_accounts_count >
> On 2014/07/10 20:57:32, Nicolas Zea wrote:
> > I also wonder if there's  risk of issues if an account was removed and then
> one
> > was added back. Last checkin accounts count might match the current token
> size.
> > Is that possible?
> 
> This approach can run into issues where the count is the same because you
added
> one and removed another, hence we are using the accounts_removed flag.
> 
> If some accounts can be removed when the device is not looking, we generally
> fall back to the number comparison.
> 
> There is another way to do the whole thing, which requires storing a complete
> list of accounts and making a more thorough comparison when setting a new list
> of accounts. I can implement it that way. We won't require the account_removed
> flag then.
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> File components/gcm_driver/gcm_client_impl.h (right):
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_client_impl.h:133: // The check-in info for the
user.
> Returned by the server.
> On 2014/07/10 20:20:08, fgorski wrote:
> > update documentation
> 
> Done.
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_client_impl.h:206: // Callback for setting last
> checkin time in the |gcm_store_|.
> On 2014/07/10 20:20:08, fgorski wrote:
> > update documentation
> 
> Done.
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> File components/gcm_driver/gcm_driver_desktop.h (right):
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_driver_desktop.h:59: // GCMDriver implementation:
> On 2014/07/10 20:57:32, Nicolas Zea wrote:
> > should these two sections be combined (this and the one above)?
> 
> Done.
> 
>
https://codereview.chromium.org/378643002/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_driver_desktop.h:78: bool account_removed);
> On 2014/07/10 20:57:32, Nicolas Zea wrote:
> > nit: style guide prefers using an enum to a bool in a public method like
this
> > (e.g. enum AccountStatus {ACCOUNT_REMOVED, NO_ACCOUNTS_REMOVED})
> 
> Done.
> 
>
https://codereview.chromium.org/378643002/diff/60001/google_apis/gcm/engine/c...
> File google_apis/gcm/engine/checkin_request.h (right):
> 
>
https://codereview.chromium.org/378643002/diff/60001/google_apis/gcm/engine/c...
> google_apis/gcm/engine/checkin_request.h:44: const std::map<std::string,
> std::string> account_tokens,
> On 2014/07/10 20:57:32, Nicolas Zea wrote:
> > pass by const ref
> 
> Done.

[fgorski, 2014-07-15 17:46:37]

PTAL.
This one persists the account that were used by last checkin. Since I don't need
to pass information about account removal all the way down I dropped the
AccountStatus email.

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

https://codereview.chromium.org/378643002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:12: #include
"components/gcm_driver/gcm_client.h"
remove

[Nicolas Zea, 2014-07-16 20:53:55]

LGTM with some nits and a couple comments

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

https://codereview.chromium.org/378643002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:39: // Identity observer only
has actual work to do when the user is acutally signed
nit: acutally

https://codereview.chromium.org/378643002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:114: if
(gcm_account_tracker_)
Shouldn't we always have an account tracker at this point?

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:452: // restarts of Chrome. If there is
a checkin in progress, it will be canceled.
Comment about why we don't perform a checkin if an account has been added?

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:454: DVLOG(1) << "Detecting that
account has been removed, forcing checkin.";
nit: Detecting -> Detected

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:138: void SnapshotCheckinAccounts();
Hmm, I wonder if this struct is becoming complex enough to merit being a class,
with explicit setter functions.

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:145: // True if accounts where already
provided through SetAccountsForCheckin(),
nit: where -> were

[fgorski, 2014-07-17 03:35:35]

I ran tests with DCHECK instead of if in place you pointed out (account log
out). There are sync integration tests failing and browser_tests failing because
of that.
I'll check in with if statement and we can take another look in separate patch.

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

https://codereview.chromium.org/378643002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:12: #include
"components/gcm_driver/gcm_client.h"
On 2014/07/15 17:46:37, fgorski wrote:
> remove

Done.

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

https://codereview.chromium.org/378643002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:39: // Identity observer only
has actual work to do when the user is acutally signed
On 2014/07/16 20:53:54, Nicolas Zea wrote:
> nit: acutally

Done.

https://codereview.chromium.org/378643002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:114: if
(gcm_account_tracker_)
On 2014/07/16 20:53:54, Nicolas Zea wrote:
> Shouldn't we always have an account tracker at this point?

Replacing with DCHECK and starting trybots.

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:452: // restarts of Chrome. If there is
a checkin in progress, it will be canceled.
On 2014/07/16 20:53:54, Nicolas Zea wrote:
> Comment about why we don't perform a checkin if an account has been added?

Done.

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:454: DVLOG(1) << "Detecting that
account has been removed, forcing checkin.";
On 2014/07/16 20:53:54, Nicolas Zea wrote:
> nit: Detecting -> Detected

Done.

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:138: void SnapshotCheckinAccounts();
On 2014/07/16 20:53:54, Nicolas Zea wrote:
> Hmm, I wonder if this struct is becoming complex enough to merit being a
class,
> with explicit setter functions.

Ack. Lemme do a separate patch for that, there is a few things I want to move
around (e.g. last_checkin_time, check  if account was removed, so on.)

TODO added.

https://codereview.chromium.org/378643002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:145: // True if accounts where already
provided through SetAccountsForCheckin(),
On 2014/07/16 20:53:54, Nicolas Zea wrote:
> nit: where -> were

Done.

[fgorski, 2014-07-17 03:35:42]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-07-17 03:39:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fgorski@chromium.org/378643002/140001

[commit-bot: I haz the power, 2014-07-17 07:36:02]

Change committed as 283693