Respect the new Online Wallet sign-in response.

Respect the new Online Wallet sign-in response.

In addition to handling non-200 response codes, we now
parse the response to see if the sign-in was successful.

See the http://crbug.com/244463#c12 for more details.

BUG=244463
TEST=WalletSigninHelperTest

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

[aruslan, 2013-06-13 22:10:32]

PTAL wrt "YES" and not "YES".

[ahutter, 2013-06-13 23:52:10]

Some (optional) nits but otherwise lgtm

https://codereview.chromium.org/16858016/diff/1/components/autofill/content/b...
File components/autofill/content/browser/wallet/wallet_signin_helper.cc (right):

https://codereview.chromium.org/16858016/diff/1/components/autofill/content/b...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:318:
GoogleServiceAuthError* error) {
Should you DCHECK(error)?

https://codereview.chromium.org/16858016/diff/1/components/autofill/content/b...
File components/autofill/content/browser/wallet/wallet_signin_helper.h (right):

https://codereview.chromium.org/16858016/diff/1/components/autofill/content/b...
components/autofill/content/browser/wallet/wallet_signin_helper.h:108: // and
initiates fetching the user name on success or calls the delegate
|url_fetcher_|

https://codereview.chromium.org/16858016/diff/1/components/autofill/content/b...
components/autofill/content/browser/wallet/wallet_signin_helper.h:112: //
Processes the response to the automatic sign-in (in url_fetcher_),
ditto

https://codereview.chromium.org/16858016/diff/1/components/autofill/content/b...
File components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc
(right):

https://codereview.chromium.org/16858016/diff/1/components/autofill/content/b...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:170:
200,
Any reason you aren't using net::HTTP_OK?

[aruslan, 2013-06-18 21:21:46]

Thanks, Alex!
dbeam@ -- PTAL and apply the OWNERS seal if appropriate.

https://chromiumcodereview.appspot.com/16858016/diff/1/components/autofill/co...
File components/autofill/content/browser/wallet/wallet_signin_helper.cc (right):

https://chromiumcodereview.appspot.com/16858016/diff/1/components/autofill/co...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:318:
GoogleServiceAuthError* error) {
On 2013/06/13 23:52:10, ahutter wrote:
> Should you DCHECK(error)?

Done.

https://chromiumcodereview.appspot.com/16858016/diff/1/components/autofill/co...
File components/autofill/content/browser/wallet/wallet_signin_helper.h (right):

https://chromiumcodereview.appspot.com/16858016/diff/1/components/autofill/co...
components/autofill/content/browser/wallet/wallet_signin_helper.h:108: // and
initiates fetching the user name on success or calls the delegate
On 2013/06/13 23:52:10, ahutter wrote:
> |url_fetcher_|

Done; I removed both methods.

https://chromiumcodereview.appspot.com/16858016/diff/1/components/autofill/co...
components/autofill/content/browser/wallet/wallet_signin_helper.h:112: //
Processes the response to the automatic sign-in (in url_fetcher_),
On 2013/06/13 23:52:10, ahutter wrote:
> ditto

Done; I removed both methods.

https://chromiumcodereview.appspot.com/16858016/diff/1/components/autofill/co...
File components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc
(right):

https://chromiumcodereview.appspot.com/16858016/diff/1/components/autofill/co...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:170:
200,
On 2013/06/13 23:52:10, ahutter wrote:
> Any reason you aren't using net::HTTP_OK?

Done.

[Dan Beam, 2013-06-18 21:33:50]

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
File components/autofill/content/browser/wallet/wallet_signin_helper.cc (right):

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:11: #include
"base/strings/stringprintf.h"
#include "base/strings/string_util.h"

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:277:
DCHECK(url_fetcher_);
nit: to make local closer and reduce number of times you check url_fetcher_ in
debug mode

if (!url_fetcher_) {
  NOTREACHED();
  return;
}

std::string data;
if (!url_fetcher_->GetResponseAsString(&data)) {
  OnOtherError();
  return false;
}

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:281:
LOG(ERROR) << "failed to GetResponseAsString";
remove or changed to DLOG(ERROR) or something

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:286: if (data
!= "YES") {
if (!LowerCaseEqualsASCII(data, "yes")) {

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
File components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc
(right):

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:117:
GaiaUrls::GetInstance()->client_login_url(), net::HTTP_OK,
thanks, this looks prettier

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:169:
void MockFailedPassiveAuthUrlMergeAndRedirectResponseNo() {
METHOD NAME: Y U NO SHORTER?!

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:173:
"NOOOO");
nit: "nooooooooooooooo" (as in http://nooooooooooooooo.com)

[aruslan, 2013-06-18 23:05:25]

Thanks! PTAL at the fixes.

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
File components/autofill/content/browser/wallet/wallet_signin_helper.cc (right):

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:11: #include
"base/strings/stringprintf.h"
On 2013/06/18 21:33:50, Dan Beam wrote:
> #include "base/strings/string_util.h"

base::StringSprintf isn't in string_util.h :)

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:277:
DCHECK(url_fetcher_);
On 2013/06/18 21:33:50, Dan Beam wrote:
> nit: to make local closer and reduce number of times you check url_fetcher_ in
> debug mode
> 
> if (!url_fetcher_) {
>   NOTREACHED();
>   return;
> }
> 
> std::string data;
> if (!url_fetcher_->GetResponseAsString(&data)) {
>   OnOtherError();
>   return false;
> }

Done.

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:281:
LOG(ERROR) << "failed to GetResponseAsString";
On 2013/06/18 21:33:50, Dan Beam wrote:
> remove or changed to DLOG(ERROR) or something

Done.

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:286: if (data
!= "YES") {
On 2013/06/18 21:33:50, Dan Beam wrote:
> if (!LowerCaseEqualsASCII(data, "yes")) {

Done.

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
File components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc
(right):

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:117:
GaiaUrls::GetInstance()->client_login_url(), net::HTTP_OK,
On 2013/06/18 21:33:50, Dan Beam wrote:
> thanks, this looks prettier

Done.

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:169:
void MockFailedPassiveAuthUrlMergeAndRedirectResponseNo() {
On 2013/06/18 21:33:50, Dan Beam wrote:
> METHOD NAME: Y U NO SHORTER?!

Done.

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc:173:
"NOOOO");
On 2013/06/18 21:33:50, Dan Beam wrote:
> nit: "nooooooooooooooo" (as in http://nooooooooooooooo.com)

Done.

[Dan Beam, 2013-06-19 06:53:58]

lgtm

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
File components/autofill/content/browser/wallet/wallet_signin_helper.cc (right):

https://chromiumcodereview.appspot.com/16858016/diff/7001/components/autofill...
components/autofill/content/browser/wallet/wallet_signin_helper.cc:11: #include
"base/strings/stringprintf.h"
On 2013/06/18 23:05:25, aruslan wrote:
> On 2013/06/18 21:33:50, Dan Beam wrote:
> > #include "base/strings/string_util.h"
> 
> base::StringSprintf isn't in string_util.h :)

i meant for LowerCaseEqualsASCII()

[commit-bot: I haz the power, 2013-06-19 09:16:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/16858016/13001

[commit-bot: I haz the power, 2013-06-20 02:48:54]

Change committed as 207284