Making OAuth2TokenService multi-login aware, updating callers, minor fixes

Making OAuth2TokenService multi-login aware:
* Adding account_id as a parameter to public methods, like: StartRequestXX, RefreshTokenIsAvailable, GetRefreshToken, InvalidateToken,
* Updating callers and derived classes,
* Updating cache handling for the OAuth2 access tokens in OAuth2TokenService

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

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

[fgorski, 2013-08-21 22:55:47]

Drew, Michael,

Please take a look at the public potion of the API that I want to propose. This
is the first crack at understanding the scope of changes and checking with you
if the resulting public interface of the O2TS will be usable by consumers,
before I send a very broad review request to owners.

For now GetPrimaryAccountId is on PO2TS, but we can discuss that and see if
there is a better place for it.

I'll update the guts of the token services once changes from you and Zel are in.

Thanks!

[Andrew T Wilson (Slow), 2013-08-23 09:38:12]

OK, my stuff has landed. It would be good if you can do two things in addition
to my notes below:

1) Update FakeProfileOAuthTokenService
2) Merge the APIs for AndroidProfileOAuthTokenService now that we're passing in
an account ID/username.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/invalidation/t...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/invalidation/t...
chrome/browser/invalidation/ticl_invalidation_service.cc:209:
signin_manager_->GetAuthenticatedUsername(), oauth2_scopes, this);
Why are we using GetAuthenticatedUsername() instead of GetPrimaryAccountId()
here?

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.cc:406: if (HasCacheEntry(scopes))
Don't we need to take the account_id down to the cache level as well (don't want
to return a previously-fetched token for a different account).

It's OK if we're just tweaking APIs for now, but you should add a TODO() at
least if it's intentional.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
chrome/browser/signin/profile_oauth2_token_service.cc:182: for
(std::map<std::string, std::string>::const_iterator iter =
Add a comment as to what this is doing. Can users hit this case in reality (say,
after a re-auth) or should this be a CHECK/DCHECK instead?

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
chrome/browser/signin/profile_oauth2_token_service.cc:222: //
ClearCacheForAccount(account_id);
Add a TODO here?

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
chrome/browser/signin/profile_oauth2_token_service.cc:233:
UpdateAuthError(GoogleServiceAuthError::AuthErrorNone());
Just a note that we probably have to figure out how we want SigninGlobalError to
work in this multi-profile world. Do we need multiple SigninGlobalError objects
(one per account?) or just a single one and somehow have SigninGlobalError deal
with multi-accounts internally.

My point is that if I have two accounts, A and B, and they both have auth
errors, I probably shouldn't clear the "global" auth error (as we're doing here)
just because I've re-auth one of the accounts.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/ui/auto_login_...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/ui/auto_login_...
chrome/browser/ui/auto_login_prompter.cc:42:
signin_manager->GetAuthenticatedUsername()))
Still unsure about mixing GetAuthenticatedUsername() and GetPrimaryAccountId() -
what's your take about when one should be used instead of the other? Code like
this will probably break if we decide to make account ID anything other than the
user's email address (like the gaia ID).

[fgorski, 2013-08-23 19:13:35]

I addressed the code review comments, but will not be able to work on the
Fake/Android O2TSs until Monday.

As for caching, I plan to update all of it, once change from Zel lands, unless
we are putting that one on hold.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/invalidation/t...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/invalidation/t...
chrome/browser/invalidation/ticl_invalidation_service.cc:209:
signin_manager_->GetAuthenticatedUsername(), oauth2_scopes, this);
On 2013/08/23 09:38:13, Andrew T Wilson wrote:
> Why are we using GetAuthenticatedUsername() instead of GetPrimaryAccountId()
> here?

Done. I left it in that way as a case study, anticipating the comment.
Essentially there are 2 ways to go about making conversion to account_id for
classes that want to filter on account: Either we pass in account_id (see
google_apis in this change), or we need to take dependency on
ProfileOAuth2TokenService (as proposed in the new version of
TiclInvalidationService). We need to make a call if both are acceptable.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.cc:406: if (HasCacheEntry(scopes))
On 2013/08/23 09:38:13, Andrew T Wilson wrote:
> Don't we need to take the account_id down to the cache level as well (don't
want
> to return a previously-fetched token for a different account).
> 
> It's OK if we're just tweaking APIs for now, but you should add a TODO() at
> least if it's intentional.

Waiting for a change from Zel to revamp the whole caching, as account_id might
end up as a part of the cache key, and he is already modifying that.

Added TODO's into cache related methods.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
chrome/browser/signin/profile_oauth2_token_service.cc:182: for
(std::map<std::string, std::string>::const_iterator iter =
On 2013/08/23 09:38:13, Andrew T Wilson wrote:
> Add a comment as to what this is doing. Can users hit this case in reality
(say,
> after a re-auth) or should this be a CHECK/DCHECK instead?

Done. Added a comment and a TODO to investigate CHECK/DCHECK option.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
chrome/browser/signin/profile_oauth2_token_service.cc:222: //
ClearCacheForAccount(account_id);
On 2013/08/23 09:38:13, Andrew T Wilson wrote:
> Add a TODO here?

Done.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/signin/profile...
chrome/browser/signin/profile_oauth2_token_service.cc:233:
UpdateAuthError(GoogleServiceAuthError::AuthErrorNone());
On 2013/08/23 09:38:13, Andrew T Wilson wrote:
> Just a note that we probably have to figure out how we want SigninGlobalError
to
> work in this multi-profile world. Do we need multiple SigninGlobalError
objects
> (one per account?) or just a single one and somehow have SigninGlobalError
deal
> with multi-accounts internally.
> 
> My point is that if I have two accounts, A and B, and they both have auth
> errors, I probably shouldn't clear the "global" auth error (as we're doing
here)
> just because I've re-auth one of the accounts.

Done. Included a section in design doc. Updated the signature of UpdateAuthError
method.

https://codereview.chromium.org/23382008/diff/1/chrome/browser/ui/auto_login_...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://codereview.chromium.org/23382008/diff/1/chrome/browser/ui/auto_login_...
chrome/browser/ui/auto_login_prompter.cc:42:
signin_manager->GetAuthenticatedUsername()))
On 2013/08/23 09:38:13, Andrew T Wilson wrote:
> Still unsure about mixing GetAuthenticatedUsername() and GetPrimaryAccountId()
-
> what's your take about when one should be used instead of the other? Code like
> this will probably break if we decide to make account ID anything other than
the
> user's email address (like the gaia ID).

Done. That one was a mistake.

[Andrew T Wilson (Slow), 2013-08-26 15:03:50]

OK, I made a spot check of your changes and they look good. I'll make a more
thorough pass once you've merged with Zel's patch (assuming that's happening
still) and have done the Android stuff.

[Roger Tawa OOO till Jul 10th, 2013-08-26 21:04:53]

