Implementing VERIFY_CVV required action.

Implementing VERIFY_CVV required action.

BUG=168673
TEST=unit tests and manual testing
NOTRY=true

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

[Dan Beam, 2013-03-26 01:46:25]

estade@: chrome/browser/ui/autofill
isherman@: components/autofill/browser

[Dan Beam, 2013-03-26 01:46:57]

note: there are test, forgot to update description

[Dan Beam, 2013-03-26 01:47:05]

tests**

[ahutter, 2013-03-26 15:27:09]

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/app/generat...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/app/generat...
chrome/app/generated_resources.grd:10743: +      <message
name="IDS_AUTOFILL_DIALOG_DETAILS_FROM_WALLET" desc="Text explaining where the
dialog got their payment info from (on first run of this dialog while using
Google Wallet).">
super nit: got its

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

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1062: if
(dialog_type_ == DIALOG_TYPE_AUTOCHECKOUT) {
I think this block with the UpdateButtonStrip needs to be in FinishSubmit or
things won't work correctly. I think the buttons will get out of whack with the
new addition of CVV stuff.

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1070: // Update
the button strip after autocheckout may have started.
nit: Autocheckout

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/data_model_wrapper.h:64:
DISALLOW_COPY_AND_ASSIGN(DataModelWrapper);
I think this needs to be private.

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:255: void
SetCanActivate(bool can_activate);
Why not style instead of activate? All your comments talk about styling.

[Dan Beam, 2013-03-26 18:03:14]

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

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1062: if
(dialog_type_ == DIALOG_TYPE_AUTOCHECKOUT) {
On 2013/03/26 15:27:09, ahutter wrote:
> I think this block with the UpdateButtonStrip needs to be in FinishSubmit or
> things won't work correctly. I think the buttons will get out of whack with
the
> new addition of CVV stuff.

I can move this, but if it's pressing it'd probably be better to do in a
separate CL (unless it's related to this one).

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1070: // Update
the button strip after autocheckout may have started.
On 2013/03/26 15:27:09, ahutter wrote:
> nit: Autocheckout

Done.

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/data_model_wrapper.h:64:
DISALLOW_COPY_AND_ASSIGN(DataModelWrapper);
On 2013/03/26 15:27:09, ahutter wrote:
> I think this needs to be private.

Done.

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:251: void
SetActive(bool active);
^ this is why

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:255: void
SetCanActivate(bool can_activate);
On 2013/03/26 15:27:09, ahutter wrote:
> Why not style instead of activate? All your comments talk about styling.

While I agree and think that Activate is also a views-y term (meaning
focusable), I'll leave this up to estade@ as he wrote the "SetActive()" so I'm
just reusing the Active terminology he started.

[ahutter, 2013-03-26 18:08:24]

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

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1062: if
(dialog_type_ == DIALOG_TYPE_AUTOCHECKOUT) {
On 2013/03/26 18:03:14, Dan Beam wrote:
> On 2013/03/26 15:27:09, ahutter wrote:
> > I think this block with the UpdateButtonStrip needs to be in FinishSubmit or
> > things won't work correctly. I think the buttons will get out of whack with
> the
> > new addition of CVV stuff.
> 
> I can move this, but if it's pressing it'd probably be better to do in a
> separate CL (unless it's related to this one).

VERIFY_CVV used to just fail Wallet so I think it's related to this one.

[Dan Beam, 2013-03-26 19:03:19]

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

https://chromiumcodereview.appspot.com/12893007/diff/13001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1062: if
(dialog_type_ == DIALOG_TYPE_AUTOCHECKOUT) {
On 2013/03/26 18:08:24, ahutter wrote:
> On 2013/03/26 18:03:14, Dan Beam wrote:
> > On 2013/03/26 15:27:09, ahutter wrote:
> > > I think this block with the UpdateButtonStrip needs to be in FinishSubmit
or
> > > things won't work correctly. I think the buttons will get out of whack
with
> > the
> > > new addition of CVV stuff.
> > 
> > I can move this, but if it's pressing it'd probably be better to do in a
> > separate CL (unless it's related to this one).
> 
> VERIFY_CVV used to just fail Wallet so I think it's related to this one.

Done.

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

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1779: case
DIALOG_TYPE_AUTOCHECKOUT:
^ ahutter@: good new spot?

[Evan Stade, 2013-03-26 19:29:54]

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

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1063: did_submit_
= true;
this variable name no longer matches its behavior.

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/autofill/data_model_wrapper.h:35: DataModelWrapper();
why

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:355: active &=
can_activate_;
I don't like that you're using a bitwise operator here, also I don't like
changing the value of a parameter.

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1125:
group.container->SetCanActivate(can_activate);
"can activate" can just be controlled by whether proxy_button_ is visible.

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:253: // Whether this
section can be actively styled or not. If |can_activate| is
I don't know what "actively styled" means.

[ahutter, 2013-03-26 19:47:06]

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

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1779: case
DIALOG_TYPE_AUTOCHECKOUT:
On 2013/03/26 19:03:20, Dan Beam wrote:
> ^ ahutter@: good new spot?

Yep. Thanks.

[Dan Beam, 2013-03-27 00:38:26]

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

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1063: did_submit_
= true;
On 2013/03/26 19:29:54, Evan Stade wrote:
> this variable name no longer matches its behavior.

Done.

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/autofill/data_model_wrapper.h:35: DataModelWrapper();
On 2013/03/26 19:29:54, Evan Stade wrote:
> why

derived classes only found the private copy ctor, made protected

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:355: active &=
can_activate_;
On 2013/03/26 19:29:54, Evan Stade wrote:
> I don't like that you're using a bitwise operator here, also I don't like
> changing the value of a parameter.

Done.

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1125:
group.container->SetCanActivate(can_activate);
On 2013/03/26 19:29:54, Evan Stade wrote:
> "can activate" can just be controlled by whether proxy_button_ is visible.

Done.

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/15003/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:253: // Whether this
section can be actively styled or not. If |can_activate| is
On 2013/03/26 19:29:54, Evan Stade wrote:
> I don't know what "actively styled" means.

s/actively styled/active/

[Evan Stade, 2013-03-27 02:39:38]

lgtm with 2 comments

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1137:
group.container->Refresh();
I think you can piggy back on SetForwardMouseEvents

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:277: bool active_;
don't need this

[Dan Beam, 2013-03-27 02:50:36]

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1137:
group.container->Refresh();
On 2013/03/27 02:39:38, Evan Stade wrote:
> I think you can piggy back on SetForwardMouseEvents

Done.

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://codereview.chromium.org/12893007/diff/28001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:277: bool active_;
On 2013/03/27 02:39:38, Evan Stade wrote:
> don't need this

Done.

[Ilya Sherman, 2013-03-27 23:59:23]

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

https://chromiumcodereview.appspot.com/12893007/diff/43017/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1112:
AutofillMetrics::DIALOG_ACCEPTED);
Should we log the metric now, or after the CVC challenge is completed?  I'd lean
toward the latter.

https://chromiumcodereview.appspot.com/12893007/diff/43017/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/43017/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:448: // continue to
be true while process required actions.
nit: "while process" -> "while processing"

https://chromiumcodereview.appspot.com/12893007/diff/43017/components/autofil...
File components/autofill/browser/credit_card.cc (right):

https://chromiumcodereview.appspot.com/12893007/diff/43017/components/autofil...
components/autofill/browser/credit_card.cc:442: return TypeForDisplay(type_);
nit: Write this as "return CreditCard::TypeForDisplay(type_);" to emphasize that
you're calling a static method.

https://chromiumcodereview.appspot.com/12893007/diff/43017/components/autofil...
File components/autofill/browser/wallet/wallet_items.cc (right):

https://chromiumcodereview.appspot.com/12893007/diff/43017/components/autofil...
components/autofill/browser/wallet/wallet_items.cc:264: string16 display_type =
CreditCard::TypeForDisplay(kGenericCard);
Optional nit: Consider initing to an empty string, and moving kGenericCard into
a catch-all else case.

https://chromiumcodereview.appspot.com/12893007/diff/43017/components/autofil...
File components/autofill/browser/wallet/wallet_items.h (right):

https://chromiumcodereview.appspot.com/12893007/diff/43017/components/autofil...
components/autofill/browser/wallet/wallet_items.h:91: // Returns the display
type of the and last for digits (e.g. Visa - 4444).
nit: "for" -> "four"

[Dan Beam, 2013-03-28 03:38:12]

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

https://codereview.chromium.org/12893007/diff/43017/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1112:
AutofillMetrics::DIALOG_ACCEPTED);
On 2013/03/27 23:59:23, Ilya Sherman wrote:
> Should we log the metric now, or after the CVC challenge is completed?  I'd
lean
> toward the latter.

Moved to FinishSubmit().

https://codereview.chromium.org/12893007/diff/43017/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/12893007/diff/43017/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:448: // continue to
be true while process required actions.
On 2013/03/27 23:59:23, Ilya Sherman wrote:
> nit: "while process" -> "while processing"

Done.

https://codereview.chromium.org/12893007/diff/43017/components/autofill/brows...
File components/autofill/browser/credit_card.cc (right):

https://codereview.chromium.org/12893007/diff/43017/components/autofill/brows...
components/autofill/browser/credit_card.cc:442: return TypeForDisplay(type_);
On 2013/03/27 23:59:23, Ilya Sherman wrote:
> nit: Write this as "return CreditCard::TypeForDisplay(type_);" to emphasize
that
> you're calling a static method.

Done.

https://codereview.chromium.org/12893007/diff/43017/components/autofill/brows...
File components/autofill/browser/wallet/wallet_items.cc (right):

https://codereview.chromium.org/12893007/diff/43017/components/autofill/brows...
components/autofill/browser/wallet/wallet_items.cc:264: string16 display_type =
CreditCard::TypeForDisplay(kGenericCard);
On 2013/03/27 23:59:23, Ilya Sherman wrote:
> Optional nit: Consider initing to an empty string, and moving kGenericCard
into
> a catch-all else case.

well, don't we disagree, lol. anyways, Done.

https://codereview.chromium.org/12893007/diff/43017/components/autofill/brows...
File components/autofill/browser/wallet/wallet_items.h (right):

https://codereview.chromium.org/12893007/diff/43017/components/autofill/brows...
components/autofill/browser/wallet/wallet_items.h:91: // Returns the display
type of the and last for digits (e.g. Visa - 4444).
On 2013/03/27 23:59:23, Ilya Sherman wrote:
> nit: "for" -> "four"

Done.

[Ilya Sherman, 2013-03-28 05:28:56]

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

https://codereview.chromium.org/12893007/diff/59001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:506: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
Hrm, I think you've just globally disabled the cancel button for Autocheckout
unless there was an error.

https://codereview.chromium.org/12893007/diff/59001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1092: if
(!is_submitting_) {
I don't think this is desirable.  The goal is to *not* log this metric if
canceling out of the Autocheckout progress dialog view; but IMO it *should* be
logged if the user cancels during a CVC challenge.

[Dan Beam, 2013-03-29 00:43:10]

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

https://codereview.chromium.org/12893007/diff/59001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:506: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
On 2013/03/28 05:28:57, Ilya Sherman wrote:
> Hrm, I think you've just globally disabled the cancel button for Autocheckout
> unless there was an error.

Added back !is_submitting_

https://codereview.chromium.org/12893007/diff/59001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1092: if
(!is_submitting_) {
On 2013/03/28 05:28:57, Ilya Sherman wrote:
> I don't think this is desirable.  The goal is to *not* log this metric if
> canceling out of the Autocheckout progress dialog view; but IMO it *should* be
> logged if the user cancels during a CVC challenge.

Changed to use callback_.is_null()

[Dan Beam, 2013-03-29 00:47:12]

+sky@ for chrome/browser/ui/views

[Ilya Sherman, 2013-03-29 00:54:04]

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

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:505: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
Autocheckout can get a Risk challenge too, though, right?  It seems like we
should write a method like IsAutocheckoutProgressBarShowing() (or something
shorter and more descriptive of the phase of Autocheckout we're in), and use
that here, as well as when logging the metric.

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1103: // the
error.
nit: I'd move this up to the top of the method and merge it with the comment you
added.

[Dan Beam, 2013-03-29 04:39:49]

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

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:505: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
On 2013/03/29 00:54:04, Ilya Sherman wrote:
> Autocheckout can get a Risk challenge too, though, right?  It seems like we
> should write a method like IsAutocheckoutProgressBarShowing() (or something
> shorter and more descriptive of the phase of Autocheckout we're in), and use
> that here, as well as when logging the metric.

rAc() dialogs should never have a disabled cancel button (AFAIK). this is
autocheckout specific and shouldn't have been effecting rAc().  I only just
noticed while writing this patch, but it has little to do with handling required
actions.

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1103: // the
error.
On 2013/03/29 00:54:04, Ilya Sherman wrote:
> nit: I'd move this up to the top of the method and merge it with the comment
you
> added.

Done.

[Ilya Sherman, 2013-03-29 08:05:52]

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

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:505: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
On 2013/03/29 04:39:49, Dan Beam wrote:
> On 2013/03/29 00:54:04, Ilya Sherman wrote:
> > Autocheckout can get a Risk challenge too, though, right?  It seems like we
> > should write a method like IsAutocheckoutProgressBarShowing() (or something
> > shorter and more descriptive of the phase of Autocheckout we're in), and use
> > that here, as well as when logging the metric.
> 
> rAc() dialogs should never have a disabled cancel button (AFAIK). this is
> autocheckout specific and shouldn't have been effecting rAc().  I only just
> noticed while writing this patch, but it has little to do with handling
required
> actions.

What I'm saying is that an Autocheckout dialog that's in the VERIFY_CVV state
should have the cancel button enabled, even though none of the three conditions
listed here are satisfied by such a dialog.

[Dan Beam, 2013-03-29 09:03:33]

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

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:505: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
On 2013/03/29 08:05:52, Ilya Sherman wrote:
> On 2013/03/29 04:39:49, Dan Beam wrote:
> > On 2013/03/29 00:54:04, Ilya Sherman wrote:
> > > Autocheckout can get a Risk challenge too, though, right?  It seems like
we
> > > should write a method like IsAutocheckoutProgressBarShowing() (or
something
> > > shorter and more descriptive of the phase of Autocheckout we're in), and
use
> > > that here, as well as when logging the metric.
> > 
> > rAc() dialogs should never have a disabled cancel button (AFAIK). this is
> > autocheckout specific and shouldn't have been effecting rAc().  I only just
> > noticed while writing this patch, but it has little to do with handling
> required
> > actions.
> 
> What I'm saying is that an Autocheckout dialog that's in the VERIFY_CVV state
> should have the cancel button enabled, even though none of the three
conditions
> listed here are satisfied by such a dialog.

This patch changes nothing about autocheckout's behavior.

[Dan Beam, 2013-03-29 09:08:43]

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

https://codereview.chromium.org/12893007/diff/79001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:503: // while
Autocheckout is in progress.
^ and this TODO() is mainly why I'm avoiding changing this.

[sky, 2013-03-29 15:35:56]

LGTM

[Evan Stade, 2013-03-29 20:50:07]

https://codereview.chromium.org/12893007/diff/79001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12893007/diff/79001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1092:
controller_->IsSubmitPausedOn(wallet::VERIFY_CVV))) {
I think this logic should move to the controller. Can we have a SectionState
struct which returns suggestion text, icon, extra textfield text, extra
textfield icon, and editability (edit link visibility) which is returned all in
one call to the controller?

[Dan Beam, 2013-03-29 21:00:32]

https://codereview.chromium.org/12893007/diff/79001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12893007/diff/79001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1092:
controller_->IsSubmitPausedOn(wallet::VERIFY_CVV))) {
On 2013/03/29 20:50:08, Evan Stade wrote:
> I think this logic should move to the controller. Can we have a SectionState
> struct which returns suggestion text, icon, extra textfield text, extra
> textfield icon, and editability (edit link visibility) which is returned all
in
> one call to the controller?

talking with estade@ we actually want a suggestion state, not the whole
section's state.

[Ilya Sherman, 2013-03-29 21:33:05]

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

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:505: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
On 2013/03/29 09:03:33, Dan Beam wrote:
> On 2013/03/29 08:05:52, Ilya Sherman wrote:
> > On 2013/03/29 04:39:49, Dan Beam wrote:
> > > On 2013/03/29 00:54:04, Ilya Sherman wrote:
> > > > Autocheckout can get a Risk challenge too, though, right?  It seems like
> we
> > > > should write a method like IsAutocheckoutProgressBarShowing() (or
> something
> > > > shorter and more descriptive of the phase of Autocheckout we're in), and
> use
> > > > that here, as well as when logging the metric.
> > > 
> > > rAc() dialogs should never have a disabled cancel button (AFAIK). this is
> > > autocheckout specific and shouldn't have been effecting rAc().  I only
just
> > > noticed while writing this patch, but it has little to do with handling
> > required
> > > actions.
> > 
> > What I'm saying is that an Autocheckout dialog that's in the VERIFY_CVV
state
> > should have the cancel button enabled, even though none of the three
> conditions
> > listed here are satisfied by such a dialog.
> 
> This patch changes nothing about autocheckout's behavior.

Yes, but that's a problem.

With this patch, Autocheckout has three possible states of the dialog:
  (1) The user is entering data / hasn't clicked submit yet.
  (2) The user has clicked submit, and needs to respond to a Risk challenge.
  (3) The user has clicked submit and responded to any Risk challenges that came
up.

requestAutocomplete doesn't have state (3), because we just close the dialog. 
Autocheckout has state (3), where it is showing the progress bar.  For
Autocheckout, the cancel button should be disabled only in state (3).  However,
you current logic disables it in state (2) as well.

Does that make sense?  Am I missing something?

[Dan Beam, 2013-03-29 23:50:09]

https://codereview.chromium.org/12893007/diff/79001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12893007/diff/79001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1092:
controller_->IsSubmitPausedOn(wallet::VERIFY_CVV))) {
On 2013/03/29 20:50:08, Evan Stade wrote:
> I think this logic should move to the controller. Can we have a SectionState
> struct which returns suggestion text, icon, extra textfield text, extra
> textfield icon, and editability (edit link visibility) which is returned all
in
> one call to the controller?

Done.

[Dan Beam, 2013-03-30 01:28:15]

see if the updated logic works for you

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

https://codereview.chromium.org/12893007/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:505: dialog_type_
== DIALOG_TYPE_REQUEST_AUTOCOMPLETE;
On 2013/03/29 21:33:05, Ilya Sherman wrote:
> On 2013/03/29 09:03:33, Dan Beam wrote:
> > On 2013/03/29 08:05:52, Ilya Sherman wrote:
> > > On 2013/03/29 04:39:49, Dan Beam wrote:
> > > > On 2013/03/29 00:54:04, Ilya Sherman wrote:
> > > > > Autocheckout can get a Risk challenge too, though, right?  It seems
like
> > we
> > > > > should write a method like IsAutocheckoutProgressBarShowing() (or
> > something
> > > > > shorter and more descriptive of the phase of Autocheckout we're in),
and
> > use
> > > > > that here, as well as when logging the metric.
> > > > 
> > > > rAc() dialogs should never have a disabled cancel button (AFAIK). this
is
> > > > autocheckout specific and shouldn't have been effecting rAc().  I only
> just
> > > > noticed while writing this patch, but it has little to do with handling
> > > required
> > > > actions.
> > > 
> > > What I'm saying is that an Autocheckout dialog that's in the VERIFY_CVV
> state
> > > should have the cancel button enabled, even though none of the three
> > conditions
> > > listed here are satisfied by such a dialog.
> > 
> > This patch changes nothing about autocheckout's behavior.
> 
> Yes, but that's a problem.
> 
> With this patch, Autocheckout has three possible states of the dialog:
>   (1) The user is entering data / hasn't clicked submit yet.
>   (2) The user has clicked submit, and needs to respond to a Risk challenge.
>   (3) The user has clicked submit and responded to any Risk challenges that
came
> up.
> 
> requestAutocomplete doesn't have state (3), because we just close the dialog. 
> Autocheckout has state (3), where it is showing the progress bar.  For
> Autocheckout, the cancel button should be disabled only in state (3). 
However,
> you current logic disables it in state (2) as well.
> 
> Does that make sense?  Am I missing something?

Done.

[Ilya Sherman, 2013-03-31 06:04:36]

https://codereview.chromium.org/12893007/diff/106001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12893007/diff/106001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:511: return true;
It doesn't seem like you should need this check at all.  The two dialogs should
behave the same until the progress bar is showing.

https://codereview.chromium.org/12893007/diff/106001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:515: return
had_autocheckout_error_ || !is_submit_going;
I think what you want to check is !callback_.is_null();

[Dan Beam, 2013-04-01 22:17:23]

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

https://chromiumcodereview.appspot.com/12893007/diff/106001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:511: return true;
On 2013/03/31 06:04:37, Ilya Sherman wrote:
> It doesn't seem like you should need this check at all.  The two dialogs
should
> behave the same until the progress bar is showing.

Done.

https://chromiumcodereview.appspot.com/12893007/diff/106001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:515: return
had_autocheckout_error_ || !is_submit_going;
On 2013/03/31 06:04:37, Ilya Sherman wrote:
> I think what you want to check is !callback_.is_null();

Done.

[Ilya Sherman, 2013-04-01 22:55:02]

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

https://chromiumcodereview.appspot.com/12893007/diff/115001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:510: return
!callback_.is_null();
... but you should keep the had_autocheckout_error_ condition, right?

[Dan Beam, 2013-04-01 23:08:38]

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

https://chromiumcodereview.appspot.com/12893007/diff/115001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:510: return
!callback_.is_null();
On 2013/04/01 22:55:03, Ilya Sherman wrote:
> ... but you should keep the had_autocheckout_error_ condition, right?

Done.

[Ilya Sherman, 2013-04-01 23:18:20]

LGTM

[commit-bot: I haz the power, 2013-04-01 23:44:41]

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

[commit-bot: I haz the power, 2013-04-02 01:37:35]

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

[commit-bot: I haz the power, 2013-04-02 03:16:29]

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

[commit-bot: I haz the power, 2013-04-02 03:19:11]

Change committed as 191752