Multi-account AccountChooser for interactive autocomplete.

Multi-account AccountChooser for interactive autocomplete.

BUG=223923, 226855, 230227
TBR=sky@chromium.org
NOTRY=True

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

[aruslan, 2013-03-29 19:25:24]

Hi Evan, please take a look.
Should I move the AccountChooserModel to a separate .h/.cc/_unittest?

[Evan Stade, 2013-03-29 23:08:44]

yes, I think AccountChooserModel deserves its own file now. Please add code that
exercises all your new functions (not just unit tests) because otherwise it's
hard for me to grasp why they are necessary. I don't really like the "caller is
responsible for updating the view" paradigm, but I can't tell if there's a
better way without seeing the code written.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:612: // there is
no menu, just a sign in link.
comment is hard to follow. "Autofill data" is not an account.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:628:
account_chooser_model_.checked_item()),
isn't the checked item already an index? aren't index and command id synonymous
for the account chooser?

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models.cc:101: const int
AccountChooserModel::kContentAreaWalletItemId = 0;
I do not understand this variable name or its purpose.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models.cc:127: if (checked_item_ ==
kContentAreaWalletItemId) return;
please put return on a new line.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models.h:127: // which should be
considered as the selected account change.
this comment is hard to understand.

[aruslan, 2013-03-30 00:07:47]

Thanks, Evan, please take a look.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:612: // there is
no menu, just a sign in link.
On 2013/03/29 23:08:45, Evan Stade wrote:
> comment is hard to follow. "Autofill data" is not an account.

Done.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:628:
account_chooser_model_.checked_item()),
On 2013/03/29 23:08:45, Evan Stade wrote:
> isn't the checked item already an index? aren't index and command id
synonymous
> for the account chooser?

Not anymore; do you suggest it should be?
Command-ids are simpler as they allow to express the fact that the user attempts
to sign into the Wallet, but we don't know what this account is; or that the
only option the user has is to pay without the Wallet.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models.cc:101: const int
AccountChooserModel::kContentAreaWalletItemId = 0;
On 2013/03/29 23:08:45, Evan Stade wrote:
> I do not understand this variable name or its purpose.

Done.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models.cc:127: if (checked_item_ ==
kContentAreaWalletItemId) return;
On 2013/03/29 23:08:45, Evan Stade wrote:
> please put return on a new line.

Done.

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://codereview.chromium.org/13331007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models.h:127: // which should be
considered as the selected account change.
On 2013/03/29 23:08:45, Evan Stade wrote:
> this comment is hard to understand. 

Done.

[aruslan, 2013-03-30 00:43:23]

Extracted AccountChooserModel to its file and tried to come up with better
comments and names.

> Please add code that exercises all your new functions
> (not just unit tests) because otherwise it's hard for
> me to grasp why they are necessary. 

All new functions except SetAvailableAccounts() are used in
autofill_dialog_controller_impl.
SetAvailableAccounts() usage is will be in a separate CL with the Android
support once we are past this one.

> I don't really like the "caller is responsible for updating
> the view" paradigm, but I can't tell if there's a better way
> without seeing the code written.

Neither am I; it's a question of "do we want the view to get updated a number of
times in a row" or not.  Better ideas are welcome!

[aruslan, 2013-04-01 14:47:50]

On 2013/03/29 23:08:44, Evan Stade wrote:
> yes, I think AccountChooserModel deserves its own file now. Please add code
that

Hi Evan, please take a look :)

[Evan Stade, 2013-04-01 23:54:34]

I think some better terminology could improve the clarity of this class.
Multiple accounts can be currently signed in, so it's confusing to refer to the
one that you're actually using for wallet as the (one and only) signed-in
account.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.cc:132: if
(prefs_->GetBoolean(prefs::kAutofillDialogPayWithoutWallet))
nit: prefer ternary

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.cc:147:
AddCheckItem(kSignedInWalletItemId, ASCIIToUTF16(current_username_));
you want UTF8ToUTF16.

Also, this conversion should not even need to happen. Why doesn't
WalletSigninHelper deal in string16? if there's some valid reason it needs to be
utf8, then so be it, but current_username_ and available_accounts_ should be
string16 not std::string.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.h (right):

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:50: // This function calls
AccountChoiceChanged on the delegate.
nit: don't think this last line is necessary. It's an implementation detail and
likely to become stale.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:54: // signed-in account is
known.
I don't know what you mean by "known" or what "available" means really. What
does it mean for an account to be unknown? unavailable? Could you be more
explicit?

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:58: // The set doesn't have
to include the currently signed-in account.
so you can have an account that's signed in but unavailable? this is confusing.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:60: // It is the
responsibility of the caller to update the view accordingly.
why does this one force the caller to know that the account choice will have
changed instead of just calling that function (or a different/new function on
AccountChooserModelDelegate)

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:63: // Should be called if
the signed-in account in the content area is known.
I don't know what "content area" means here. Are you talking about the tab
contents? I'd call that the cookie jar, except that I don't think you should be
documenting when something should be called, rather what it does. This applies
to many of the comments here.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:65: // It is the
responsibility of the caller to update the view accordingly.
ditto

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:69: // Any Wallet error
automatically resets the currently signed-in account.
resets it to what?

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:71: // It is the
responsibility of the caller to update the view accordingly.
ditto

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:81: // The
AccountChoiceChanged() will be called on the delegate.
ditto

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:134: // May be different from
the currently selected account (e.g. autofill data).
refering to autofill data as an account is a source of confusion for me.