Looking good Filip.  I wonder if this can broken down into multiple CLs to make
it easier to review.  A comments below, I will take a closer look again.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:174: NOTREACHED();
Might be cleaner now to add a DCHECK for type at line 168 and remove this
NOTREACHED call.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:136: Consumer* consumer);
Do the three StartRequest() methods need to be virtual?

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:194: const std::string&
refresh_token) {
Since the Fetcher class now has the |account_id|, I think you can change this
argument from |const std::string& refresh_token| to |const std::string&
account_id|.  This should simply the function a lot.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:224: }
I think GetAccounts() belongs to base class OAuth2TokenService.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/auto_lo...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/auto_lo...
chrome/browser/ui/auto_login_prompter.cc:43: return false;
Need { and } since condition spans more than one line.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:281:
service->StartRequest(account_id, oauth_scopes, this));
I the case of chromeos and type=="device", account_id will be an empry string. 
Is that allowed?

[fgorski, 2013-08-26 23:30:54]

Still trying to work out how to use FakePO2TS to test PO2TSRequest to remove
virtual from StartRquest. Also needed to fix some test issue which blocked me
for a while.

I will do the AndroidPO2TS in next update to the CL as well.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:174: NOTREACHED();
On 2013/08/26 21:04:53, Roger Tawa wrote:
> Might be cleaner now to add a DCHECK for type at line 168 and remove this
> NOTREACHED call.

Done.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:194: const std::string&
refresh_token) {
On 2013/08/26 21:04:53, Roger Tawa wrote:
> Since the Fetcher class now has the |account_id|, I think you can change this
> argument from |const std::string& refresh_token| to |const std::string&
> account_id|.  This should simply the function a lot.

I've put it on a TODO for myself, but I am deferring cache handling updates
until Zel's CL is committed or abandoned.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:224: }
On 2013/08/26 21:04:53, Roger Tawa wrote:
> I think GetAccounts() belongs to base class OAuth2TokenService.

I've added GetAccounts() to the public interface of O2TS as an abstract method
(which we can provide a default implementation for to avoid fixing all of the
test overloads of O2TS). I still think that it makes more sense to have to
concrete implementation (the one above) here in PO2TS, as that is the class that
knows about accounts that have refresh tokens.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/auto_lo...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/auto_lo...
chrome/browser/ui/auto_login_prompter.cc:43: return false;
On 2013/08/26 21:04:53, Roger Tawa wrote:
> Need { and } since condition spans more than one line.

Done.

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/23382008/diff/11001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:281:
service->StartRequest(account_id, oauth_scopes, this));
On 2013/08/26 21:04:53, Roger Tawa wrote:
> I the case of chromeos and type=="device", account_id will be an empry string.

> Is that allowed?

It's exactly what I expect to happen. I'll document that.
Alternate solution is to use GetRobotAccountId(), if it makes sense, and
provided we make it accessible.

[fgorski, 2013-08-28 18:32:26]

Drew, PTAL.

Everyone,
I've added an update to AndroidPO2TS. Given the size of the changelist, I am
almost ready to start slicing it, and temporarily explode the interface to 6
methods instead of 3, for an easy transition. Unless we are happy with TBRing
the owners of the files updated for an API change.

[Roger Tawa OOO till Jul 10th, 2013-08-29 15:41:40]

Looks good Filip.  All changes in google_apis and chrome\browser\signin look
good modulo comments below.  A few things still missing as we've already
discussed off thread:

- handling of access token cache (waiting for Zel's CL)
- use new FakePO2TS

As for TBR, I think the following changes could be TBRed:
local_discovery_ui_handler
api_util
auto_login_prompter

Also, most of the changes above android_profile_oauth2_token_service.h could be
TBRed except the following I think:

invalidation_service_factory
ticl_invalidation_service
device_oauth2_token_service
cloud_policy_client_registration_helper

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:227:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Should we DCHECK that |account_id| is empty?  Same for GetRefreshToken().

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:63: &&
authenticated_username == account_id;
Shouldn't this check the underlying android account manager to see if an account
is available?  Even the old code before your change seems incorrect.  I'll ask
Tommy to take a look.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/fak...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:42: }
I'm not convinced that FakePO2TS should allow itself to be created without being
initialized.  That's what I did in https://codereview.chromium.org/19567004/,
but I need to make sure all test still pass with this change.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:213: return signin_manager
? signin_manager->GetAuthenticatedUsername() :
Should probably DCHECK that SM is not null, at least as long as this method
exists in this class.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc:127:
std::string oauth2_access_token) {
nit: fix indent.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:111: // No
username for profile in unit tests, defaulting to empty string.
Would it be better to use a dummy name in tests?  That way the O2TS api could
CHECK for empty strings.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:695: OnRefreshTokensLoaded();
Shouldn't this happen only if |account_id| is the primary account id? Or are we
just assuming OnRefreshTokensLoaded() will fail if the primary is not available
yet?

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service.cc:80: const std::string& account_id,
nit: for consistency, put account_id ahead of chrome_client_id and
chrome_client_secret?  Same for ctor below.

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service_unittest.cc:93: std::string account_id_;
This member is not initialized anywhere.  Maybe set it something like
"user@gmail.com" in SetUp() ?

[fgorski, 2013-08-29 23:04:13]

Updated, fixed a bug related to the primary account refresh token.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:227:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2013/08/29 15:41:40, Roger Tawa wrote:
> Should we DCHECK that |account_id| is empty?  Same for GetRefreshToken().

Done.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:63: &&
authenticated_username == account_id;
On 2013/08/29 15:41:40, Roger Tawa wrote:
> Shouldn't this check the underlying android account manager to see if an
account
> is available?  Even the old code before your change seems incorrect.  I'll ask
> Tommy to take a look.

You are correct, however there are 2 ways to go about that:
1. If we want all of the accounts in Android to be visible here, we check with
system account manager.
2. Otherwise we filter it down only to the accounts that we want to expose in
Chrome, and the logic had to be a bit more sophisticated.

I didn't want to solve the issue here (due to size of CL), hence went with a
simple check for a primary account.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/fak...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:42: }
On 2013/08/29 15:41:40, Roger Tawa wrote:
> I'm not convinced that FakePO2TS should allow itself to be created without
being
> initialized.  That's what I did in https://codereview.chromium.org/19567004/,
> but I need to make sure all test still pass with this change.

I'll happily adjust, but I was trying to respect the comment, with a relevant
piece of code.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:213: return signin_manager
? signin_manager->GetAuthenticatedUsername() :
On 2013/08/29 15:41:40, Roger Tawa wrote:
> Should probably DCHECK that SM is not null, at least as long as this method
> exists in this class.

Done.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:695: OnRefreshTokensLoaded();
On 2013/08/29 15:41:40, Roger Tawa wrote:
> Shouldn't this happen only if |account_id| is the primary account id? Or are
we
> just assuming OnRefreshTokensLoaded() will fail if the primary is not
available
> yet?

Done.

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service.cc:80: const std::string& account_id,
On 2013/08/29 15:41:40, Roger Tawa wrote:
> nit: for consistency, put account_id ahead of chrome_client_id and
> chrome_client_secret?  Same for ctor below.

Done.

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/75001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service_unittest.cc:93: std::string account_id_;
On 2013/08/29 15:41:40, Roger Tawa wrote:
> This member is not initialized anywhere.  Maybe set it something like
> "user@gmail.com" in SetUp() ?

