Upstream iOS implementation of ProfileOAuth2TokenService

Upstream iOS implementation of ProfileOAuth2TokenService

This CL adds the iOS implementation of the ProfileOAuth2TokenService.

BUG=NONE

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

[blundell, 2014-04-10 13:47:43]

LGTM :)

https://codereview.chromium.org/226643012/diff/40001/components/signin/ios/br...
File components/signin/ios/browser/profile_oauth2_token_service_ios.h (right):

https://codereview.chromium.org/226643012/diff/40001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.h:20: //
ProfileOAuth2TokenServiceFactory for OS_IOS when iOS authentication service
nit: Change this comment to avoid mentioning the factory.

https://codereview.chromium.org/226643012/diff/40001/ios/public/provider/comp...
File
ios/public/provider/components/signin/browser/profile_oauth2_token_service_ios_provider.h
(right):

https://codereview.chromium.org/226643012/diff/40001/ios/public/provider/comp...
ios/public/provider/components/signin/browser/profile_oauth2_token_service_ios_provider.h:24:
class BrowserContextKeyedBaseFactory;
kill

https://codereview.chromium.org/226643012/diff/40001/ios/public/provider/comp...
ios/public/provider/components/signin/browser/profile_oauth2_token_service_ios_provider.h:44:
// Interface that provides iOS support for ProfileOAuth2TokenServiceIOS.
s/iOS support/support

[msarda, 2014-04-10 13:57:21]

https://codereview.chromium.org/226643012/diff/40001/components/signin/ios/br...
File components/signin/ios/browser/profile_oauth2_token_service_ios.h (right):

https://codereview.chromium.org/226643012/diff/40001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.h:20: //
ProfileOAuth2TokenServiceFactory for OS_IOS when iOS authentication service
On 2014/04/10 13:47:43, blundell wrote:
> nit: Change this comment to avoid mentioning the factory.

Done.

https://codereview.chromium.org/226643012/diff/40001/ios/public/provider/comp...
File
ios/public/provider/components/signin/browser/profile_oauth2_token_service_ios_provider.h
(right):

https://codereview.chromium.org/226643012/diff/40001/ios/public/provider/comp...
ios/public/provider/components/signin/browser/profile_oauth2_token_service_ios_provider.h:24:
class BrowserContextKeyedBaseFactory;
On 2014/04/10 13:47:43, blundell wrote:
> kill

Done.

https://codereview.chromium.org/226643012/diff/40001/ios/public/provider/comp...
ios/public/provider/components/signin/browser/profile_oauth2_token_service_ios_provider.h:44:
// Interface that provides iOS support for ProfileOAuth2TokenServiceIOS.
On 2014/04/10 13:47:43, blundell wrote:
> s/iOS support/support

Done.

[msarda, 2014-04-10 13:57:27]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2014-04-10 13:57:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/226643012/60001

[Roger Tawa OOO till Jul 10th, 2014-04-10 15:04:08]

lgtm

A few questions and nit.  I see this is already in the CQ, which is fine.

https://codereview.chromium.org/226643012/diff/60001/components/signin/core/b...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/226643012/diff/60001/components/signin/core/b...
components/signin/core/browser/signin_client.h:23: // core SigninClient.
Is this a bug you plan to fix soon?  Can you put more info in the bug as to why
this is here now and why you want to eventually remove it :-)

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
File components/signin/ios/browser/profile_oauth2_token_service_ios.h (right):

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.h:126: };
Is the only reason to duplicate this class here is because the one in MO2TS has
a refresh token member, and here we don't?

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.h:140:
AccountInfoMap accounts_;
Are all the following true?
|accounts_| is only used when use_legacy_token_service_== false.
When we switch from legacy to non-legacy, we always revoke all creds first.
Chrome does not allow switching from non-legacy to legacy.

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
File components/signin/ios/browser/profile_oauth2_token_service_ios.mm (right):

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.mm:455:
DCHECK(!account_id.empty());
Add dcheck?

    DCHECK(!use_legacy_token_service_);

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.mm:478:
DCHECK(!account_id.empty());
Add dcheck?

    DCHECK(!use_legacy_token_service_);

[msarda, 2014-04-10 15:13:35]

https://codereview.chromium.org/226643012/diff/60001/components/signin/core/b...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/226643012/diff/60001/components/signin/core/b...
components/signin/core/browser/signin_client.h:23: // core SigninClient.
On 2014/04/10 15:04:08, Roger Tawa wrote:
> Is this a bug you plan to fix soon?  Can you put more info in the bug as to
why
> this is here now and why you want to eventually remove it :-)

No, this is low priority. It was something Colin wanted me to add in here, but
this will become clearer if I need to remove once we do more componentization
and understand what is the best strategy to use for mutli-platform components.

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
File components/signin/ios/browser/profile_oauth2_token_service_ios.h (right):

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.h:126: };
On 2014/04/10 15:04:08, Roger Tawa wrote:
> Is the only reason to duplicate this class here is because the one in MO2TS
has
> a refresh token member, and here we don't?

You are pretty much true, but I think this is just an implementation detail.
This may diverge in the long run.

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.h:140:
AccountInfoMap accounts_;
On 2014/04/10 15:04:08, Roger Tawa wrote:
> Are all the following true?
> |accounts_| is only used when use_legacy_token_service_== false.
True.
> When we switch from legacy to non-legacy, we always revoke all creds first.
True. This is done in the iOS downstream call though, so it is not visible on
the .
> Chrome does not allow switching from non-legacy to legacy.
True. The method that can change use_legacy_token_service_ is 
StartUsingSharedAuthentication which is from legacy to non-legacy.

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
File components/signin/ios/browser/profile_oauth2_token_service_ios.mm (right):

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.mm:455:
DCHECK(!account_id.empty());
On 2014/04/10 15:04:08, Roger Tawa wrote:
> Add dcheck?
> 
>     DCHECK(!use_legacy_token_service_);

Done. I'll fix it in a future CL.

https://codereview.chromium.org/226643012/diff/60001/components/signin/ios/br...
components/signin/ios/browser/profile_oauth2_token_service_ios.mm:478:
DCHECK(!account_id.empty());
On 2014/04/10 15:04:08, Roger Tawa wrote:
> Add dcheck?
> 
>     DCHECK(!use_legacy_token_service_);

Done. I'll fix it in a future CL.

[blundell, 2014-04-10 15:17:58]

https://codereview.chromium.org/226643012/diff/60001/components/signin/core/b...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/226643012/diff/60001/components/signin/core/b...
components/signin/core/browser/signin_client.h:23: // core SigninClient.
Ideally the client wouldn't have anything platform-specific. However, the
situation around ProfileOAuth2TokenService is challenging due the tricky
inheritance issues, and I don't want to block the high-value work of upstreaming
PO2TSIOS on  having to figure out those issues. As Mihai says, it should become
more clear over time what the right situation is here (and maybe Alan's work
will help, actually).

On 2014/04/10 15:13:36, msarda wrote:
> On 2014/04/10 15:04:08, Roger Tawa wrote:
> > Is this a bug you plan to fix soon?  Can you put more info in the bug as to
> why
> > this is here now and why you want to eventually remove it :-)
> 
> No, this is low priority. It was something Colin wanted me to add in here, but
> this will become clearer if I need to remove once we do more componentization
> and understand what is the best strategy to use for mutli-platform components.

[commit-bot: I haz the power, 2014-04-10 16:39:21]

Change committed as 263008