[aruslan, 2013-04-10 17:40:31]

I'll for dbeam@ changes to go through and then I'll change this accordingly.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.cc:132: if
(prefs_->GetBoolean(prefs::kAutofillDialogPayWithoutWallet))
On 2013/04/01 23:54:34, Evan Stade wrote:
> nit: prefer ternary

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.h (right):

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:50: // This function calls
AccountChoiceChanged on the delegate.
On 2013/04/01 23:54:34, Evan Stade wrote:
> nit: don't think this last line is necessary. It's an implementation detail
and
> likely to become stale.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:54: // signed-in account is
known.
On 2013/04/01 23:54:34, Evan Stade wrote:
> I don't know what you mean by "known" or what "available" means really. What
> does it mean for an account to be unknown? unavailable? Could you be more
> explicit?

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:58: // The set doesn't have
to include the currently signed-in account.
On 2013/04/01 23:54:34, Evan Stade wrote:
> so you can have an account that's signed in but unavailable? this is
confusing.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:60: // It is the
responsibility of the caller to update the view accordingly.
On 2013/04/01 23:54:34, Evan Stade wrote:
> why does this one force the caller to know that the account choice will have
> changed instead of just calling that function (or a different/new function on
> AccountChooserModelDelegate)

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:63: // Should be called if
the signed-in account in the content area is known.
On 2013/04/01 23:54:34, Evan Stade wrote:
> I don't know what "content area" means here. Are you talking about the tab
> contents? I'd call that the cookie jar, except that I don't think you should
be
> documenting when something should be called, rather what it does. This applies
> to many of the comments here.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:65: // It is the
responsibility of the caller to update the view accordingly.
On 2013/04/01 23:54:34, Evan Stade wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:69: // Any Wallet error
automatically resets the currently signed-in account.
On 2013/04/01 23:54:34, Evan Stade wrote:
> resets it to what?

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:71: // It is the
responsibility of the caller to update the view accordingly.
On 2013/04/01 23:54:34, Evan Stade wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:81: // The
AccountChoiceChanged() will be called on the delegate.
On 2013/04/01 23:54:34, Evan Stade wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:81: // The
AccountChoiceChanged() will be called on the delegate.
On 2013/04/01 23:54:34, Evan Stade wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/13331007/diff/14001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:134: // May be different from
the currently selected account (e.g. autofill data).
On 2013/04/01 23:54:34, Evan Stade wrote:
> refering to autofill data as an account is a source of confusion for me.

Done.

[aruslan, 2013-04-11 18:31:58]

Hi Evan, I renamed the functions, fixed comments, added (hopefully) more clear
terminology section, switched to use string16 etc.

Please take a look.

[Evan Stade, 2013-04-11 19:27:45]

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.cc:48:
!(current_username_.empty() && additional_accounts_.empty());
are you sure we want to show the sign in link when there's been a wallet error?
I thought wallet errors were not recoverable in our UI.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.h (right):

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:59: void
ForceSelectWalletAccount();
why ForceSelect...? Why not just Select...?

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:66: void
SetAdditionalAccounts(const std::vector<string16>& accounts);
it seems this is only exercised by tests. Could you remove code that is only
exercised by tests, and only add it (plus the tests) when you have code that
uses it? Otherwise it is hard to review the interface.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:74: void
SetActiveWalletAccountName(const string16& account);
So is it the "active wallet account name" or the "current username"?

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:81: string16
GetActiveWalletAccountName() const;
should return const string16&. Should be called current_username() and defined
inline.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:108: static const int
kActiveWalletItemId;  // The active Wallet account.
you seem to double-document each of these

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:110: static const int
kFirstAdditionalItemId;  // First id for additional accounts.
you shouldn't need to expose these publicly.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:116:
AccountChooserModelDelegate* account_delegate_;
nit: just |delegate_|

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:127: // info.
nit: remove final sentence

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1653: //
TODO(estade): fill in the email address.
you should be able to delete this TODO

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1658:
suggested_email_.AddKeyedItem(base::IntToString(i), emails[i]);
this doesn't make that much sense to me, why suggest other logged in emails?
That seems to increase the chance of something unintended happening, e.g. I use
my personal account to buy something and use my work email as a contact email. I
think that one of two options make sense:

1) just add the currently in-use email
2) (1) and also add emails from Autofill

[aruslan, 2013-04-12 00:46:01]