Done.

[Roger Tawa OOO till Jul 10th, 2013-08-30 00:40:07]

A couple of answers below.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:63: &&
authenticated_username == account_id;
On 2013/08/29 23:04:14, Filip Gorski wrote:
> On 2013/08/29 15:41:40, Roger Tawa wrote:
> > Shouldn't this check the underlying android account manager to see if an
> account
> > is available?  Even the old code before your change seems incorrect.  I'll
ask
> > Tommy to take a look.
> 
> You are correct, however there are 2 ways to go about that:
> 1. If we want all of the accounts in Android to be visible here, we check with
> system account manager.
> 2. Otherwise we filter it down only to the accounts that we want to expose in
> Chrome, and the logic had to be a bit more sophisticated.
> 
> I didn't want to solve the issue here (due to size of CL), hence went with a
> simple check for a primary account. 

The goal is to expose all the android accounts here, yes.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/fak...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:42: }
On 2013/08/29 23:04:14, Filip Gorski wrote:
> On 2013/08/29 15:41:40, Roger Tawa wrote:
> > I'm not convinced that FakePO2TS should allow itself to be created without
> being
> > initialized.  That's what I did in
https://codereview.chromium.org/19567004/,
> > but I need to make sure all test still pass with this change.
> 
> I'll happily adjust, but I was trying to respect the comment, with a relevant
> piece of code.

I still need to make sure what I did in my CL will actually work :-)  If so,
then it might be worth adjusting here.

[Andrew T Wilson (Slow), 2013-08-30 10:47:00]

