Allow gaia auth fetcher to work with arbitrary clients.

Allow gaia auth fetcher to work with arbitrary clients.

BUG=581793
TEST=Extended GaiaAuthFetcherTest passes
TEST=Tested together https://codereview.chromium.org/1618193003/.
     Auth code is returned for GMS Core client id and is consumed
     at Android.

Committed: https://crrev.com/2a5da822dd88c4eb3c538c134fd4b81f1207b8fb
Cr-Commit-Position: refs/heads/master@{#371989}

[khmel, 2016-01-27 19:48:35]

Description was changed from

==========
Allow gaia auth fetcher to work with arbitrary clients.

BUG=581793
TEST=Extended GaiaAuthFetcherTest passes
TEST=Tested together https://codereview.chromium.org/1618193003/.
     Auth code is returned for GMS Core client id and is consumed
     at Android.
==========

to

==========
Allow gaia auth fetcher to work with arbitrary clients.

BUG=581793
TEST=Extended GaiaAuthFetcherTest passes
TEST=Tested together https://codereview.chromium.org/1618193003/.
     Auth code is returned for GMS Core client id and is consumed
     at Android.
==========

[khmel, 2016-01-27 19:48:35]

khmel@chromium.org changed reviewers:
+ xiyuan@chromium.org, zelidrag@chromium.org

[khmel, 2016-01-27 19:49:17]

As discussed https://codereview.chromium.org/1618193003, the easiest way to get
auth code for GMS Core client is to extend gaia auth fetcher.

PTAL

[khmel, 2016-01-27 22:10:41]

Fixed unsafe usage found during real testing.

PTAL

[xiyuan, 2016-01-27 22:51:30]

Mostly good. Thank you for doing this.

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher.cc (right):

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:187: fetch_token_from_auth_code_(false) {
nit: use the non-static class member initializers and also update
fetch_pending_.

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:537: void
GaiaAuthFetcher::StartCookieForOAuthLoginTokenExchangeWithClientIdAndDeviceId(
nit: The name is insane. Can we call it StartCookieForOAuthLoginTokenExchange
and rely on function overloading?

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:759:
consumer_->OnClientOAuthCode(auth_code);
I wonder whether we should call this. The auth code would be used to exchange
for RT in this case. We probably should not pass it out and let consumer to mess
with it.

[khmel, 2016-01-27 23:16:13]

Thanks Xiyuan for your fast review. I've updated code.

PTAL

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher.cc (right):

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:187: fetch_token_from_auth_code_(false) {
On 2016/01/27 22:51:29, xiyuan wrote:
> nit: use the non-static class member initializers and also update
> fetch_pending_.

Done.

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:537: void
GaiaAuthFetcher::StartCookieForOAuthLoginTokenExchangeWithClientIdAndDeviceId(
On 2016/01/27 22:51:30, xiyuan wrote:
> nit: The name is insane. Can we call it StartCookieForOAuthLoginTokenExchange
> and rely on function overloading?

Agree :)
But I followed style used here. BTW, I tried to rename function above too
(StartCookieForOAuthLoginTokenExchangeWithDeviceId). But it is used in legacy
code. And if we rename StartCookieForOAuthLoginTokenExchangeWithDeviceId we need
to rename several function here ending with DeviceId as well which will cause a
massive change. I think renaming of newly added function is fine.

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:759:
consumer_->OnClientOAuthCode(auth_code);
On 2016/01/27 22:51:30, xiyuan wrote:
> I wonder whether we should call this. The auth code would be used to exchange
> for RT in this case. We probably should not pass it out and let consumer to
mess
> with it.

Yes, this would be better.

[xiyuan, 2016-01-27 23:19:31]

LGTM

But you still need an owner's stamp, either from zel or rogerta.

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher.cc (right):

https://codereview.chromium.org/1644713002/diff/40001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:537: void
GaiaAuthFetcher::StartCookieForOAuthLoginTokenExchangeWithClientIdAndDeviceId(
On 2016/01/27 23:16:13, khmel wrote:
> On 2016/01/27 22:51:30, xiyuan wrote:
> > nit: The name is insane. Can we call it
StartCookieForOAuthLoginTokenExchange
> > and rely on function overloading?
> 
> Agree :)
> But I followed style used here. BTW, I tried to rename function above too
> (StartCookieForOAuthLoginTokenExchangeWithDeviceId). But it is used in legacy
> code. And if we rename StartCookieForOAuthLoginTokenExchangeWithDeviceId we
need
> to rename several function here ending with DeviceId as well which will cause
a
> massive change. I think renaming of newly added function is fine.

Understand. I would try to follow existing convention as well. But this name is
inhumane. :p

I am okay to leave StartCookieForOAuthLoginTokenExchangeWithDeviceId for another
day.

[khmel, 2016-01-27 23:24:28]

khmel@chromium.org changed reviewers:
+ rogerta@chromium.org

[khmel, 2016-01-27 23:26:50]

Hi Zel and Roger,

This CL was kindly reviewed by Xiyuan. As a OWNER, could you please review?

PTAL

[zel, 2016-01-28 01:57:19]

lgtm

[khmel, 2016-01-28 02:08:30]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2016-01-28 02:11:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644713002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644713002/60001

[commit-bot: I haz the power, 2016-01-28 03:23:38]

Description was changed from

==========
Allow gaia auth fetcher to work with arbitrary clients.

BUG=581793
TEST=Extended GaiaAuthFetcherTest passes
TEST=Tested together https://codereview.chromium.org/1618193003/.
     Auth code is returned for GMS Core client id and is consumed
     at Android.
==========

to

==========
Allow gaia auth fetcher to work with arbitrary clients.

BUG=581793
TEST=Extended GaiaAuthFetcherTest passes
TEST=Tested together https://codereview.chromium.org/1618193003/.
     Auth code is returned for GMS Core client id and is consumed
     at Android.
==========

[commit-bot: I haz the power, 2016-01-28 03:23:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-28 03:24:26]

Description was changed from

==========
Allow gaia auth fetcher to work with arbitrary clients.

BUG=581793
TEST=Extended GaiaAuthFetcherTest passes
TEST=Tested together https://codereview.chromium.org/1618193003/.
     Auth code is returned for GMS Core client id and is consumed
     at Android.
==========

to

==========
Allow gaia auth fetcher to work with arbitrary clients.

BUG=581793
TEST=Extended GaiaAuthFetcherTest passes
TEST=Tested together https://codereview.chromium.org/1618193003/.
     Auth code is returned for GMS Core client id and is consumed
     at Android.

Committed: https://crrev.com/2a5da822dd88c4eb3c538c134fd4b81f1207b8fb
Cr-Commit-Position: refs/heads/master@{#371989}
==========

[commit-bot: I haz the power, 2016-01-28 03:24:27]

Patchset 4 (id:??) landed as
https://crrev.com/2a5da822dd88c4eb3c538c134fd4b81f1207b8fb
Cr-Commit-Position: refs/heads/master@{#371989}