Thanks, Evan, I moved all stuff that is currently not used in this CL to the
subsequent one.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.cc:48:
!(current_username_.empty() && additional_accounts_.empty());
On 2013/04/11 19:27:45, Evan Stade wrote:
> are you sure we want to show the sign in link when there's been a wallet
error?
> I thought wallet errors were not recoverable in our UI.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.h (right):

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:59: void
ForceSelectWalletAccount();
On 2013/04/11 19:27:45, Evan Stade wrote:
> why ForceSelect...? Why not just Select...?

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:66: void
SetAdditionalAccounts(const std::vector<string16>& accounts);
On 2013/04/11 19:27:45, Evan Stade wrote:
> it seems this is only exercised by tests. Could you remove code that is only
> exercised by tests, and only add it (plus the tests) when you have code that
> uses it? Otherwise it is hard to review the interface.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:74: void
SetActiveWalletAccountName(const string16& account);
On 2013/04/11 19:27:45, Evan Stade wrote:
> So is it the "active wallet account name" or the "current username"?

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:81: string16
GetActiveWalletAccountName() const;
On 2013/04/11 19:27:45, Evan Stade wrote:
> should return const string16&. Should be called current_username() and defined
> inline.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:108: static const int
kActiveWalletItemId;  // The active Wallet account.
On 2013/04/11 19:27:45, Evan Stade wrote:
> you seem to double-document each of these

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:110: static const int
kFirstAdditionalItemId;  // First id for additional accounts.
On 2013/04/11 19:27:45, Evan Stade wrote:
> you shouldn't need to expose these publicly.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:116:
AccountChooserModelDelegate* account_delegate_;
On 2013/04/11 19:27:45, Evan Stade wrote:
> nit: just |delegate_|

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:127: // info.
On 2013/04/11 19:27:45, Evan Stade wrote:
> nit: remove final sentence

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1653: //
TODO(estade): fill in the email address.
On 2013/04/11 19:27:45, Evan Stade wrote:
> you should be able to delete this TODO

Done.

https://chromiumcodereview.appspot.com/13331007/diff/40001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1658:
suggested_email_.AddKeyedItem(base::IntToString(i), emails[i]);
On 2013/04/11 19:27:45, Evan Stade wrote:
> this doesn't make that much sense to me, why suggest other logged in emails?
> That seems to increase the chance of something unintended happening, e.g. I
use
> my personal account to buy something and use my work email as a contact email.
I
> think that one of two options make sense:
> 
> 1) just add the currently in-use email
> 2) (1) and also add emails from Autofill

Done; I added your comment to http://crbug.com/223757.

[aruslan, 2013-04-12 01:33:02]

Hi Evan, the subsequent CL stuff is removed, all comments are addressed; please
take a look, thanks!

(The subsequent CL is at https://codereview.chromium.org/14213002; due to file
renames it currently includes all the changes from this CL.)

[Evan Stade, 2013-04-12 17:06:36]

lgtm with a few more nits

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.cc:124: // A throbber should be
shown until the Wallet account name is set.
is there a bug filed specifically for this?

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.h (right):

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:36: // Online Wallet service.
 There may be multiple signed-in accounts, but at any
be consistent about the number of spaces after a period.

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:58: // Sets the selection to
the local autofill data.
capitalize Autofill.

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:59: void
SelectAutofillData();
how about SelectUseAutofill?

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:681:
account_chooser_model_.HasAccountsToChoose())
curlies

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1338: // to the
Wallet.  This will trigger AccountChoiceChanged.
one space after period (to be consistent with this file)

[aruslan, 2013-04-12 20:43:30]

Thanks, Evan!

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.cc:124: // A throbber should be
shown until the Wallet account name is set.
On 2013/04/12 17:06:36, Evan Stade wrote:
> is there a bug filed specifically for this?

Done.

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
File chrome/browser/ui/autofill/account_chooser_model.h (right):

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:36: // Online Wallet service.
 There may be multiple signed-in accounts, but at any
On 2013/04/12 17:06:36, Evan Stade wrote:
> be consistent about the number of spaces after a period.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:58: // Sets the selection to
the local autofill data.
On 2013/04/12 17:06:36, Evan Stade wrote:
> capitalize Autofill.

Done.

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/account_chooser_model.h:59: void
SelectAutofillData();
On 2013/04/12 17:06:36, Evan Stade wrote:
> how about SelectUseAutofill?

Done.

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:681:
account_chooser_model_.HasAccountsToChoose())
On 2013/04/12 17:06:36, Evan Stade wrote:
> curlies

Done.

https://chromiumcodereview.appspot.com/13331007/diff/61001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1338: // to the
Wallet.  This will trigger AccountChoiceChanged.
On 2013/04/12 17:06:36, Evan Stade wrote:
> one space after period (to be consistent with this file)

Done.

[commit-bot: I haz the power, 2013-04-12 21:32:18]

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

[commit-bot: I haz the power, 2013-04-12 21:32:24]

Failed to apply patch for chrome/browser/ui/autofill/autofill_dialog_models.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_dialog_models.cc
  Hunk #1 FAILED at 96.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_models.cc.rej

