Change the behavior of the [X] Save details to Wallet checkbox notification to:

Change the interaction between the

  [X] Save details in Google Wallet

checkbox notification and the account chooser (and rest of the dialog).  Previous problems were:

- the notification disappeared when the account chooser wasn't on Wallet
- the account chooser didn't change to "Pay without Wallet" when unchecked
- no way to tell if user is signed in when "Pay without Wallet" showing
- closing a dialog changed the state of an existing dialog (no longer first run)

BUG=224787
R=estade@chromium.org,aruslan@chromium.org,sky@chromium.org

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

[Dan Beam, 2013-04-05 02:45:57]

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_models.cc:153:
prefs_->SetBoolean(prefs::kAutofillDialogPayWithoutWallet,
should this model know about prefs?

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_models.cc:180: if
(prefs_->GetBoolean(prefs::kAutofillDialogPayWithoutWallet))
^ it already did here, but this could be changed

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/v...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:611:
checkbox->SetChecked(!controller_->GetPayWithoutWallet());
an alternative to this would be to use prefs directly in the view and/or pass
|controller_| to |notification_area_| so it can ask
|controller_->GetPayWithoutWallet()| directly during creation

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:612:
checkbox->set_listener(this);
^ an alternative to this would be to have the notification area itself be a
buttonlistener and pass in a reference to |controller_| (so it'd be able to say
controller_->SetPayWithoutWallet()) or just use prefs directly

[Dan Beam, 2013-04-05 02:50:08]

some for aruslan@, some for estade@

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left):

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:552: if
(wallet_items_.get()) {
this needed to work without |wallet_items_|

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:402:
GetWalletItems();
this matches the other calls we have (e.g. GetFullWallet())

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:705: }
^ this isn't strictly necessary to do in the controller, but I'm worried that
all the views will need to subscribe to pref updates themselves rather than
saying:

  void AutofillDialogControllerImpl::PrefChanged(const std::string& pref) {
    if (pref == prefs::kAutofillDialogPayWithoutWallet && view_) {
      view_->UpdateNotificationArea();
      view_->UpdateAccountChooser();
    }
  }

(which I can wire up / add if we want)

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1488: if
(account_chooser_model_.WalletIsSelected() && !wallet_items_)
^ we could probably re-fetch if we want...

[aruslan, 2013-04-05 03:45:35]

I like this much more.

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left):

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:552: if
(wallet_items_.get()) {
On 2013/04/05 02:50:08, Dan Beam wrote:
> this needed to work without |wallet_items_|

Yup, it's already like this in another CL.
In REQUIRES_RESPONSE case below please break instead of not reached.

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:402:
GetWalletItems();
On 2013/04/05 02:50:08, Dan Beam wrote:
> this matches the other calls we have (e.g. GetFullWallet())

Ack. Here and in GetWalletItems below the codereview shows weird indentation, I
guess it's just a glitch.

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:705: }
On 2013/04/05 02:50:08, Dan Beam wrote:
> ^ this isn't strictly necessary to do in the controller, but I'm worried that
> all the views will need to subscribe to pref updates themselves rather than
> saying:
> 
>   void AutofillDialogControllerImpl::PrefChanged(const std::string& pref) {
>     if (pref == prefs::kAutofillDialogPayWithoutWallet && view_) {
>       view_->UpdateNotificationArea();
>       view_->UpdateAccountChooser();
>     }
>   }
> 
> (which I can wire up / add if we want)

Currently, the account chooser is wired up to trigger AcoountChoiceChanged. I'm
happy to see this gone.

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1488: if
(account_chooser_model_.WalletIsSelected() && !wallet_items_)
On 2013/04/05 02:50:08, Dan Beam wrote:
> ^ we could probably re-fetch if we want...

