Implement the gaia captcha state and unlock capability in the sync setup wiza...

Implement the gaia captcha state and unlock capability in the sync setup wizard.

BUG=19738
TEST=Get a Google Account into captcha state. Start syncing. You should
be presented with a captcha image and textbox to answer the challenge.
Doing so correctly shoud unlock your account and proceed with sync setup.


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

[tim (not reviewing), 2009-11-12 10:15:06]

I attached a screenshot to the bug, but note it is missing the captcha image. I
think I've been throttled or something by Gaia because I only get 'connection
unavailable' errors at the moment, so I had to manually step into the captcha
state in the debugger to generate that screenshot.  I will try again tomorrow.
Basically the big blue space is filled with typical white-backgrounded captcha
text.

Also, I can upload the locale_settings changes tomorrow.

[ncarter (slow), 2009-11-12 17:16:45]

Looks decent.

http://codereview.chromium.org/389017/diff/7001/7009
File chrome/browser/sync/engine/auth_watcher.cc (right):

http://codereview.chromium.org/389017/diff/7001/7009#newcode226
Line 226: captcha_token = gaia_->captcha_token();
As far as I can tell there are no callers that actually do specify the
captcha_token.  Would it be better to just remove it so that we don't require
the callers to awkwardly provide empty strings?

http://codereview.chromium.org/389017/diff/7001/7010
File chrome/browser/sync/engine/auth_watcher.h (right):

http://codereview.chromium.org/389017/diff/7001/7010#newcode178
Line 178: std::string email;
Please de-indent.

http://codereview.chromium.org/389017/diff/7001/7008
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/389017/diff/7001/7008#newcode525
Line 525: // |username|, |password|, and |captcha| are be owned by the caller.
"are be owned".

http://codereview.chromium.org/389017/diff/7001/7008#newcode527
Line 527: const char* captcha);
If you like, it's allowed to use std::string at this interface now.

http://codereview.chromium.org/389017/diff/7001/7006
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/389017/diff/7001/7006#newcode122
Line 122: const std::string& captcha);
This change breaks Linux -- see the trybot.

http://codereview.chromium.org/389017/diff/7001/7013
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/389017/diff/7001/7013#newcode168
Line 168: url(args.captchaUrl);;
;;

http://codereview.chromium.org/389017/diff/7001/7013#newcode240
Line 240: setElementDisplay("errormsg_0_Connection", 'none');
It doesn't make sense to hide the captcha here, does it?  I gather that you rely
on the fact that the captcha is an absorbing state, so once it's visible, it
stays visible.

http://codereview.chromium.org/389017/diff/7001/7013#newcode402
Line 402: value="" class='gaia le val' />
single quotes seems inconsistent.  Where are these "gaia le ... " classes
defined anyway?

http://codereview.chromium.org/389017/diff/7001/7013#newcode429
Line 429: <div id="throb" style="background-image:url(throbber.png);"
unbalanced quotes?!?

http://codereview.chromium.org/389017/diff/7001/7011
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/389017/diff/7001/7011#newcode75
Line 75: std::string username, password, captcha;
This tuple is plumbed through many method signatures.  Consider a struct.

http://codereview.chromium.org/389017/diff/7001/7016
File chrome/browser/sync/sync_setup_wizard_unittest.cc (right):

http://codereview.chromium.org/389017/diff/7001/7016#newcode24
Line 24: static const char* kTestCaptcha="pizzamyheart";
" = ", next line too.

[tim (not reviewing), 2009-11-12 19:34:14]

snapshot updated.

