Add IdentityProvider-based AccountTracker to google_apis

Add IdentityProvider-based AccountTracker to google_apis

This is a re-implementation of extensions::AccountTracker, built on
IdentityProvider instead of SigninManager and
ProfileOAuth2TokenService. Removing the dependency on chrome/browser
will allow GCM to use this code.

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

[Michael Courage, 2014-06-16 20:43:39]

fgorski: google_apis/gaia/account_tracker*
rogerta: google_apis/gaia/fake_oauth2_token_service.* + OWNERS

[fgorski, 2014-06-17 04:14:00]

Looking forward to using it.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
File google_apis/gaia/account_tracker.cc (right):

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:25: STLDeleteValues(&user_info_requests_);
Is there a chance Shutdown does not get call before AccountTracker is deleted,
leading to a memory leak?

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
File google_apis/gaia/account_tracker.h (right):

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:27: std::string account_key;  // The account
ID used by OAuth2TokenService.
Did you consider the following:
* calling it account_id
* turning it into a method that returns whichever of the other 2 is the right
account_id at the time.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:67: std::string FindAccountKeyByGaiaId(const
std::string& gaia_id);
Could you have a method to get AccountIds by GaiaId? => solves the problem of
saving too much if I can always look it up.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:78: virtual void OnActiveAccountLogin()
OVERRIDE;
Is active account a new name for primary account?

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:106: scoped_ptr<IdentityProvider>
identity_provider_;
Why is AccountTracker taking the ownership of IdentityProvider?

[Roger Tawa OOO till Jul 10th, 2014-06-17 15:28:20]

lgtm++

Thanks Michael, I was planning on doing myself at some point but had not found
the time.  Apart from its use in GCM, this is an important step in helping
chrome move away from email address and to gaiaid as a unique key for accounts.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
File google_apis/gaia/account_tracker.cc (right):

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:25: STLDeleteValues(&user_info_requests_);
On 2014/06/17 04:14:00, fgorski wrote:
> Is there a chance Shutdown does not get call before AccountTracker is deleted,
> leading to a memory leak?

I'd eventually like this make this class a |KeyedService|, so shutdown should
always be called.  But good idea to DCHECK or something in the dtor that
Shutdown() was called.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:39: const std::string primary_account_id =
active_account_id();
Rename local var to |active_account_id| ?

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:77: // Ignore refresh tokens if there is no
primary account ID at all.
Maybe do a global replace of |primary| with |active| ?

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:148: void
AccountTracker::UpdateSignInState(const std::string account_key,
What is the reason to remove the & from here and below?

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
File google_apis/gaia/account_tracker.h (right):

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:78: virtual void OnActiveAccountLogin()
OVERRIDE;
On 2014/06/17 04:14:00, fgorski wrote:
> Is active account a new name for primary account?

No.  IdentityProvider has its own concept of one account being more special than
others, and exposes that as the "active" account of the given provider.  Its
like a more generalized concept than primary account.  The "primary" account is
a profile specific thing, sort of like a backward compatibility mode for code
that knows only about the sync account.

[Michael Courage, 2014-06-18 23:13:18]

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
File google_apis/gaia/account_tracker.cc (right):

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:25: STLDeleteValues(&user_info_requests_);
On 2014/06/17 15:28:20, Roger Tawa wrote:
> On 2014/06/17 04:14:00, fgorski wrote:
> > Is there a chance Shutdown does not get call before AccountTracker is
deleted,
> > leading to a memory leak?
> 
> I'd eventually like this make this class a |KeyedService|, so shutdown should
> always be called.  But good idea to DCHECK or something in the dtor that
> Shutdown() was called.

Done.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:39: const std::string primary_account_id =
active_account_id();
On 2014/06/17 15:28:20, Roger Tawa wrote:
> Rename local var to |active_account_id| ?

Done. (note: giving method and var the same name does not compile)

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:77: // Ignore refresh tokens if there is no
primary account ID at all.
On 2014/06/17 15:28:20, Roger Tawa wrote:
> Maybe do a global replace of |primary| with |active| ?

Done.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.cc:148: void
AccountTracker::UpdateSignInState(const std::string account_key,
On 2014/06/17 15:28:20, Roger Tawa wrote:
> What is the reason to remove the & from here and below?

StopTrackingAccount had a use-after-free before the change because of pointer
aliasing. Line 182: accounts_.erase(account_key) could delete the account_key
parameter. StopTrackingAllAccounts triggered this issue.

I could have made explicit copies for just the cases where an alias is possible,
but passing string values isn't all that expensive for infrequent operations
like account state change, and eliminates that class of bug.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
File google_apis/gaia/account_tracker.h (right):

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:27: std::string account_key;  // The account
ID used by OAuth2TokenService.
On 2014/06/17 04:14:00, fgorski wrote:
> Did you consider the following:
> * calling it account_id
> * turning it into a method that returns whichever of the other 2 is the right
> account_id at the time.

Considered, but this seemed more straightforward to me.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:67: std::string FindAccountKeyByGaiaId(const
std::string& gaia_id);
On 2014/06/17 04:14:00, fgorski wrote:
> Could you have a method to get AccountIds by GaiaId? => solves the problem of
> saving too much if I can always look it up.

Done.

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:106: scoped_ptr<IdentityProvider>
identity_provider_;
On 2014/06/17 04:14:00, fgorski wrote:
> Why is AccountTracker taking the ownership of IdentityProvider?

That seemed to match existing IdentityProvider usage, but there is no real
reason. Removed ownership.

[fgorski, 2014-06-19 13:48:15]

lgtm

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
File google_apis/gaia/account_tracker.h (right):

https://codereview.chromium.org/336253002/diff/1/google_apis/gaia/account_tra...
google_apis/gaia/account_tracker.h:78: virtual void OnActiveAccountLogin()
OVERRIDE;
On 2014/06/17 15:28:20, Roger Tawa wrote:
> On 2014/06/17 04:14:00, fgorski wrote:
> > Is active account a new name for primary account?
> 
> No.  IdentityProvider has its own concept of one account being more special
than
> others, and exposes that as the "active" account of the given provider.  Its
> like a more generalized concept than primary account.  The "primary" account
is
> a profile specific thing, sort of like a backward compatibility mode for code
> that knows only about the sync account.

Thanks for the explanation... Now I feel like I've been out of the loop forever
;)

https://codereview.chromium.org/336253002/diff/20002/google_apis/gaia/account...
File google_apis/gaia/account_tracker.h (right):

https://codereview.chromium.org/336253002/diff/20002/google_apis/gaia/account...
google_apis/gaia/account_tracker.h:104: IdentityProvider* identity_provider_;
nit:  // Not owned.

[Michael Courage, 2014-06-19 17:03:30]

The CQ bit was checked by courage@chromium.org

[commit-bot: I haz the power, 2014-06-19 17:04:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/courage@chromium.org/336253002/50001

[commit-bot: I haz the power, 2014-06-19 20:56:55]

Change committed as 278476