Patch:       chrome/browser/ui/autofill/autofill_dialog_models.cc
Index: chrome/browser/ui/autofill/autofill_dialog_models.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_models.cc
b/chrome/browser/ui/autofill/autofill_dialog_models.cc
index
ed50352ece95f161686a3c5cc87c2a1a9014a8a6..b5419dec9ea3c8984710d827f8b1e18002f7412c
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_models.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_models.cc
@@ -96,71 +96,6 @@ void SuggestionsMenuModel::ExecuteCommand(int command_id, int
event_flags) {
   delegate_->SuggestionItemSelected(*this);
 }
 
-// AccountChooserModel
---------------------------------------------------------
-
-const int AccountChooserModel::kWalletItemId = 0;
-const int AccountChooserModel::kAutofillItemId = 1;
-
-AccountChooserModelDelegate::~AccountChooserModelDelegate() {}
-
-AccountChooserModel::AccountChooserModel(
-    AccountChooserModelDelegate* delegate,
-    PrefService* prefs)
-    : ALLOW_THIS_IN_INITIALIZER_LIST(ui::SimpleMenuModel(this)),
-      account_delegate_(delegate),
-      checked_item_(prefs->GetBoolean(prefs::kAutofillDialogPayWithoutWallet) ?
-          kAutofillItemId : kWalletItemId),
-      had_wallet_error_(false) {
-  AddCheckItem(kWalletItemId,
-               l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_GOOGLE_WALLET));
-  SetIcon(
-      kWalletItemId,
-      ui::ResourceBundle::GetSharedInstance().GetImageNamed(IDR_WALLET_ICON));
-  AddCheckItemWithStringId(kAutofillItemId,
-                           IDS_AUTOFILL_DIALOG_PAY_WITHOUT_WALLET);
-}
-
-AccountChooserModel::~AccountChooserModel() {
-}
-
-bool AccountChooserModel::IsCommandIdChecked(int command_id) const {
-  return command_id == checked_item_;
-}
-
-bool AccountChooserModel::IsCommandIdEnabled(int command_id) const {
-  if (command_id == kWalletItemId && had_wallet_error_)
-    return false;
-
-  return true;
-}
-
-bool AccountChooserModel::GetAcceleratorForCommandId(
-    int command_id,
-    ui::Accelerator* accelerator) {
-  return false;
-}
-
-void AccountChooserModel::ExecuteCommand(int command_id, int event_flags) {
-  if (checked_item_ == command_id)
-    return;
-
-  checked_item_ = command_id;
-  account_delegate_->AccountChoiceChanged();
-}
-
-void AccountChooserModel::SetHadWalletError() {
-  had_wallet_error_ = true;
-  ExecuteCommand(kAutofillItemId, 0);
-}
-
-void AccountChooserModel::SetHadWalletSigninError() {
-  ExecuteCommand(kAutofillItemId, 0);
-}
-
-bool AccountChooserModel::WalletIsSelected() const {
-  return checked_item_ == kWalletItemId;
-}
-
 // MonthComboboxModel
----------------------------------------------------------
 
 MonthComboboxModel::MonthComboboxModel() {}

[commit-bot: I haz the power, 2013-04-13 03:23:30]

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

[commit-bot: I haz the power, 2013-04-18 00:19:31]

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

[commit-bot: I haz the power, 2013-04-18 00:19:35]

Failed to apply patch for
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
  Hunk #8 FAILED at 810.
  Hunk #18 FAILED at 1910.
  Hunk #19 succeeded at 2211 (offset 1 line).
  2 out of 19 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc.rej

Patch:       chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index
c548cbc9e9fe2558f0ec6496502c0e9c68022cf8..522c3c288638dbe837588df448056706fb9707dd
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -192,7 +192,7 @@ void GetBillingInfoFromOutputs(const DetailOutputMap&
output,
   }
 }
 