On 2009/11/12 17:16:45, nick wrote:
> Looks decent.
> 
> http://codereview.chromium.org/389017/diff/7001/7009
> File chrome/browser/sync/engine/auth_watcher.cc (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7009#newcode226
> Line 226: captcha_token = gaia_->captcha_token();
> As far as I can tell there are no callers that actually do specify the
> captcha_token.  Would it be better to just remove it so that we don't require
> the callers to awkwardly provide empty strings?
> 
> http://codereview.chromium.org/389017/diff/7001/7010
> File chrome/browser/sync/engine/auth_watcher.h (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7010#newcode178
> Line 178: std::string email;
> Please de-indent.
> 
> http://codereview.chromium.org/389017/diff/7001/7008
> File chrome/browser/sync/engine/syncapi.h (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7008#newcode525
> Line 525: // |username|, |password|, and |captcha| are be owned by the caller.
> "are be owned".
> 
> http://codereview.chromium.org/389017/diff/7001/7008#newcode527
> Line 527: const char* captcha);
> If you like, it's allowed to use std::string at this interface now.
> 
> http://codereview.chromium.org/389017/diff/7001/7006
> File chrome/browser/sync/profile_sync_service.h (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7006#newcode122
> Line 122: const std::string& captcha);
> This change breaks Linux -- see the trybot.
> 
> http://codereview.chromium.org/389017/diff/7001/7013
> File chrome/browser/sync/resources/gaia_login.html (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7013#newcode168
> Line 168: url(args.captchaUrl);;
> ;;
> 
> http://codereview.chromium.org/389017/diff/7001/7013#newcode240
> Line 240: setElementDisplay("errormsg_0_Connection", 'none');
> It doesn't make sense to hide the captcha here, does it?  I gather that you
rely
> on the fact that the captcha is an absorbing state, so once it's visible, it
> stays visible.
> 
> http://codereview.chromium.org/389017/diff/7001/7013#newcode402
> Line 402: value="" class='gaia le val' />
> single quotes seems inconsistent.  Where are these "gaia le ... " classes
> defined anyway?
> 
> http://codereview.chromium.org/389017/diff/7001/7013#newcode429
> Line 429: <div id="throb" style="background-image:url(throbber.png);"
> unbalanced quotes?!?
> 
> http://codereview.chromium.org/389017/diff/7001/7011
> File chrome/browser/sync/sync_setup_flow.cc (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7011#newcode75
> Line 75: std::string username, password, captcha;
> This tuple is plumbed through many method signatures.  Consider a struct.
> 
> http://codereview.chromium.org/389017/diff/7001/7016
> File chrome/browser/sync/sync_setup_wizard_unittest.cc (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7016#newcode24
> Line 24: static const char* kTestCaptcha="pizzamyheart";
> " = ", next line too.

[tim (not reviewing), 2009-11-12 19:34:47]

snapshot updated

http://codereview.chromium.org/389017/diff/7001/7009
File chrome/browser/sync/engine/auth_watcher.cc (right):

http://codereview.chromium.org/389017/diff/7001/7009#newcode226
Line 226: captcha_token = gaia_->captcha_token();
On 2009/11/12 17:16:45, nick wrote:
> As far as I can tell there are no callers that actually do specify the
> captcha_token.  Would it be better to just remove it so that we don't require
> the callers to awkwardly provide empty strings?

There aren't, but the token is sent (via GoogleServiceAuthError) when it comes
in and so it seemed appropriate to accept it back, since no other part of the
syncapi layer is stateful to the point of assuming the auth request is in
response to the last auth error (though maybe nothing else really cares).  The
right thing is likely to plumb the token back through, but then I felt that
would warrant a captcha specific API (so if you are just providing a password,
you don't need all that). This seemed like a good comprimise for now, the
last-minute-ness of this change kinda makes me want to keep API changes to a
minimum.

http://codereview.chromium.org/389017/diff/7001/7010
File chrome/browser/sync/engine/auth_watcher.h (right):

http://codereview.chromium.org/389017/diff/7001/7010#newcode178
Line 178: std::string email;
On 2009/11/12 17:16:45, nick wrote:
> Please de-indent.

Done.

http://codereview.chromium.org/389017/diff/7001/7008
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/389017/diff/7001/7008#newcode525
Line 525: // |username|, |password|, and |captcha| are be owned by the caller.
On 2009/11/12 17:16:45, nick wrote:
> "are be owned".

Done.
http://www.youtube.com/watch?v=qItugh-fFgg

http://codereview.chromium.org/389017/diff/7001/7008#newcode527
Line 527: const char* captcha);
On 2009/11/12 17:16:45, nick wrote:
> If you like, it's allowed to use std::string at this interface now.

I'll just stick to it for now.  Making these types of changes is a slippery
slope; I did some of that in part 1 of this 2 part captcha change in an effort
to keep this part purely focused on the goal :)

http://codereview.chromium.org/389017/diff/7001/7006
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/389017/diff/7001/7006#newcode122
Line 122: const std::string& captcha);
On 2009/11/12 17:16:45, nick wrote:
> This change breaks Linux -- see the trybot.

Thanks, trying again...

http://codereview.chromium.org/389017/diff/7001/7013
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/389017/diff/7001/7013#newcode168
Line 168: url(args.captchaUrl);;
On 2009/11/12 17:16:45, nick wrote:
> ;;

Done.

http://codereview.chromium.org/389017/diff/7001/7013#newcode240
Line 240: setElementDisplay("errormsg_0_Connection", 'none');
On 2009/11/12 17:16:45, nick wrote:
> It doesn't make sense to hide the captcha here, does it?  I gather that you
rely
> on the fact that the captcha is an absorbing state, so once it's visible, it
> stays visible.

You are correct.

http://codereview.chromium.org/389017/diff/7001/7013#newcode402
Line 402: value="" class='gaia le val' />
On 2009/11/12 17:16:45, nick wrote:
> single quotes seems inconsistent.  Where are these "gaia le ... " classes
> defined anyway?

Fixed quotes.  There are several 'style' blocks somehow, gaia le val is defined
in one of them. Again, this all came from a template, and I don't think we ever
cleaned it up. Please, I beg you, don't make me do it in this CL.. I spent way
to many countless hours fiddling with CSS in this file for this change already
:(

http://codereview.chromium.org/389017/diff/7001/7013#newcode429
Line 429: <div id="throb" style="background-image:url(throbber.png);"
On 2009/11/12 17:16:45, nick wrote:
> unbalanced quotes?!?

Nice catch, um, I don't know how that happened.

http://codereview.chromium.org/389017/diff/7001/7011
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/389017/diff/7001/7011#newcode75
Line 75: std::string username, password, captcha;
On 2009/11/12 17:16:45, nick wrote:
> This tuple is plumbed through many method signatures.  Consider a struct.

I like the suggestion, though I can't decide where to put a struct for those
auth params. there are several such 'auth request' structs already in syncapi,
it doesn't really belong in GoogleServiceAuthError because it makes sense to
just want to pass a username and password and not be typecasted as "error", I
don't want to put it in PSS because then SBH would have to include it. I could
add a new file, but that feels like overkill. any idea? Also Im considering (one
of my comment in auth watcher) having a captcha specific API in which case such
a struct could go in GoogleServiceAuthError (though there is already 'Captcha',
which would I guess become 'CaptchaChallenge' and 'CaptchaAnswer' or something.
I could add a todo, im always happy to make little tinkering CLs like that :)

http://codereview.chromium.org/389017/diff/7001/7016
File chrome/browser/sync/sync_setup_wizard_unittest.cc (right):

http://codereview.chromium.org/389017/diff/7001/7016#newcode24
Line 24: static const char* kTestCaptcha="pizzamyheart";
On 2009/11/12 17:16:45, nick wrote:
> " = ", next line too.

Done.

[ncarter (slow), 2009-11-12 21:07:17]

http://codereview.chromium.org/389017/diff/7001/7011
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/389017/diff/7001/7011#newcode75
Line 75: std::string username, password, captcha;
On 2009/11/12 19:34:48, timsteele wrote:
> On 2009/11/12 17:16:45, nick wrote:
> > This tuple is plumbed through many method signatures.  Consider a struct.
> 
> I like the suggestion, though I can't decide where to put a struct for those
> auth params. there are several such 'auth request' structs already in syncapi,
> it doesn't really belong in GoogleServiceAuthError because it makes sense to
> just want to pass a username and password and not be typecasted as "error", I
> don't want to put it in PSS because then SBH would have to include it. I could
> add a new file, but that feels like overkill. any idea? Also Im considering
(one
> of my comment in auth watcher) having a captcha specific API in which case
such
> a struct could go in GoogleServiceAuthError (though there is already
'Captcha',
> which would I guess become 'CaptchaChallenge' and 'CaptchaAnswer' or
something.
> I could add a todo, im always happy to make little tinkering CLs like that :)

Put it in:

sync/util/sync_types.h

Adding a captcha specific API seems like overkill.  I prefer the way you've done
it.

[ncarter (slow), 2009-11-12 21:14:31]

On 2009/11/12 21:07:17, nick wrote:
> http://codereview.chromium.org/389017/diff/7001/7011
> File chrome/browser/sync/sync_setup_flow.cc (right):
> 
> http://codereview.chromium.org/389017/diff/7001/7011#newcode75
> Line 75: std::string username, password, captcha;
> On 2009/11/12 19:34:48, timsteele wrote:
> > On 2009/11/12 17:16:45, nick wrote:
> > > This tuple is plumbed through many method signatures.  Consider a struct.
> > 
> > I like the suggestion, though I can't decide where to put a struct for those
> > auth params. there are several such 'auth request' structs already in
syncapi,
> > it doesn't really belong in GoogleServiceAuthError because it makes sense to
> > just want to pass a username and password and not be typecasted as "error",
I
> > don't want to put it in PSS because then SBH would have to include it. I
could
> > add a new file, but that feels like overkill. any idea? Also Im considering
> (one
> > of my comment in auth watcher) having a captcha specific API in which case
> such
> > a struct could go in GoogleServiceAuthError (though there is already
> 'Captcha',
> > which would I guess become 'CaptchaChallenge' and 'CaptchaAnswer' or
> something.
> > I could add a todo, im always happy to make little tinkering CLs like that
:)
> 
> Put it in:
> 
> sync/util/sync_types.h
> 
> Adding a captcha specific API seems like overkill.  I prefer the way you've
done
> it.

LGTM.  Please consider adding a struct for the
username/password/captcha_response tuple in a follow-up change.