OK, need to run catch a plane, but I've started reviewing this. I'll try to
finish at my next layover if I have time.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:368:
chromeos::DeviceOAuth2TokenServiceFactory::Get()->GetRefreshToken(
The fact that there's this unnecessary account_id parameter to
DeviceOAuth2TokenService indicates that we don't quite have the right level of
abstraction - I think the API would have been cleaner if we'd gone down the path
of having separate OAuth2TokenServices per signed-in account (so each O2TS is
inherently tied to a specific account ID).

But I'm OK with this change as-is -- it's workable and we can always do some
future refactoring if this turns out to be cumbersome.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc
(right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc:92: request_
= token_service_->StartRequest(token_service_->GetPrimaryAccountId(),
I think the wrapping is wrong here - either you should wrap the first param to
the next line, or align all params with the open paren.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:238: account_id,
scopes, validating_consumer.get());
Yeah, this stuff needs to be moved to FetchOAuth2Token() since it's possible to
bypass it by using one of the other StartRequestXXXX APIs.

Not as part of this CL, though.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:42: virtual
scoped_ptr<Request> StartRequest(const std::string& account_id,
Can you add a TODO and file a bug to not OVERRIDE this? It doesn't make sense to
just override StartRequest() without overriding all the other APIs (like
StartRequestForClient/StartRequestWithContext). I think the right place to
override is FetchOAuth2Token().

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:52: virtual
std::string GetRefreshToken(const std::string& account_id) OVERRIDE;
Document that account_id *must* be empty since we don't support multiple
accounts? Or else, maybe enforce that the account_id passed in matches the
device service account ID?

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:166:
ASSERT_EQ("", oauth2_service_.GetRefreshToken(std::string()));
Would be really nice if DeviceOAuth2TokenService() exposed something like
GetDeviceAccountID() and we used that instead of an empty string for all these.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/driv...
File chrome/browser/drive/drive_api_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/driv...
chrome/browser/drive/drive_api_service.cc:344: new
google_apis::AuthService(oauth2_token_service_, account_id_,
I don't think this is wrapped correctly

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/gdat...
File chrome/browser/drive/gdata_wapi_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/gdat...
chrome/browser/drive/gdata_wapi_service.cc:165: new
AuthService(oauth2_token_service_, account_id_,
Wrong wrap?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Subsequent lines need to align with the first param.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:225: std::string(),
scope_set, this);
See my previous comment about adding GetDeviceAccountID() instead of passing
empty strings.

[Roger Tawa OOO till Jul 10th, 2013-08-30 18:20:50]

https://codereview.chromium.org/23382008/diff/39059/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.h (right):

https://codereview.chromium.org/23382008/diff/39059/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.h:13: #include
"chrome/browser/signin/profile_oauth2_token_service.h"
btw, you'll need to update the invalidation factory to make sure it depends on
PO2TS and not TS.  This may be true for other PKSs that you are changing.

[fgorski, 2013-08-30 20:10:16]

Didn't notice the comments didn't get published.

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/75001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc:127:
std::string oauth2_access_token) {
On 2013/08/29 15:41:40, Roger Tawa wrote:
> nit: fix indent.

Done.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:368:
chromeos::DeviceOAuth2TokenServiceFactory::Get()->GetRefreshToken(
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> The fact that there's this unnecessary account_id parameter to
> DeviceOAuth2TokenService indicates that we don't quite have the right level of
> abstraction - I think the API would have been cleaner if we'd gone down the
path
> of having separate OAuth2TokenServices per signed-in account (so each O2TS is
> inherently tied to a specific account ID).
> 
> But I'm OK with this change as-is -- it's workable and we can always do some
> future refactoring if this turns out to be cumbersome.

Done. Using GetDeviceAccountId().

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc
(right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc:92: request_
= token_service_->StartRequest(token_service_->GetPrimaryAccountId(),
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> I think the wrapping is wrong here - either you should wrap the first param to
> the next line, or align all params with the open paren.

Done.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:238: account_id,
scopes, validating_consumer.get());
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> Yeah, this stuff needs to be moved to FetchOAuth2Token() since it's possible
to
> bypass it by using one of the other StartRequestXXXX APIs.
> 
> Not as part of this CL, though.

crbug.com/282454.
There is the aspect of validation here, that is not easily movable (as I
mentioned in an email earlier, and the bug), but I think we could solve it in a
way that makes a clear separation between acquiring the access token and
validating the refresh token:
* we could launch the validation of the refresh token, when it is provided, so
that by the time it is required to get the access token, it is already
validated.
* onrefreshtokenavailable is only sent out once the validation was successful
* GetRefreshToken checks for validity of the token and only returns a token,
when it is valid.

(Updated the bug with the comments above)

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:42: virtual
scoped_ptr<Request> StartRequest(const std::string& account_id,
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> Can you add a TODO and file a bug to not OVERRIDE this? It doesn't make sense
to
> just override StartRequest() without overriding all the other APIs (like
> StartRequestForClient/StartRequestWithContext). I think the right place to
> override is FetchOAuth2Token().

Done. crbug.com/282454

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:52: virtual
std::string GetRefreshToken(const std::string& account_id) OVERRIDE;
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> Document that account_id *must* be empty since we don't support multiple
> accounts? Or else, maybe enforce that the account_id passed in matches the
> device service account ID?

I went with your comment to introduce GetDeviceAccountId and propagated that
thought in the documentation of the method.

I've also added a comment to the class documentation, that sets the expectation.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:166:
ASSERT_EQ("", oauth2_service_.GetRefreshToken(std::string()));
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> Would be really nice if DeviceOAuth2TokenService() exposed something like
> GetDeviceAccountID() and we used that instead of an empty string for all
these.

Done. Went with GetDeviceAccountId (over ID) for API consistency.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/driv...
File chrome/browser/drive/drive_api_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/driv...
chrome/browser/drive/drive_api_service.cc:344: new
google_apis::AuthService(oauth2_token_service_, account_id_,
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> I don't think this is wrapped correctly

Done.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/gdat...
File chrome/browser/drive/gdata_wapi_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/drive/gdat...
chrome/browser/drive/gdata_wapi_service.cc:165: new
AuthService(oauth2_token_service_, account_id_,
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> Wrong wrap?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> Subsequent lines need to align with the first param.

Done.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:225: std::string(),
scope_set, this);
On 2013/08/30 10:47:00, Andrew T Wilson wrote:
> See my previous comment about adding GetDeviceAccountID() instead of passing
> empty strings.

Done.

[fgorski, 2013-08-30 22:29:02]

Rewrote the AndroidPO2TS.

Rolled back the DCHECK on signin_manager => we currently call
GetForProfileIfExists. I think we can introduce the check for empty account_id
in a separate CL.

As for invalidation service, all is taken care of.

https://codereview.chromium.org/23382008/diff/39059/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.h (right):

https://codereview.chromium.org/23382008/diff/39059/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.h:13: #include
"chrome/browser/signin/profile_oauth2_token_service.h"
On 2013/08/30 18:20:50, Roger Tawa wrote:
> btw, you'll need to update the invalidation factory to make sure it depends on
> PO2TS and not TS.  This may be true for other PKSs that you are changing.

That was actually the case before my change (valid because
InvalidationServiceFactory already depends on PO2TS, and its transitive). My
change already removes Token Service from TiclInvalidatoinService.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:66: return
(bool)refresh_token_is_available;
I am not sure what the preferred treatment of the conversion from jboolean to
bool is, assuming above is correct.

I considered the following alternate implementation: call GetAccounts and then
see if the account_id is present, but in that case we are calling much heavier
method, which didn't make sense.

[nyquist, 2013-08-31 17:51:09]

https://codereview.chromium.org/23382008/diff/60002/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AndroidProfileOAuth2TokenServiceHelper.java
(right):

https://codereview.chromium.org/23382008/diff/60002/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/signin/AndroidProfileOAuth2TokenServiceHelper.java:154:
return accountManagerHelper.getAccountForName(accountName) != null;
Could you extract this to a new method in AccountManagerHelper?

public boolean hasAccountForName(String accountName) {
  return getAccountForName(accountName) != null;
}

And then call that method from here?

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:66: return
(bool)refresh_token_is_available;
On 2013/08/30 22:29:03, Filip Gorski wrote:
> I am not sure what the preferred treatment of the conversion from jboolean to
> bool is, assuming above is correct.
> 
> I considered the following alternate implementation: call GetAccounts and then
> see if the account_id is present, but in that case we are calling much heavier
> method, which didn't make sense.

This is what what you do does behind the scenes now anyway, but it is better
when this happens in the AccountManagerHelper instead.

[Andrew T Wilson (Slow), 2013-09-03 07:27:58]

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:169: Logout();
I'm fine with this, but I wonder if maybe we should be hooking something like
"OnRefreshTokenRevoked()" instead? Why do we hook this instead of the other?

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:265:
UpdateInvalidatorCredentials();
Why do we not Logout() here?

[Andrew T Wilson (Slow), 2013-09-03 14:04:24]

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:120:
token_service_->GetRefreshToken(std::string()),
Should this really be an empty string?

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:431:
service->StartRequestForClient(std::string(),
Why is this an empty string? Shouldn't this be the primary account ID?

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/local_disc...
File chrome/browser/local_discovery/privet_confirm_api_flow_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/local_disc...
chrome/browser/local_discovery/privet_confirm_api_flow_unittest.cc:91:
std::string(),
This is fine - I just wonder if perhaps we want to prevent ever passing in an
empty account ID to OAuth2TokenService, so we should codify that with a DCHECK()
and make tests pass in a non-empty test acct ID. Your call - just wanted to
suggest it.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:40: const
std::string& account_id,
This is OK and is consistent with the API changes we're making, but note that
ManagedUsers don't really have account_ids, so we'll have to invent an account
ID for this class (similar to how we had to invent an account ID for
DeviceOAuth2TokenService) - this is yet another indication that OA2TS really
doesn't want to be multi-account aware because there are at least two instances
where account_id doesn't make sense.

Not a huge deal - just a minor API wart that probably doesn't warrant doing a
big refactoring, but wanted to point it out.

The alternative would be to have multiple OAuth2TokenService instances
associated with a single profile, and instead of adding plumbing where we pass
an account_id through all of these APIs, we instead have something like this:

ProfileOAuth2TokenServiceManager* manager =
ProfileOAuth2TokenServiceManager::GetForProfile(profile);
ProfileOAuth2TokenService* service =
manager->GetTokenServiceForAccount(manager->GetPrimaryAccountId());
if (service) {
  // This account is signed in...
  service->StartRequest(scopes, consumer);
}

That way we don't have account ID polluting the StartRequest() API when many of
the subclasses don't support the concept of accounts.

Again, just a suggestion - we can definitely live with it the way it is.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_registration_utility.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_registration_utility.cc:139:
token_service->GetPrimaryAccountId(),
you might reach out to bauerb about this - it's not clear to me that we have a
"primary account" for managed profiles, per se.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc
(right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc:74:
token_service->RefreshTokenIsAvailable(account_id));
I think it's more correct to align with the open paren of the previous line.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h
(left):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h:53:
AndroidProfileOAuth2TokenService* token_service,
yay

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:89:
j_accounts.obj(),
Is the idea that we'll ultimately give the user control over which subset of his
accounts are "signed in" to chrome? As it stands, we just iterate over *all*
accounts on the device, which might not be the right behavior (for example, I
don't automatically sync gmail for every account on my device). Maybe there's
some more work involved here we should add a TODO() for?

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:111: // Allocate a
copy of the request WeakPtr on the heap, because the object
nit: indent

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/fak...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:66: void
IssueTokenForScope(const ScopeSet& scopes,
May need to add a username for all of these IssueXXXX() APIs? Not a huge deal,
but there's not any way to only generate an error for a single account
otherwise.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.h:65: std::string
GetPrimaryAccountId();
Roger seemed to think maybe this should live elsewhere (like maybe in
SigninManager?) - I'm fine with this here, but maybe ping him?

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:263:
EXPECT_TRUE(find(accounts.begin(), accounts.end(), "account_id1") !=
nit: consider instead:

EXPECT_EQ(1u, accounts.count("account_id1"));

Maybe a little more compact/readable.

[fgorski, 2013-09-03 20:50:38]

Addressing most of the comments and fixing an issue.

https://codereview.chromium.org/23382008/diff/60002/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AndroidProfileOAuth2TokenServiceHelper.java
(right):

https://codereview.chromium.org/23382008/diff/60002/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/signin/AndroidProfileOAuth2TokenServiceHelper.java:154:
return accountManagerHelper.getAccountForName(accountName) != null;
On 2013/08/31 17:51:10, nyquist wrote:
> Could you extract this to a new method in AccountManagerHelper?
> 
> public boolean hasAccountForName(String accountName) {
>   return getAccountForName(accountName) != null;
> }
> 
> And then call that method from here?

Done.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:120:
token_service_->GetRefreshToken(std::string()),
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> Should this really be an empty string?

Done.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:431:
service->StartRequestForClient(std::string(),
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> Why is this an empty string? Shouldn't this be the primary account ID?

Done. You are right, it should.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/local_disc...
File chrome/browser/local_discovery/privet_confirm_api_flow_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/local_disc...
chrome/browser/local_discovery/privet_confirm_api_flow_unittest.cc:91:
std::string(),
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> This is fine - I just wonder if perhaps we want to prevent ever passing in an
> empty account ID to OAuth2TokenService, so we should codify that with a
DCHECK()
> and make tests pass in a non-empty test acct ID. Your call - just wanted to
> suggest it.

Done. I agree with you. Roger mentioned a potential DCHECK earlier I just never
got down to revisiting all of the calls. Given that GetRefreshToken is simply
returning tokens here, I am using a dummy account_id.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_registration_utility.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_registration_utility.cc:139:
token_service->GetPrimaryAccountId(),
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> you might reach out to bauerb about this - it's not clear to me that we have a
> "primary account" for managed profiles, per se.

I've sent an email to bauerb@ with questions related to
ManagedUserRefreshTokenFetcher.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc
(right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc:74:
token_service->RefreshTokenIsAvailable(account_id));
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> I think it's more correct to align with the open paren of the previous line.

Done.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h
(left):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h:53:
AndroidProfileOAuth2TokenService* token_service,
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> yay

Done.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:66: return
(bool)refresh_token_is_available;
On 2013/08/31 17:51:10, nyquist wrote:
> On 2013/08/30 22:29:03, Filip Gorski wrote:
> > I am not sure what the preferred treatment of the conversion from jboolean
to
> > bool is, assuming above is correct.
> > 
> > I considered the following alternate implementation: call GetAccounts and
then
> > see if the account_id is present, but in that case we are calling much
heavier
> > method, which didn't make sense.
> 
> This is what what you do does behind the scenes now anyway, but it is better
> when this happens in the AccountManagerHelper instead.

Done.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:89:
j_accounts.obj(),
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> Is the idea that we'll ultimately give the user control over which subset of
his
> accounts are "signed in" to chrome? As it stands, we just iterate over *all*
> accounts on the device, which might not be the right behavior (for example, I
> don't automatically sync gmail for every account on my device). Maybe there's
> some more work involved here we should add a TODO() for?

I raised that with Roger in a phone call before I started Android related work
and we made a call to actually list all of the accounts. At least for now. I am
adding a TODO.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:111: // Allocate a
copy of the request WeakPtr on the heap, because the object
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> nit: indent

Done.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/fak...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:66: void
IssueTokenForScope(const ScopeSet& scopes,
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> May need to add a username for all of these IssueXXXX() APIs? Not a huge deal,
> but there's not any way to only generate an error for a single account
> otherwise.

Added a TODO, will not change that as part of this CL.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:263:
EXPECT_TRUE(find(accounts.begin(), accounts.end(), "account_id1") !=
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> nit: consider instead:
> 
> EXPECT_EQ(1u, accounts.count("account_id1"));
> 
> Maybe a little more compact/readable.

I did, but std::vector does not have a count.
I used algorithm count instead to make the line more concise.
EXPECT_EQ(1u, count(accounts.begin(), accounts.end(), "account_id1"));
Does that make sense?

[Roger Tawa OOO till Jul 10th, 2013-09-03 21:06:41]

Hi guys,

A few initial comments, will continue review.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:40: const
std::string& account_id,
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> This is OK and is consistent with the API changes we're making, but note that
> ManagedUsers don't really have account_ids, so we'll have to invent an account
> ID for this class (similar to how we had to invent an account ID for
> DeviceOAuth2TokenService) - this is yet another indication that OA2TS really
> doesn't want to be multi-account aware because there are at least two
instances
> where account_id doesn't make sense.
> 
> Not a huge deal - just a minor API wart that probably doesn't warrant doing a
> big refactoring, but wanted to point it out.
> 
> The alternative would be to have multiple OAuth2TokenService instances
> associated with a single profile, and instead of adding plumbing where we pass
> an account_id through all of these APIs, we instead have something like this:
> 
> ProfileOAuth2TokenServiceManager* manager =
> ProfileOAuth2TokenServiceManager::GetForProfile(profile);
> ProfileOAuth2TokenService* service =
> manager->GetTokenServiceForAccount(manager->GetPrimaryAccountId());
> if (service) {
>   // This account is signed in...
>   service->StartRequest(scopes, consumer);
> }
> 
> That way we don't have account ID polluting the StartRequest() API when many
of
> the subclasses don't support the concept of accounts.
> 
> Again, just a suggestion - we can definitely live with it the way it is.

Agreed that the |account_id| is not needed here, since its always the account
connected to the profile that counts, i.e., the primary account.  I don't think
this has any bearing on the O2TS api though.

The OAuth2TokenService today is acting like both the manager you describe and
token service instances.  But as you say, I don't think this warrants a big
refactoring.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.h:65: std::string
GetPrimaryAccountId();
On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> Roger seemed to think maybe this should live elsewhere (like maybe in
> SigninManager?) - I'm fine with this here, but maybe ping him?

As a public api, seems like its better if it live with SM.  Or maybe a static
like:

static std::string XXX::GetPrimaryAccount(Profile* profile);

where XXX could be SigninManager or something else.

[Bernhard Bauer, 2013-09-03 21:18:38]

https://codereview.chromium.org/23382008/diff/116001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/116001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:46:
const char kAccountId[] = "account_id";
Can you move this up to the first block of constants? Those are the constants
for the client.

https://codereview.chromium.org/23382008/diff/116001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:130:
account_id_,
If we don't care about the account ID otherwise, you could just pass the
constant here directly.

[fgorski, 2013-09-03 22:03:04]

https://codereview.chromium.org/23382008/diff/116001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/116001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:46:
const char kAccountId[] = "account_id";
On 2013/09/03 21:18:38, Bernhard Bauer wrote:
> Can you move this up to the first block of constants? Those are the constants
> for the client.

Done.

https://codereview.chromium.org/23382008/diff/116001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:130:
account_id_,
On 2013/09/03 21:18:38, Bernhard Bauer wrote:
> If we don't care about the account ID otherwise, you could just pass the
> constant here directly.

Done.

[Bernhard Bauer, 2013-09-03 22:13:05]

https://codereview.chromium.org/23382008/diff/89001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/89001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:26:
const char kManagedUserId[] = "abcdef";
I meant here :)

Also, could you please alphabetize the constants in this block while you're
here?

[fgorski, 2013-09-03 23:29:24]

Addressing outstanding comments.

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/invalidati...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:169: Logout();
On 2013/09/03 07:27:59, Andrew T Wilson wrote:
> I'm fine with this, but I wonder if maybe we should be hooking something like
> "OnRefreshTokenRevoked()" instead? Why do we hook this instead of the other?

Drew, I don't know this class that well yet. I am trying to make the least
amount of changes possible (which I am also likely failing at). Can we rethink
the TiclInvalidationService as part of a separate CL?

https://codereview.chromium.org/23382008/diff/90001/chrome/browser/invalidati...
chrome/browser/invalidation/ticl_invalidation_service.cc:265:
UpdateInvalidatorCredentials();
On 2013/09/03 07:27:59, Andrew T Wilson wrote:
> Why do we not Logout() here?

As per comment above. I am trying to recreate a logic from before the change and
not make an update to the class.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:40: const
std::string& account_id,
On 2013/09/03 21:06:42, Roger Tawa wrote:
> On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> > This is OK and is consistent with the API changes we're making, but note
that
> > ManagedUsers don't really have account_ids, so we'll have to invent an
account
> > ID for this class (similar to how we had to invent an account ID for
> > DeviceOAuth2TokenService) - this is yet another indication that OA2TS really
> > doesn't want to be multi-account aware because there are at least two
> instances
> > where account_id doesn't make sense.
> > 
> > Not a huge deal - just a minor API wart that probably doesn't warrant doing
a
> > big refactoring, but wanted to point it out.
> > 
> > The alternative would be to have multiple OAuth2TokenService instances
> > associated with a single profile, and instead of adding plumbing where we
pass
> > an account_id through all of these APIs, we instead have something like
this:
> > 
> > ProfileOAuth2TokenServiceManager* manager =
> > ProfileOAuth2TokenServiceManager::GetForProfile(profile);
> > ProfileOAuth2TokenService* service =
> > manager->GetTokenServiceForAccount(manager->GetPrimaryAccountId());
> > if (service) {
> >   // This account is signed in...
> >   service->StartRequest(scopes, consumer);
> > }
> > 
> > That way we don't have account ID polluting the StartRequest() API when many
> of
> > the subclasses don't support the concept of accounts.
> > 
> > Again, just a suggestion - we can definitely live with it the way it is.
> 
> Agreed that the |account_id| is not needed here, since its always the account
> connected to the profile that counts, i.e., the primary account.  I don't
think
> this has any bearing on the O2TS api though.
> 
> The OAuth2TokenService today is acting like both the manager you describe and
> token service instances.  But as you say, I don't think this warrants a big
> refactoring.

If we decide not to go with account_id here we'll have to use
ProfileOAuth2TokenService instead of OAuth2TokenService. I think it is best to
make a change once bauerb comes up with ManagedUserOAuth2TokenService and
exactly what is needed then.

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/60002/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.h:65: std::string
GetPrimaryAccountId();
On 2013/09/03 21:06:42, Roger Tawa wrote:
> On 2013/09/03 14:04:24, Andrew T Wilson wrote:
> > Roger seemed to think maybe this should live elsewhere (like maybe in
> > SigninManager?) - I'm fine with this here, but maybe ping him?
> 
> As a public api, seems like its better if it live with SM.  Or maybe a static
> like:
> 
> static std::string XXX::GetPrimaryAccount(Profile* profile);
> 
> where XXX could be SigninManager or something else.

OK, I'll make a change tomorrow and retest everything. There are might be places
thou, where we are not using SigninManager explicitly yet, and direct dependency
will have to be added (through header file inclusion).

[nyquist, 2013-09-05 00:42:43]

https://codereview.chromium.org/23382008/diff/162001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/23382008/diff/162001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:139: *
Returns true, if the account exists, false otherwise.
Returns whether the account exists.

[fgorski, 2013-09-05 19:25:54]

Addressing comments, adding more reviewers:

xiyuan@:
chrome/browser/chromeos/app_mode
chrome/browser/invalidation

satorux@:
chrome/browser/chromeos/contacts
chrome/browser/chromeos/drive
chrome/browser/drive
chrome/browser/google_apis

dpolukhin@:
chrome/browser/chromeos/login

mnissler@:
chrome/browser/chromeos/settings

dcheng@:
chrome/browser/extensions/api/push_messaging/
chrome/browser/invalidation (shared with nyquist)

miket@:
chrome/browser/extensiosn/extension_install_prompt.cc

sky@:
chrome/browser/history/
chrome/browser/ui

gene@:
chrome/browser/local_discovery

rlp@:
chrome/browser/profiles/

kinuko@:
chrome/browser/sync_file_system/

courage@:
crhome/browser/extensions/api/identity

bauerb@:
chrome/browser/managed_mode

nyquist@:
sync/android/

atwilson@:
chrome/browser/signin
chrome/browser/sync
chrome/browser/policy

rogerta@:
google_apis/gaia
chrome/browser/signin
chrome/browser/sync

https://codereview.chromium.org/23382008/diff/89001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/23382008/diff/89001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:26:
const char kManagedUserId[] = "abcdef";
On 2013/09/03 22:13:06, Bernhard Bauer wrote:
> I meant here :)
> 
> Also, could you please alphabetize the constants in this block while you're
> here?

Done.

https://codereview.chromium.org/23382008/diff/162001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/23382008/diff/162001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:139: *
Returns true, if the account exists, false otherwise.
On 2013/09/05 00:42:44, nyquist wrote:
> Returns whether the account exists.

Done.

[Michael Courage, 2013-09-05 20:20:58]

chrome/browser/extensions/api/identity lgtm
A couple of drive-by comments elsewhere.

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:229: //
ClearCacheForAccount(account_id);
Is this meant to be commented out? Maybe there's supposed to be a TODO here?

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:118: // Method responsible
for persistence of credentials. Enables overriding for
I'd probably remove the first sentence here (PersistCredentials pretty much says
it all), or match the phrasing in the RevokeCredentials comment:
// Persists credentials for |account_id|.

The comma after "other cases" should go.

[xiyuan, 2013-09-05 20:29:42]

chrome/browser/chromeos/app_mode LGTM

[nyquist, 2013-09-05 22:59:53]

chrome/android/, sync/android/, chrome/browser/signin/android_* lgtm

[Dmitry Polukhin, 2013-09-05 23:07:02]

LGTM of chrome/browser/chromeos/login

[gene, 2013-09-05 23:16:31]

LGTM of chrome/browser/local_discovery

[gene, 2013-09-05 23:16:37]

LGTM of chrome/browser/local_discovery

[miket_OOO, 2013-09-05 23:25:03]

> miket@:
> chrome/browser/extensiosn/extension_install_prompt.cc

LGTM this one file.

[sky, 2013-09-05 23:43:21]

LGTM

[satorux1, 2013-09-06 00:56:51]

LGTM

[dcheng, 2013-09-06 01:27:33]

chrome/browser/invalidation/* and chrome/browser/extensions/api/push_messaging/*
LGTM.

It may be nice to update the description to mention that you're updating most
(all?) locations to just use the primary login token for now.

[kinuko, 2013-09-06 04:19:51]

*/sync_file_system/* lgtm

[Bernhard Bauer, 2013-09-06 05:38:34]

managed_mode LGTM

[Andrew T Wilson (Slow), 2013-09-06 09:23:09]

chrome/browser/signin + policy + sync lgtm with a few nits

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:143:
refresh_tokens_[account_id] = tok_details->token();
nit: I'd probably update refresh_tokens_[] before calling UpdateAuthError() in
case someone who is listening for auth error changes also wants to use the new
refresh token...

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:506: DLOG(INFO) <<
"In setup";
Remove

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:547:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get())
BTW, might be nice if this were moved out into a helper function since it's
basically copy/pasted into every test function. Feel free not to do this since
it's pre-existing code, but just in case you're looking to do more cleanup :)

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_unittest.cc:297:
signin->SetAuthenticatedUsername("test");
Why did we change the username here?

[Mattias Nissler (ping if slow), 2013-09-06 09:34:46]

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:220:
device_account_id_(kDeviceAccountId) {
I think this is wrong. We do have a proper account ID, which is returned by
GetRobotAccountID(). Please use that instead.

https://codereview.chromium.org/23382008/diff/117001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/117001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service.h:110: virtual scoped_ptr<Request>
StartRequest(const std::string& account_id,
It'd be helpful to explain how account_id affects behavior of this, in
particular explaining that we may have different tokens associated with
different accounts even if |scopes| is identical.

[Roger Tawa OOO till Jul 10th, 2013-09-06 17:58:14]

Looks good Filip.  I think you just need to merge with Zel's change and update
the caching code.

[rpetterson, 2013-09-09 20:40:32]

c/b/profiles LGTM, in case you still need it.

[fgorski, 2013-09-12 23:46:22]

Rebased, updated based on the comments, and cache handling added.

PTAL gene@ (local_discovery), mnissler@ (DeviceO2TS), atwilson@ (new code in
PO2TS and O2TS), rogerta@ (new code in PO2TS and O2TS)

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:229: //
ClearCacheForAccount(account_id);
On 2013/09/05 20:20:59, Michael Courage wrote:
> Is this meant to be commented out? Maybe there's supposed to be a TODO here?

Done. Changing to a TODO for now.

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/118001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:118: // Method responsible
for persistence of credentials. Enables overriding for
On 2013/09/05 20:20:59, Michael Courage wrote:
> I'd probably remove the first sentence here (PersistCredentials pretty much
says
> it all), or match the phrasing in the RevokeCredentials comment:
> // Persists credentials for |account_id|.
> 
> The comma after "other cases" should go.

Done.

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:220:
device_account_id_(kDeviceAccountId) {
On 2013/09/06 09:34:48, Mattias Nissler wrote:
> I think this is wrong. We do have a proper account ID, which is returned by
> GetRobotAccountID(). Please use that instead.

Done.

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:143:
refresh_tokens_[account_id] = tok_details->token();
On 2013/09/06 09:23:10, Andrew T Wilson wrote:
> nit: I'd probably update refresh_tokens_[] before calling UpdateAuthError() in
> case someone who is listening for auth error changes also wants to use the new
> refresh token...

Done.

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:506: DLOG(INFO) <<
"In setup";
On 2013/09/06 09:23:10, Andrew T Wilson wrote:
> Remove

Done.

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:547:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get())
On 2013/09/06 09:23:10, Andrew T Wilson wrote:
> BTW, might be nice if this were moved out into a helper function since it's
> basically copy/pasted into every test function. Feel free not to do this since
> it's pre-existing code, but just in case you're looking to do more cleanup :)

Done. Extracted these 4 lines to a method IssueTestTokens, following other unit
test classes for profile sync service. Also, the value of the token is unified
(between "sync_token" and "token", as it is irrelevant).

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/117001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_unittest.cc:297:
signin->SetAuthenticatedUsername("test");
On 2013/09/06 09:23:10, Andrew T Wilson wrote:
> Why did we change the username here?

Sync service does not care, as long as it is set, but if you look at
IssueTestTokens, only account_id = "test" is set a refresh token. For
ProfileOAuth2TokenService that matters and the change is meant to fix a failing
unit test.

https://codereview.chromium.org/23382008/diff/117001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/117001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service.h:110: virtual scoped_ptr<Request>
StartRequest(const std::string& account_id,
On 2013/09/06 09:34:48, Mattias Nissler wrote:
> It'd be helpful to explain how account_id affects behavior of this, in
> particular explaining that we may have different tokens associated with
> different accounts even if |scopes| is identical.

Done. I've updated the documentation in the class to reflect that aspect of the
change.

[gene, 2013-09-13 05:49:31]

lgtm for local_discovery

[Roger Tawa OOO till Jul 10th, 2013-09-13 14:53:54]

All looks good for O2TS and PO2TS, except for the RequestParameters struct.  It
should use |account_id| instead of |refresh_token| as a member.  The reason is
that you need to build a RequestParameters struct in the following methods:

StartRequestForClientWithContext
InvalidateToken
ClearCacheForAccount
(some other functions too)

To do so, you need to call GetRefreshToken().  On some platforms, like mobile,
this returns nothing or undefined.  So either you need to reimplement these
methods on those platforms, or make sure they are never called on those
platforms, or change RequestParameters.  I think the last option is preferable.

Also a some minor comments below.

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:72:
OAuth2TokenService::InvalidateToken(scopes, invalid_token);
This is an access token not a refresh token. Would be good rename the argument
to the right one.

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.h:35: const
std::string& invalid_token) OVERRIDE;
I think this function needs an |account_id| arg.

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:296: //
account_id or a refresh_token before that starts working.
Does this test still need to be disabled?  You have fixed the comment in the
TODO with the latest patchset.

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service.cc:461: GetRefreshToken(account_id),
GetRefreshToken() won't work on mobile platforms.  It will return an empty
string.  I wonder if it would be better to replace the |refresh_token| member of
RequestParameters with |account_id| instead, since there is a 1-to-1 mapping
between the two.

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service.h:161: const std::string& invalid_token);
Should rename |invalid_token| to |invalid_access_token| or something, just to
make it clearer in the code this is not a refresh token.  I do see that the
function comment states that this is an access token which is good.  Same for
InvalidateOAuth2Token below.

[fgorski, 2013-09-13 22:06:45]

Updated based on comments from Roger.
If we remove refresh_token from RequestParameters in favor of account_id we are
causing the fetchers to be overwritten after refresh token update. We could have
both and then it would work on both desktop and mobile platforms.

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:72:
OAuth2TokenService::InvalidateToken(scopes, invalid_token);
On 2013/09/13 14:53:56, Roger Tawa wrote:
> This is an access token not a refresh token. Would be good rename the argument
> to the right one.

Done. Renamed to access_token and propagated to OAuth2TokenService

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.h:35: const
std::string& invalid_token) OVERRIDE;
On 2013/09/13 14:53:56, Roger Tawa wrote:
> I think this function needs an |account_id| arg.

Done. Changed to implement the virtual protected InvalidateOAuth2Token().

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/23382008/diff/56001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:296: //
account_id or a refresh_token before that starts working.
On 2013/09/13 14:53:56, Roger Tawa wrote:
> Does this test still need to be disabled?  You have fixed the comment in the
> TODO with the latest patchset.

Done. Good catch. Test actually works again.

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service.cc:461: GetRefreshToken(account_id),
On 2013/09/13 14:53:56, Roger Tawa wrote:
> GetRefreshToken() won't work on mobile platforms.  It will return an empty
> string.  I wonder if it would be better to replace the |refresh_token| member
of
> RequestParameters with |account_id| instead, since there is a 1-to-1 mapping
> between the two.

Done. But using only account id breaks the in flight fetchers in a sense. We can
either get back to 2 different structures or remove the call to cancel fetchers
in PO2TS. Also without a refresh token as part of the structure, we are risking
hooking up new consumers to an in progress call with an old token.

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service.h:161: const std::string& invalid_token);
On 2013/09/13 14:53:56, Roger Tawa wrote:
> Should rename |invalid_token| to |invalid_access_token| or something, just to
> make it clearer in the code this is not a refresh token.  I do see that the
> function comment states that this is an access token which is good.  Same for
> InvalidateOAuth2Token below.

Done.

[Mattias Nissler (ping if slow), 2013-09-16 09:49:19]

chrome/browser/policy, chrome/browser/chromeos/policy,
chrome/browser/chromeos/settings LGTM now.

[Roger Tawa OOO till Jul 10th, 2013-09-16 17:31:49]

lgtm

Filip and talked about the inflight fetchers and I don't think that's a problem.

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
google_apis/gaia/oauth2_token_service.cc:461: GetRefreshToken(account_id),
On 2013/09/13 22:06:46, Filip Gorski wrote:
> On 2013/09/13 14:53:56, Roger Tawa wrote:
> > GetRefreshToken() won't work on mobile platforms.  It will return an empty
> > string.  I wonder if it would be better to replace the |refresh_token|
member
> of
> > RequestParameters with |account_id| instead, since there is a 1-to-1 mapping
> > between the two.
> 
> Done. But using only account id breaks the in flight fetchers in a sense. We
can
> either get back to 2 different structures or remove the call to cancel
fetchers
> in PO2TS. Also without a refresh token as part of the structure, we are
risking
> hooking up new consumers to an in progress call with an old token.

What scenario are you thinking of?  Something like:

1/ StartRequest("foo@gmail.com", "scope1 scope2");
2/ UpdateCredentials("foo@gmail.com", "new_refresh_token");
3/ StartRequest("foo@gmail.com", "scope1 scope2");

where 2 and 3 happen before the first StartRequest() completes?

Since UpdateCredentials() cancels all pending fetchers, any subsequent call for
a new access token will use a new fetcher.  Also, OnFetchComplete() will never
be called for the original request, so it can't overwrite the cache with bad
data.

[fgorski, 2013-09-16 18:06:29]

On 2013/09/16 17:31:49, Roger Tawa wrote:
> lgtm
> 
> Filip and talked about the inflight fetchers and I don't think that's a
problem.
> 
>
https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
> File google_apis/gaia/oauth2_token_service.cc (right):
> 
>
https://codereview.chromium.org/23382008/diff/56001/google_apis/gaia/oauth2_t...
> google_apis/gaia/oauth2_token_service.cc:461: GetRefreshToken(account_id),
> On 2013/09/13 22:06:46, Filip Gorski wrote:
> > On 2013/09/13 14:53:56, Roger Tawa wrote:
> > > GetRefreshToken() won't work on mobile platforms.  It will return an empty
> > > string.  I wonder if it would be better to replace the |refresh_token|
> member
> > of
> > > RequestParameters with |account_id| instead, since there is a 1-to-1
mapping
> > > between the two.
> > 
> > Done. But using only account id breaks the in flight fetchers in a sense. We
> can
> > either get back to 2 different structures or remove the call to cancel
> fetchers
> > in PO2TS. Also without a refresh token as part of the structure, we are
> risking
> > hooking up new consumers to an in progress call with an old token.
> 
> What scenario are you thinking of?  Something like:
> 
> 1/ StartRequest("foo@gmail.com", "scope1 scope2");
> 2/ UpdateCredentials("foo@gmail.com", "new_refresh_token");
> 3/ StartRequest("foo@gmail.com", "scope1 scope2");
> 
> where 2 and 3 happen before the first StartRequest() completes?
> 
> Since UpdateCredentials() cancels all pending fetchers, any subsequent call
for
> a new access token will use a new fetcher.  Also, OnFetchComplete() will never
> be called for the original request, so it can't overwrite the cache with bad
> data.

I was referring to
OAuth2TokenServiceTest.ChangedRefreshTokenDoesNotAffectInFlightRequests
which is equivalent to the scenario described by you.
I've updated the test to capture the expectation.

If we reach conclusion that we should not cancel the in-flight fetchers, there
are 2 ways
to approach the problem:
* We can add refresh_token to the RequestParameters (it will be empty on mobile
platforms,
but we don't care since we don't control the fetchers there)
* We could update the fetcher code, to proactively grab a refresh token as it is
re-trying,
and it hits credential issue. That way we can always add a request to the
fetcher in progress,
and potentially reset the retry count, knowing that if there has been a
credential update
for the in-flight fetcher, it will recover from it with the next attempt. 

I've made a fix to the bug you've found and a piece of documentation that was
not
re-based correctly and attempting a CQ now.

[commit-bot: I haz the power, 2013-09-16 18:09:47]

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

[commit-bot: I haz the power, 2013-09-17 00:52:27]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[fgorski, 2013-09-18 21:00:50]

I fixed the UserPolicySigninService tests on Android. The code is currently run
by try jobs. I'll flip the bit for CQ later today.

atwilson@ please take a look at the policy section, that needed to be updated
after Patch 29.

[commit-bot: I haz the power, 2013-09-18 23:11:42]

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

[commit-bot: I haz the power, 2013-09-18 23:18:33]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-19 00:31:37]

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

[Andrew T Wilson (Slow), 2013-09-19 01:28:47]

LGTM - I actually didn't see any significant change in Patch 29. Can you ping me
to highlight what you changed, and if it's a problem we can fix it in a separate
CL?

[commit-bot: I haz the power, 2013-09-19 04:52:55]

Change committed as 224031

[fgorski, 2013-09-19 18:02:53]

On 2013/09/19 04:52:55, I haz the power (commit-bot) wrote:
> Change committed as 224031

Change was reverted.
Reopening

[commit-bot: I haz the power, 2013-09-19 20:52:01]

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

[commit-bot: I haz the power, 2013-09-19 23:02:40]

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

[commit-bot: I haz the power, 2013-09-20 01:38:05]

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

[commit-bot: I haz the power, 2013-09-20 01:52:50]

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

[commit-bot: I haz the power, 2013-09-20 05:41:04]

Change committed as 224294