Handle failed signin when setting kAutofillDialogPayWithoutWallet pref.

Handle failed signin when setting kAutofillDialogPayWithoutWallet pref.

BUG=244902
R=dbeam@chromium.org

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

[Dan Beam, 2013-05-31 00:21:54]

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left):

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2697:
manually_selected_pay_without_wallet);
don't we use this to determine if it's first run?

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1803: 
why not just detect the change here?

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2700: }
and ensure this pref is set to its current value here?

[Evan Stade, 2013-05-31 02:47:33]

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2700: }
On 2013/05/31 00:21:55, Dan Beam wrote:
> and ensure this pref is set to its current value here?

it seems better to stop making this pref pull double duty, i.e. use this code as
written in this CL but create a new pref for the number of runs of the dialog.
I'd just as soon leave IsFirstRun as is for now and I'll start a discussion for
determining the correct behavior of the various promo-ish banners.

[Evan Stade, 2013-05-31 03:12:28]

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15871004/diff/4001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2700: }
On 2013/05/31 02:47:33, Evan Stade wrote:
> On 2013/05/31 00:21:55, Dan Beam wrote:
> > and ensure this pref is set to its current value here?
> 
> it seems better to stop making this pref pull double duty, i.e. use this code
as
> written in this CL but create a new pref for the number of runs of the dialog.
> I'd just as soon leave IsFirstRun as is for now and I'll start a discussion
for
> determining the correct behavior of the various promo-ish banners.

filed https://code.google.com/p/chromium/issues/detail?id=245542 for this.

[Dan Beam, 2013-05-31 22:22:52]

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:693: if
(account_chooser_model_.active_wallet_account_name().empty()) {
nit: and here (!account_chooser_model_.HasAccountsToChoose())

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2696:
!account_chooser_model_.active_wallet_account_name().empty()) {
nit: account_chooser_model_.HasAccountsToChoose()

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:1665:
controller()->OnAccept();
^ oh yeah, the other reason I didn't review this CL is that your tests were
dchecking

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:1709:
controller()->OnAccept();
^ reverse the order of these to fix your test

[Dan Beam, 2013-05-31 22:39:13]

https://codereview.chromium.org/15871004/diff/26001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/15871004/diff/26001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:1701:
controller()->OnDidGetFullWallet(wallet::GetTestFullWallet());
just do this for now, i'll fix the order of GetWalletItems() vs
OnUserNameFetch() later (maybe)

[Evan Stade, 2013-05-31 22:41:10]

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:693: if
(account_chooser_model_.active_wallet_account_name().empty()) {
On 2013/05/31 22:22:53, Dan Beam wrote:
> nit: and here (!account_chooser_model_.HasAccountsToChoose())

The current code is logical and easy to make sense of. Changing the code to what
you suggest requires the reader to have knowledge of the implementation of
HasAccountsToChoose.

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2696:
!account_chooser_model_.active_wallet_account_name().empty()) {
On 2013/05/31 22:22:53, Dan Beam wrote:
> nit: account_chooser_model_.HasAccountsToChoose()

Done.

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:1665:
controller()->OnAccept();
On 2013/05/31 22:22:53, Dan Beam wrote:
> ^ oh yeah, the other reason I didn't review this CL is that your tests were
> dchecking

it would be nice to have mentioned that, but given the (predictable) triviality
of the fix, I don't think that needs to block you from reviewing.

https://codereview.chromium.org/15871004/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:1709:
controller()->OnAccept();
On 2013/05/31 22:22:53, Dan Beam wrote:
> ^ reverse the order of these to fix your test

that did the trick.

[Dan Beam, 2013-05-31 22:42:43]

lgtm

[commit-bot: I haz the power, 2013-05-31 23:07:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/15871004/34001

[commit-bot: I haz the power, 2013-06-01 04:18:33]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Evan Stade, 2013-06-03 19:28:16]

Committed patchset #8 manually as r203748 (presubmit successful).