Changes to the GAIA auth fetching to support one-click login.

Changes to the GAIA auth fetching to support one-click login.

BUG=110050
TEST=no user visible functionality, see new unit tests.

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

[Andrew T Wilson (Slow), 2012-02-27 18:00:41]

A few nits. BTW, I don't really know the GAIA fetcher APIs, so maybe timsteele
should also take a peek?

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_consumer.h (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_consumer.h:16: typedef std::vector<std::string>
ResponseCookies;
Why is this in |net| but nothing else in this file is? Is there a downside to
having this outside net (or just no typedef?)

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.cc:855: DCHECK(url ==
source->GetURL());
Why do only some of these if clauses have this DCHECK?

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.h:269: int load_flags,
Can we document |load_flags|?

[Roger Tawa OOO till Jul 10th, 2012-02-29 18:46:53]

Thanks Drew.  Comments addressed, changes uploaded. I did a gclient sync as I
addressed your comments, so there are some changes in there that are not mine,
sorry about that.

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_consumer.h (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_consumer.h:16: typedef std::vector<std::string>
ResponseCookies;
On 2012/02/27 18:00:41, Andrew T Wilson wrote:
> Why is this in |net| but nothing else in this file is? Is there a downside to
> having this outside net (or just no typedef?)

This is essentially a forward declare.  I moved the forward declare to here from
gaia_auth_fetcher.h

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.cc:855: DCHECK(url ==
source->GetURL());
On 2012/02/27 18:00:41, Andrew T Wilson wrote:
> Why do only some of these if clauses have this DCHECK?

These are cases before this change that did not look at the original URL as
well.  Note the token_auth_gurl_ cases did not look at the original URL but
should have, and did not cause problems before because it was not being used. 
Now it is.

I added these DCHECKs to make sure I did not do anything wrong while working on
this code, but I can remove them if you prefer.

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.h:269: int load_flags,
On 2012/02/27 18:00:41, Andrew T Wilson wrote:
> Can we document |load_flags|?

Done.

[Andrew T Wilson (Slow), 2012-02-29 21:57:17]

LGTM.

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_consumer.h (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_consumer.h:16: typedef std::vector<std::string>
ResponseCookies;
Yeah, I guess I'm just thinking that nothing breaks if we remove the net::
namespace here or just directly pass around std::vector<string>. But I don't
feel strongly about it so leave it if you like.

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.cc:864: DCHECK(url ==
source->GetURL());
Yeah, I'd either add the DCHECK() to every if clause, or remove them. Not having
it everywhere implies that somehow the invariant *doesn't* hold in those other
if clauses.

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.h:263: // Create a fetcher useable for
making any Gaia request.  |body| is used
nit: useable->usable (yeah, it was an existing typo, I know :)

[Roger Tawa OOO till Jul 10th, 2012-03-01 17:55:47]

Thanks Drew.  Comments addressed, changes uploaded.  Will commit after trybot
runs.

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_consumer.h (right):

http://codereview.chromium.org/9465018/diff/5002/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_consumer.h:16: typedef std::vector<std::string>
ResponseCookies;
On 2012/02/29 21:57:17, Andrew T Wilson wrote:
> Yeah, I guess I'm just thinking that nothing breaks if we remove the net::
> namespace here or just directly pass around std::vector<string>. But I don't
> feel strongly about it so leave it if you like.

I investigated simply declaring a new string vector here, but there is code that
depends on this actually being net::ResponseCookies, see OnTokenAuthFetched(). 
So instead of having two string vector types in this code, I figured I would
just stick to one.

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.cc:864: DCHECK(url ==
source->GetURL());
On 2012/02/29 21:57:17, Andrew T Wilson wrote:
> Yeah, I'd either add the DCHECK() to every if clause, or remove them. Not
having
> it everywhere implies that somehow the invariant *doesn't* hold in those other
> if clauses.

Removed from all, since for sure it does not hold on some of them.

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/9465018/diff/9001/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.h:263: // Create a fetcher useable for
making any Gaia request.  |body| is used
On 2012/02/29 21:57:17, Andrew T Wilson wrote:
> nit: useable->usable (yeah, it was an existing typo, I know :)

Done.

[commit-bot: I haz the power, 2012-03-01 19:54:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/9465018/15004

[commit-bot: I haz the power, 2012-03-01 21:48:35]

Change committed as 124490