Added OAuth support to TokenService

BUG=91661
TEST=unit


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95523

[Rick Campbell, 2011-08-04 13:21:40]

Please take a look

NOTE: While this is primarily about TokenService, it also adds some and removes
some from my recent GetOAuthFetcher update.  Specifically, OAuth support for
TokenService will *not* have a distinct service name, but instead use service
scope for service name.  However, the change to OnOAuthWrapBridgeSuccess stays
-- with the proviso that the name is the scope, and a similar change for
OnOAuthWrapBridgeFailure is added here.

[Mattias Nissler (ping if slow), 2011-08-04 13:39:57]

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
File chrome/browser/net/gaia/token_service.cc (right):

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.cc:58: getter_ =
profile->GetRequestContext();
It doesn't make much sense to keep around both the profile and the fetcher in
member variables, since you can always get the latter from the former.

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.cc:291: 
remove blank line

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
File chrome/browser/net/gaia/token_service.h (right):

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.h:198: Profile* profile_;
I kind of dislike this, since Profile is a super-heavy-weight dependency. As far
as I can see, we only use it to get a request context (for which we could just
pass a bare pointer in Initialize) and for the OAuth fetcher (which I believe
doesn't actually require the profile's cookie store for the operations
TokenService issues, but I may be mistaken on this one). So if we could drop the
Profile dependency, that would make TokenService much more reusable.

Please regard this comment as FYI/non-scoring feedback. I only wanted to mention
my concerns since for policy we made the experience that Profile dependency of
TokenService is a show stopper, which is why we're not using it for e.g. device
policy and instead have our own token fetching logic. On the other hand, you can
argue that TokenService should only be designed to support Profile-bound
functionality. Just mentioning for your consideration! :)

[Roger Tawa OOO till Jul 10th, 2011-08-04 14:02:14]

lgtm

For the dependency of TokenService on Profile, I see that the only use of
profile is to pass to GaiaOAuthFetcher.  In that class, it uses the profile for
two purposes:

- as the source for NOTIFICATION_COOKIE_CHANGED events
- to get a Browser* in order to display sign in html from the server

Not sure how easy it is to break the dependency of GaiaOAuthFetcher on Profile.

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
File chrome/browser/net/gaia/token_service.h (right):

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.h:222: // List of services that we're
performing operations for which are capable
remove "for" ?

[Mattias Nissler (ping if slow), 2011-08-04 14:24:35]

Again, this is FYI, non-scoring, and I'm fine with committing this.

On Thu, Aug 4, 2011 at 4:02 PM, <rogerta@chromium.org> wrote:

> lgtm
>
> For the dependency of TokenService on Profile, I see that the only use of
> profile is to pass to GaiaOAuthFetcher.  In that class, it uses the profile
> for
> two purposes:
>
> - as the source for NOTIFICATION_COOKIE_CHANGED events
>

We could just make the code use the TokenService pointer as the source.


> - to get a Browser* in order to display sign in html from the server
>

But do we need this functionality for TokenService? As far as I can see, it
only calls GaiaOAuthFetcher::StartOAuthWrapBridge(), which uses parameters
to authenticate, and doesn't require the Browser* pointer. Am I missing
something?


> Not sure how easy it is to break the dependency of GaiaOAuthFetcher on
> Profile.


Maybe just separate the UI interactions out into a delegate class?


>
>
>
>
> http://codereview.chromium.**org/7574009/diff/1/chrome/**
>
browser/net/gaia/token_**service.h<http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_service.h>
> File chrome/browser/net/gaia/token_**service.h (right):
>
> http://codereview.chromium.**org/7574009/diff/1/chrome/**
>
browser/net/gaia/token_**service.h#newcode222<http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_service.h#newcode222>
> chrome/browser/net/gaia/token_**service.h:222: // List of services that
> we're performing operations for which are capable
> remove "for" ?
>
>
>
http://codereview.chromium.**org/7574009/<http://codereview.chromium.org/7574...
>

[(NOT FOR CODE REVIEWS), 2011-08-04 15:33:43]

Hi Mattias,

Does it make sense for token service to be the source of
NOTIFICATION_COOKIE_CHANGED events?  Not sure.

A UI delegate might work here.  I noticed that GaiaOAuthFetcher lives
in browser\net\gaia whereas GaiaAuthFetcher live in common\net\gaia.
I assume this is because of the UI dependencies in GaiaOAuthFetcher?
If so, then it might be possible to move GaiaOAuthFetcher to
common\net\gaia and the delegate can live elsewhere.

Thanks,
Roger

-



On Thu, Aug 4, 2011 at 10:24, Mattias Nissler <mnissler@chromium.org> wrote:
> Again, this is FYI, non-scoring, and I'm fine with committing this.
>
> On Thu, Aug 4, 2011 at 4:02 PM, <rogerta@chromium.org> wrote:
>>
>> lgtm
>>
>> For the dependency of TokenService on Profile, I see that the only use of
>> profile is to pass to GaiaOAuthFetcher.  In that class, it uses the
>> profile for
>> two purposes:
>>
>> - as the source for NOTIFICATION_COOKIE_CHANGED events
>
> We could just make the code use the TokenService pointer as the source.
>
>>
>> - to get a Browser* in order to display sign in html from the server
>
> But do we need this functionality for TokenService? As far as I can see, it
> only calls GaiaOAuthFetcher::StartOAuthWrapBridge(), which uses parameters
> to authenticate, and doesn't require the Browser* pointer. Am I missing
> something?
>>
>> Not sure how easy it is to break the dependency of GaiaOAuthFetcher on
>> Profile.
>
> Maybe just separate the UI interactions out into a delegate class?
>
>>
>>
>>
>>
>>
http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
>> File chrome/browser/net/gaia/token_service.h (right):
>>
>>
>>
http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
>> chrome/browser/net/gaia/token_service.h:222: // List of services that
>> we're performing operations for which are capable
>> remove "for" ?
>>
>> http://codereview.chromium.org/7574009/
>
>