-// Returns the containing window for the given |web_contents|.  The containing
+// Returns the containing window for the given |web_contents|. The containing
 // window might be a browser window for a Chrome tab, or it might be a shell
 // window for a platform app.
 BaseWindow* GetBaseWindowForWebContents(
@@ -466,11 +466,12 @@ bool AutofillDialogControllerImpl::ShouldShowSpinner()
const {
 }
 
 string16 AutofillDialogControllerImpl::AccountChooserText() const {
+  // TODO(aruslan): this should be l10n "Not using Google Wallet".
   if (!account_chooser_model_.WalletIsSelected())
     return l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PAY_WITHOUT_WALLET);
 
   if (SignedInState() == SIGNED_IN)
-    return UTF8ToUTF16(current_username_);
+    return account_chooser_model_.active_wallet_account_name();
 
   // In this case, the account chooser should be showing the signin link.
   return string16();
@@ -535,11 +536,14 @@ void AutofillDialogControllerImpl::GetWalletItems() {
   GetWalletClient()->GetWalletItems(source_url_);
 }
 
-void AutofillDialogControllerImpl::OnWalletOrSigninUpdate() {
+void AutofillDialogControllerImpl::SignedInStateUpdated() {
+  if (!account_chooser_model_.WalletIsSelected())
+    return;
+
   switch (SignedInState()) {
     case SIGNED_IN:
       // Start fetching the user name if we don't know it yet.
-      if (current_username_.empty()) {
+      if (account_chooser_model_.active_wallet_account_name().empty()) {
         signin_helper_.reset(new wallet::WalletSigninHelper(
             this, profile_->GetRequestContext()));
         signin_helper_->StartUserNameFetch();
@@ -547,24 +551,29 @@ void
AutofillDialogControllerImpl::OnWalletOrSigninUpdate() {
       break;
 
     case REQUIRES_SIGN_IN:
-      // TODO(aruslan): automatic sign-in?
+      // Switch to the local account and refresh the dialog.
+      OnWalletSigninError();
       break;
 
     case REQUIRES_PASSIVE_SIGN_IN:
       // Attempt to passively sign in the user.
-      current_username_.clear();
+      DCHECK(!signin_helper_);
+      account_chooser_model_.ClearActiveWalletAccountName();
       signin_helper_.reset(new wallet::WalletSigninHelper(
-          this, profile_->GetRequestContext()));
+          this,
+          profile_->GetRequestContext()));
       signin_helper_->StartPassiveSignin();
       break;
 
     case REQUIRES_RESPONSE:
       break;
   }
+}
 
+void AutofillDialogControllerImpl::OnWalletOrSigninUpdate() {
+  SignedInStateUpdated();
   SuggestionsUpdated();
-  view_->UpdateAccountChooser();
-  view_->UpdateNotificationArea();
+  UpdateAccountChooserView();
 
   // On the first successful response, compute the initial user state metric.
   if (initial_user_state_ == AutofillMetrics::DIALOG_USER_STATE_UNKNOWN)
@@ -573,7 +582,6 @@ void AutofillDialogControllerImpl::OnWalletOrSigninUpdate()
{
 
 void AutofillDialogControllerImpl::OnWalletSigninError() {
   signin_helper_.reset();
-  current_username_.clear();
   account_chooser_model_.SetHadWalletSigninError();
   GetWalletClient()->CancelRequests();
 }
@@ -669,12 +677,15 @@ ui::MenuModel*
AutofillDialogControllerImpl::MenuModelForSection(
 }
 
 ui::MenuModel* AutofillDialogControllerImpl::MenuModelForAccountChooser() {
-  // When paying with wallet, but not signed in, there is no menu, just a
-  // sign in link.
-  if (account_chooser_model_.WalletIsSelected() && SignedInState() !=
SIGNED_IN)
-    return NULL;
+  // If there were unrecoverable Wallet errors, or if there are choices other
+  // than "Pay without the wallet", show the full menu.
+  if (account_chooser_model_.had_wallet_error() ||
+      account_chooser_model_.HasAccountsToChoose()) {
+    return &account_chooser_model_;
+  }
 
-  return &account_chooser_model_;
+  // Otherwise, there is no menu, just a sign in link.
+  return NULL;
 }
 
 gfx::Image AutofillDialogControllerImpl::AccountChooserImage() {
@@ -684,8 +695,10 @@ gfx::Image
AutofillDialogControllerImpl::AccountChooserImage() {
   }
 
   gfx::Image icon;
-  account_chooser_model_.GetIconAt(account_chooser_model_.checked_item(),
-                                   &icon);
+  account_chooser_model_.GetIconAt(
+      account_chooser_model_.GetIndexOfCommandId(
+          account_chooser_model_.checked_item()),
+      &icon);
   return icon;
 }
 
@@ -797,9 +810,14 @@ scoped_ptr<DataModelWrapper>
AutofillDialogControllerImpl::CreateWrapper(
       return scoped_ptr<DataModelWrapper>(
           new WalletInstrumentWrapper(wallet_items_->instruments()[index]));
     }
+
+    if (section == SECTION_SHIPPING) {
+      return scoped_ptr<DataModelWrapper>(
+          new WalletAddressWrapper(wallet_items_->addresses()[index]));
+    }
+
     // TODO(dbeam): should SECTION_EMAIL get here?
-    return scoped_ptr<DataModelWrapper>(
-        new WalletAddressWrapper(wallet_items_->addresses()[index]));
+    return scoped_ptr<DataModelWrapper>();
   }
 
   if (section == SECTION_CC) {
@@ -1166,6 +1184,7 @@ std::vector<DialogNotification>
 }
 
 void AutofillDialogControllerImpl::StartSignInFlow() {
+  DCHECK(!IsPayingWithWallet());
   DCHECK(registrar_.IsEmpty());
 
   content::Source<content::NavigationController> source(view_->ShowSignIn());
@@ -1184,9 +1203,10 @@ void AutofillDialogControllerImpl::EndSignInFlow() {
 void AutofillDialogControllerImpl::NotificationCheckboxStateChanged(
     DialogNotification::Type type, bool checked) {
   if (type == DialogNotification::WALLET_USAGE_CONFIRMATION) {
-    int command = checked ? AccountChooserModel::kWalletItemId :
-                            AccountChooserModel::kAutofillItemId;
-    account_chooser_model_.ExecuteCommand(command, 0);
+    if (checked)
+      account_chooser_model_.SelectActiveWalletAccount();
+    else
+      account_chooser_model_.SelectUseAutofill();
   }
 }
 
@@ -1321,8 +1341,14 @@ void AutofillDialogControllerImpl::Observe(
       content::Details<content::LoadCommittedDetails>(details).ptr();
   if (wallet::IsSignInContinueUrl(load_details->entry->GetVirtualURL())) {
     EndSignInFlow();
-    if (account_chooser_model_.WalletIsSelected())
+
+    if (account_chooser_model_.WalletIsSelected()) {
       GetWalletItems();
+    } else {
+      // The sign-in flow means that the user implicitly switched the account
+      // to the Wallet. This will trigger AccountChoiceChanged.
+      account_chooser_model_.SelectActiveWalletAccount();
+    }
   }
 }
 
@@ -1386,28 +1412,29 @@ void AutofillDialogControllerImpl::OnDidGetFullWallet(
 
 void AutofillDialogControllerImpl::OnPassiveSigninSuccess(
     const std::string& username) {
-  current_username_ = username;
+  const string16 username16 = UTF8ToUTF16(username);
   signin_helper_.reset();
+  account_chooser_model_.SetActiveWalletAccountName(username16);
   GetWalletItems();
 }
 
 void AutofillDialogControllerImpl::OnUserNameFetchSuccess(
     const std::string& username) {
-  current_username_ = username;
+  const string16 username16 = UTF8ToUTF16(username);
   signin_helper_.reset();
+  account_chooser_model_.SetActiveWalletAccountName(username16);
   OnWalletOrSigninUpdate();
 }
 
 void AutofillDialogControllerImpl::OnAutomaticSigninSuccess(
     const std::string& username) {
-  // TODO(aruslan): automatic sign-in.
   NOTIMPLEMENTED();
 }
 
 void AutofillDialogControllerImpl::OnPassiveSigninFailure(
     const GoogleServiceAuthError& error) {
   // TODO(aruslan): report an error.
-  LOG(ERROR) << "failed to passively sign-in: " << error.ToString();
+  LOG(ERROR) << "failed to passively sign in: " << error.ToString();
   OnWalletSigninError();
 }
 
@@ -1420,14 +1447,14 @@ void
AutofillDialogControllerImpl::OnUserNameFetchFailure(
 
 void AutofillDialogControllerImpl::OnAutomaticSigninFailure(
     const GoogleServiceAuthError& error) {
-  // TODO(aruslan): automatic sign-in failure.
-  NOTIMPLEMENTED();
+  // TODO(aruslan): report an error.
+  LOG(ERROR) << "failed to automatically sign in: " << error.ToString();
+  OnWalletSigninError();
 }
 
 void AutofillDialogControllerImpl::OnDidGetWalletItems(
     scoped_ptr<wallet::WalletItems> wallet_items) {
   DCHECK(account_chooser_model_.WalletIsSelected());
-
   legal_documents_text_.clear();
   legal_document_link_ranges_.clear();
 
@@ -1504,6 +1531,9 @@ void AutofillDialogControllerImpl::OnPersonalDataChanged()
{
   SuggestionsUpdated();
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// AccountChooserModelDelegate implementation.
+
 void AutofillDialogControllerImpl::AccountChoiceChanged() {
   // Whenever the user changes the account, all manual inputs should be reset.
   ResetManualInputForSection(SECTION_EMAIL);
@@ -1517,11 +1547,28 @@ void
AutofillDialogControllerImpl::AccountChoiceChanged() {
 
   SetIsSubmitting(false);
 
-  if (account_chooser_model_.WalletIsSelected() && !wallet_items_)
-    GetWalletItems();
+  if (!signin_helper_ && account_chooser_model_.Wa…
(message too large)

[commit-bot: I haz the power, 2013-04-18 00:26:17]

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

[commit-bot: I haz the power, 2013-04-18 00:26:20]

Failed to apply patch for
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
  Hunk #8 FAILED at 810.
  Hunk #13 FAILED at 1442.
  Hunk #14 FAILED at 1461.
  Hunk #15 FAILED at 1471.
  Hunk #16 FAILED at 1482.
  Hunk #17 FAILED at 1493.
  Hunk #18 FAILED at 1502.
  Hunk #22 FAILED at 1673.
  Hunk #23 FAILED at 1689.
  Hunk #24 FAILED at 1906.
  Hunk #25 FAILED at 2006.
  Hunk #26 FAILED at 2021.
  Hunk #27 succeeded at 2207 (offset 1 line).
  12 out of 27 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc.rej

Patch:       chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index
c548cbc9e9fe2558f0ec6496502c0e9c68022cf8..27e912884adcccddcab5f681ca04e8f3ae8df4b4
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -192,7 +192,7 @@ void GetBillingInfoFromOutputs(const DetailOutputMap&
output,
   }
 }
 
-// Returns the containing window for the given |web_contents|.  The containing
+// Returns the containing window for the given |web_contents|. The containing
 // window might be a browser window for a Chrome tab, or it might be a shell
 // window for a platform app.
 BaseWindow* GetBaseWindowForWebContents(
@@ -466,11 +466,12 @@ bool AutofillDialogControllerImpl::ShouldShowSpinner()
const {
 }
 
 string16 AutofillDialogControllerImpl::AccountChooserText() const {
+  // TODO(aruslan): this should be l10n "Not using Google Wallet".
   if (!account_chooser_model_.WalletIsSelected())
     return l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PAY_WITHOUT_WALLET);
 
   if (SignedInState() == SIGNED_IN)
-    return UTF8ToUTF16(current_username_);
+    return account_chooser_model_.active_wallet_account_name();
 
   // In this case, the account chooser should be showing the signin link.
   return string16();
@@ -535,11 +536,14 @@ void AutofillDialogControllerImpl::GetWalletItems() {
   GetWalletClient()->GetWalletItems(source_url_);
 }
 
-void AutofillDialogControllerImpl::OnWalletOrSigninUpdate() {
+void AutofillDialogControllerImpl::SignedInStateUpdated() {
+  if (!account_chooser_model_.WalletIsSelected())
+    return;
+
   switch (SignedInState()) {
     case SIGNED_IN:
       // Start fetching the user name if we don't know it yet.
-      if (current_username_.empty()) {
+      if (account_chooser_model_.active_wallet_account_name().empty()) {
         signin_helper_.reset(new wallet::WalletSigninHelper(
             this, profile_->GetRequestContext()));
         signin_helper_->StartUserNameFetch();
@@ -547,24 +551,29 @@ void
AutofillDialogControllerImpl::OnWalletOrSigninUpdate() {
       break;
 
     case REQUIRES_SIGN_IN:
-      // TODO(aruslan): automatic sign-in?
+      // Switch to the local account and refresh the dialog.
+      OnWalletSigninError();
       break;
 
     case REQUIRES_PASSIVE_SIGN_IN:
       // Attempt to passively sign in the user.
-      current_username_.clear();
+      DCHECK(!signin_helper_);
+      account_chooser_model_.ClearActiveWalletAccountName();
       signin_helper_.reset(new wallet::WalletSigninHelper(
-          this, profile_->GetRequestContext()));
+          this,
+          profile_->GetRequestContext()));
       signin_helper_->StartPassiveSignin();
       break;
 
     case REQUIRES_RESPONSE:
       break;
   }
+}
 
+void AutofillDialogControllerImpl::OnWalletOrSigninUpdate() {
+  SignedInStateUpdated();
   SuggestionsUpdated();
-  view_->UpdateAccountChooser();
-  view_->UpdateNotificationArea();
+  UpdateAccountChooserView();
 
   // On the first successful response, compute the initial user state metric.
   if (initial_user_state_ == AutofillMetrics::DIALOG_USER_STATE_UNKNOWN)
@@ -573,7 +582,6 @@ void AutofillDialogControllerImpl::OnWalletOrSigninUpdate()
{
 
 void AutofillDialogControllerImpl::OnWalletSigninError() {
   signin_helper_.reset();
-  current_username_.clear();
   account_chooser_model_.SetHadWalletSigninError();
   GetWalletClient()->CancelRequests();
 }
@@ -669,12 +677,15 @@ ui::MenuModel*
AutofillDialogControllerImpl::MenuModelForSection(
 }
 
 ui::MenuModel* AutofillDialogControllerImpl::MenuModelForAccountChooser() {
-  // When paying with wallet, but not signed in, there is no menu, just a
-  // sign in link.
-  if (account_chooser_model_.WalletIsSelected() && SignedInState() !=
SIGNED_IN)
-    return NULL;
+  // If there were unrecoverable Wallet errors, or if there are choices other
+  // than "Pay without the wallet", show the full menu.
+  if (account_chooser_model_.had_wallet_error() ||
+      account_chooser_model_.HasAccountsToChoose()) {
+    return &account_chooser_model_;
+  }
 
-  return &account_chooser_model_;
+  // Otherwise, there is no menu, just a sign in link.
+  return NULL;
 }
 
 gfx::Image AutofillDialogControllerImpl::AccountChooserImage() {
@@ -684,8 +695,10 @@ gfx::Image
AutofillDialogControllerImpl::AccountChooserImage() {
   }
 
   gfx::Image icon;
-  account_chooser_model_.GetIconAt(account_chooser_model_.checked_item(),
-                                   &icon);
+  account_chooser_model_.GetIconAt(
+      account_chooser_model_.GetIndexOfCommandId(
+          account_chooser_model_.checked_item()),
+      &icon);
   return icon;
 }
 
@@ -797,9 +810,14 @@ scoped_ptr<DataModelWrapper>
AutofillDialogControllerImpl::CreateWrapper(
       return scoped_ptr<DataModelWrapper>(
           new WalletInstrumentWrapper(wallet_items_->instruments()[index]));
     }
-    // TODO(dbeam): should SECTION_EMAIL get here?
-    return scoped_ptr<DataModelWrapper>(
-        new WalletAddressWrapper(wallet_items_->addresses()[index]));
+
+    if (section == SECTION_SHIPPING) {
+      return scoped_ptr<DataModelWrapper>(
+          new WalletAddressWrapper(wallet_items_->addresses()[index]));
+    }
+
+    // TODO(dbeam): should SECTION_EMAIL get here? http://crbug.com/223923
+    return scoped_ptr<DataModelWrapper>();
   }
 
   if (section == SECTION_CC) {
@@ -1166,6 +1184,7 @@ std::vector<DialogNotification>
 }
 
 void AutofillDialogControllerImpl::StartSignInFlow() {
+  DCHECK(!IsPayingWithWallet());
   DCHECK(registrar_.IsEmpty());
 
   content::Source<content::NavigationController> source(view_->ShowSignIn());
@@ -1184,9 +1203,10 @@ void AutofillDialogControllerImpl::EndSignInFlow() {
 void AutofillDialogControllerImpl::NotificationCheckboxStateChanged(
     DialogNotification::Type type, bool checked) {
   if (type == DialogNotification::WALLET_USAGE_CONFIRMATION) {
-    int command = checked ? AccountChooserModel::kWalletItemId :
-                            AccountChooserModel::kAutofillItemId;
-    account_chooser_model_.ExecuteCommand(command, 0);
+    if (checked)
+      account_chooser_model_.SelectActiveWalletAccount();
+    else
+      account_chooser_model_.SelectUseAutofill();
   }
 }
 
@@ -1321,8 +1341,14 @@ void AutofillDialogControllerImpl::Observe(
       content::Details<content::LoadCommittedDetails>(details).ptr();
   if (wallet::IsSignInContinueUrl(load_details->entry->GetVirtualURL())) {
     EndSignInFlow();
-    if (account_chooser_model_.WalletIsSelected())
+
+    if (account_chooser_model_.WalletIsSelected()) {
       GetWalletItems();
+    } else {
+      // The sign-in flow means that the user implicitly switched the account
+      // to the Wallet. This will trigger AccountChoiceChanged.
+      account_chooser_model_.SelectActiveWalletAccount();
+    }
   }
 }
 
@@ -1386,28 +1412,29 @@ void AutofillDialogControllerImpl::OnDidGetFullWallet(
 
 void AutofillDialogControllerImpl::OnPassiveSigninSuccess(
     const std::string& username) {
-  current_username_ = username;
+  const string16 username16 = UTF8ToUTF16(username);
   signin_helper_.reset();
+  account_chooser_model_.SetActiveWalletAccountName(username16);
   GetWalletItems();
 }
 
 void AutofillDialogControllerImpl::OnUserNameFetchSuccess(
     const std::string& username) {
-  current_username_ = username;
+  const string16 username16 = UTF8ToUTF16(username);
   signin_helper_.reset();
+  account_chooser_model_.SetActiveWalletAccountName(username16);
   OnWalletOrSigninUpdate();
 }
 
 void AutofillDialogControllerImpl::OnAutomaticSigninSuccess(
     const std::string& username) {
-  // TODO(aruslan): automatic sign-in.
   NOTIMPLEMENTED();
 }
 
 void AutofillDialogControllerImpl::OnPassiveSigninFailure(
     const GoogleServiceAuthError& error) {
   // TODO(aruslan): report an error.
-  LOG(ERROR) << "failed to passively sign-in: " << error.ToString();
+  LOG(ERROR) << "failed to passively sign in: " << error.ToString();
   OnWalletSigninError();
 }
 
@@ -1420,18 +1447,18 @@ void
AutofillDialogControllerImpl::OnUserNameFetchFailure(
 
 void AutofillDialogControllerImpl::OnAutomaticSigninFailure(
     const GoogleServiceAuthError& error) {
-  // TODO(aruslan): automatic sign-in failure.
-  NOTIMPLEMENTED();
+  // TODO(aruslan): report an error.
+  LOG(ERROR) << "failed to automatically sign in: " << error.ToString();
+  OnWalletSigninError();
 }
 
 void AutofillDialogControllerImpl::OnDidGetWalletItems(
     scoped_ptr<wallet::WalletItems> wallet_items) {
   DCHECK(account_chooser_model_.WalletIsSelected());
-
   legal_documents_text_.clear();
   legal_document_link_ranges_.clear();
 
-  // TODO(dbeam): verify all items support kCartCurrency?
+  // TODO(dbeam): verify items support kCartCurrency? http://crbug.com/232952
   wallet_items_ = wallet_items.Pass();
   OnWalletOrSigninUpdate();
 }
@@ -1439,7 +1466,7 @@ void AutofillDialogControllerImpl::OnDidGetWalletItems(
 void AutofillDialogControllerImpl::OnDidSaveAddress(
…
(message too large)

[commit-bot: I haz the power, 2013-04-18 13:44:33]

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

[commit-bot: I haz the power, 2013-04-18 15:27:24]

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

[commit-bot: I haz the power, 2013-04-18 15:27:46]

Change committed as 194928