Without the account chooser CL this doesn't matter (but I would refetch anyway).
In the account chooser CL it initiates refetch if we returned to the one that we
were using, otherwise it initiates the sign-in sequence.

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://chromiumcodereview.appspot.com/13625002/diff/1001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_models.cc:153:
prefs_->SetBoolean(prefs::kAutofillDialogPayWithoutWallet,
On 2013/04/05 02:45:57, Dan Beam wrote:
> should this model know about prefs?

I'd rather didn't pretend the account chooser knows more than the set of
accounts and the user selection. In this case all this wiring should go to the
controller and everyone is happy.

[Evan Stade, 2013-04-05 23:38:22]

https://codereview.chromium.org/13625002/diff/1001/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/13625002/diff/1001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:611:
checkbox->SetChecked(!controller_->GetPayWithoutWallet());
On 2013/04/05 02:45:57, Dan Beam wrote:
> an alternative to this would be to use prefs directly in the view and/or pass
> |controller_| to |notification_area_| so it can ask
> |controller_->GetPayWithoutWallet()| directly during creation

make initial checkmark state part of the notification

https://codereview.chromium.org/13625002/diff/1001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:612:
checkbox->set_listener(this);
On 2013/04/05 02:45:57, Dan Beam wrote:
> ^ an alternative to this would be to have the notification area itself be a
> buttonlistener and pass in a reference to |controller_| (so it'd be able to
say
> controller_->SetPayWithoutWallet()) or just use prefs directly

pass controller_ in and have the NotificationArea listen for changes. When
there's a change, call
controller_->CheckboxStateChanged(DialogNotification.type(), checked);

[Evan Stade, 2013-04-05 23:38:45]

also please add tests for the set of circumstances under which this banner shows

[Dan Beam, 2013-04-06 04:49:50]

will fix existing tests and create new ones on monday

https://codereview.chromium.org/13625002/diff/1001/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/13625002/diff/1001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:611:
checkbox->SetChecked(!controller_->GetPayWithoutWallet());
On 2013/04/05 23:38:23, Evan Stade wrote:
> On 2013/04/05 02:45:57, Dan Beam wrote:
> > an alternative to this would be to use prefs directly in the view and/or
pass
> > |controller_| to |notification_area_| so it can ask
> > |controller_->GetPayWithoutWallet()| directly during creation
> 
> make initial checkmark state part of the notification

Done.

https://codereview.chromium.org/13625002/diff/1001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:612:
checkbox->set_listener(this);
On 2013/04/05 23:38:23, Evan Stade wrote:
> On 2013/04/05 02:45:57, Dan Beam wrote:
> > ^ an alternative to this would be to have the notification area itself be a
> > buttonlistener and pass in a reference to |controller_| (so it'd be able to
> say
> > controller_->SetPayWithoutWallet()) or just use prefs directly
> 
> pass controller_ in and have the NotificationArea listen for changes. When
> there's a change, call
> controller_->CheckboxStateChanged(DialogNotification.type(), checked);

Done.

[Evan Stade, 2013-04-08 18:07:05]

https://codereview.chromium.org/13625002/diff/13001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://codereview.chromium.org/13625002/diff/13001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller.h:154: virtual void
CheckboxStateChanged(DialogNotification::Type type,
nit: can you call this NotificationCheckboxStateChanged?

also, this does not belong under the "Detail Inputs" group

[Dan Beam, 2013-04-09 01:08:07]

https://codereview.chromium.org/13625002/diff/13001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://codereview.chromium.org/13625002/diff/13001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller.h:154: virtual void
CheckboxStateChanged(DialogNotification::Type type,
On 2013/04/08 18:07:05, Evan Stade wrote:
> nit: can you call this NotificationCheckboxStateChanged?
> 
> also, this does not belong under the "Detail Inputs" group

Done.

[Dan Beam, 2013-04-09 02:10:57]

writing more tests now

[Dan Beam, 2013-04-09 03:35:51]

wrote tests, ptal

[Dan Beam, 2013-04-09 03:46:49]

adding sky@ to the mix.

to clarify reviewer responsibilities:

sky@: chrome/browser/ui/views
estade@: chrome/browser/ui/autofill
aruslan@: chrome/android, chrome/browser/ui/android

[aruslan, 2013-04-09 03:53:30]

chrome/android, chrome/browser/ui/android LGTM

[sky, 2013-04-09 14:09:40]

LGTM

[Dan Beam, 2013-04-09 21:11:45]

ping estade@

[Evan Stade, 2013-04-10 04:13:08]

lgtm

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

https://codereview.chromium.org/13625002/diff/47001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:707: 
could you add brief descriptions of what each test is doing?

[Dan Beam, 2013-04-10 04:51:59]

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

https://codereview.chromium.org/13625002/diff/47001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:707: 
On 2013/04/10 04:13:09, Evan Stade wrote:
> could you add brief descriptions of what each test is doing?

Done.

[commit-bot: I haz the power, 2013-04-10 04:52:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/13625002/64001

[commit-bot: I haz the power, 2013-04-10 08:52:30]

Change committed as 193348