[Rick Campbell, 2011-08-04 16:12:58]

Possibly.  You're definitely correct about the motivation for putting it under
browser/net rather than common/net.  It might make sense to split things out as
you suggest, though my preference would be to consider this more thoroughly once
the core functionality is in place.  It feels like the sort of thing that can be
done after-the-fact rather than something likely to become hardened in place if
we don't move it immediately.

As for moving Profile out of the TokenService, I'm inclined the other way.  I'm
going to take a closer look at Mattias' suggestion and then follow-up here one
way or the other.  If it's as straight-forward as I'm hoping, I'll change that
now, otherwise I may end up landing this and filing a bug assigned to myself to
move Profile out later.

p. s.  Thank you both for jumping on this review so quickly!

[(NOT FOR CODE REVIEWS), 2011-08-04 16:52:26]

Hi Rick,

I agree the cleanup can be done a followup CL.

Thanks,
Roger

-



On Thu, Aug 4, 2011 at 12:12,  <rickcam@chromium.org> wrote:
> Possibly.  You're definitely correct about the motivation for putting it
> under
> browser/net rather than common/net.  It might make sense to split things out
> as
> you suggest, though my preference would be to consider this more thoroughly
> once
> the core functionality is in place.  It feels like the sort of thing that
> can be
> done after-the-fact rather than something likely to become hardened in place
> if
> we don't move it immediately.
>
> As for moving Profile out of the TokenService, I'm inclined the other way.
>  I'm
> going to take a closer look at Mattias' suggestion and then follow-up here
> one
> way or the other.  If it's as straight-forward as I'm hoping, I'll change
> that
> now, otherwise I may end up landing this and filing a bug assigned to myself
> to
> move Profile out later.
>
> p. s.  Thank you both for jumping on this review so quickly!
>
> http://codereview.chromium.org/7574009/
>

[Rick Campbell, 2011-08-04 17:24:42]

Since we seem to be agreed that further consideration of cleaning up the
Profile* can be deferred, I'll plan to land this shortly.  Thanks!

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
File chrome/browser/net/gaia/token_service.cc (right):

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.cc:58: getter_ =
profile->GetRequestContext();
On 2011/08/04 13:39:57, Mattias Nissler wrote:
> It doesn't make much sense to keep around both the profile and the fetcher in
> member variables, since you can always get the latter from the former.

I might be misunderstanding here.  The current dependency was introduced so that
the TokenService can use the profile when creating new GaiaOAuthFetchers, and
the fetchers need the profile in order to listen for cookie notifications.

Since fetcher's have a profile_ member, I could expose that and then use an
existing fetcher to obtain the profile when I need it (former from the latter?),
however, I'm more hesitant to explore inferring the active set of fetchers
through the Profile object (latter from the former?) since there might be
fetchers associated with the Profile that should be independent of the
TokenService.

As I mentioned in the header-file comment, my current thinking is to leave
things as they are for now, then revisit the fetcher first, and then reconsider
whether TokenService could also lose the profile data member.

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.cc:291: 
On 2011/08/04 13:39:57, Mattias Nissler wrote:
> remove blank line

Done.

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
File chrome/browser/net/gaia/token_service.h (right):

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.h:198: Profile* profile_;
On 2011/08/04 13:39:57, Mattias Nissler wrote:
> I kind of dislike this, since Profile is a super-heavy-weight dependency. As
far
> as I can see, we only use it to get a request context (for which we could just
> pass a bare pointer in Initialize) and for the OAuth fetcher (which I believe
> doesn't actually require the profile's cookie store for the operations
> TokenService issues, but I may be mistaken on this one). So if we could drop
the
> Profile dependency, that would make TokenService much more reusable.
> 
> Please regard this comment as FYI/non-scoring feedback. I only wanted to
mention
> my concerns since for policy we made the experience that Profile dependency of
> TokenService is a show stopper, which is why we're not using it for e.g.
device
> policy and instead have our own token fetching logic. On the other hand, you
can
> argue that TokenService should only be designed to support Profile-bound
> functionality. Just mentioning for your consideration! :)

My current thinking is to leave things as they are for now.  In revisiting
later, I would like to factor the Profile out of the GaiaOAuthFetcher first by
splitting the UI aspects from the more purely networking aspects, and then
reconsider removing it from the TokenService at that time.

http://codereview.chromium.org/7574009/diff/1/chrome/browser/net/gaia/token_s...
chrome/browser/net/gaia/token_service.h:222: // List of services that we're
performing operations for which are capable
On 2011/08/04 14:02:14, Roger Tawa wrote:
> remove "for" ?

Thanks.  I went for "List of services that are capable of ClientLogin-based
authentication." and similar wording in the OAuth case a bit further down.

[commit-bot: I haz the power, 2011-08-04 19:37:12]

Try job failure for 7574009-19 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-08-04 22:29:35]

Change committed as 95523