[WebPayments] Disabling done button when form invalid

[WebPayments] Disabling done button when form invalid

BUG=712224
Review-Url: https://codereview.chromium.org/2905733002
Cr-Commit-Position: refs/heads/master@{#476345}
Committed: https://chromium.googlesource.com/chromium/src/+/39dd914ed229d7d698521a532630068b6308103e

[tmartino, 2017-05-26 18:34:02]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 18:34:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 19:29:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 19:29:40]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-05-29 15:34:08]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 15:34:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tmartino, 2017-05-29 15:58:43]

tmartino@chromium.org changed reviewers:
+ mathp@chromium.org

[tmartino, 2017-05-29 15:58:43]

Hey Mathieu, PTAL.

For the time being, I decided against any form of caching; locally I don't
notice any performance hiccups. I think there are some wins to be had in this
department, but the current structure of the code makes them quite messy.

[commit-bot: I haz the power, 2017-05-29 16:20:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-29 16:20:05]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-05-29 16:45:36]

Looking great, few comments

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:411:
const std::string& locale = state()->GetApplicationLocale();
this function changed a little bit recently, can you rebase?

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:117: virtual
std::unique_ptr<views::View> CreateCustomFieldView(
Can you add a comment here, saying that if a custom view requires model
validation, they should  make sure their fields are tracked in |text_fields_| or
|comboboxes_| (e.g., by using CreateComboboxForField).

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:140: // Returns true
if all fields are valid. Relies on cached validity values,
no cache?

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_browsertest_base.cc
(right):

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:61: void
SetComboboxRowForValue(views::Combobox* combobox,
nit: *SelectComboboxRowForValue?

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:69:
combobox->SetSelectedRow(0);
The SelectBillingAddress code seems to get away with just 1 call to
SetSelectedRow, then OnBlur. Can we do the same?

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:72: // Hit
enter twice to trigger events.
Are we doing this to make sure OnBlur is called on the combobox? If so, could we
call combobox->OnBlur() directly?

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:74:
LOG(ERROR) << "Pressing enter...";
plz remove :)

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:624:
LOG(ERROR) << "Setting billing address";
please fix

[tmartino, 2017-05-29 20:35:40]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 20:35:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tmartino, 2017-05-29 20:40:55]

Description was changed from

==========
[WebPayments] Disabling done button when form invalid

BUG=
==========

to

==========
[WebPayments] Disabling done button when form invalid

BUG=712224
==========

[tmartino, 2017-05-29 21:06:58]

PTAL. Note that I expanded a browsertest which was not touched in the last
patch, to ensure we have coverage on expiration dates on credit cards (since
they have slightly atypical validation rules and events).

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:411:
const std::string& locale = state()->GetApplicationLocale();
On 2017/05/29 at 16:45:36, Mathieu wrote:
> this function changed a little bit recently, can you rebase?

Done

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:117: virtual
std::unique_ptr<views::View> CreateCustomFieldView(
On 2017/05/29 at 16:45:36, Mathieu wrote:
> Can you add a comment here, saying that if a custom view requires model
validation, they should  make sure their fields are tracked in |text_fields_| or
|comboboxes_| (e.g., by using CreateComboboxForField).

Done

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:140: // Returns true
if all fields are valid. Relies on cached validity values,
On 2017/05/29 at 16:45:36, Mathieu wrote:
> no cache?

Done

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_browsertest_base.cc
(right):

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:61: void
SetComboboxRowForValue(views::Combobox* combobox,
On 2017/05/29 at 16:45:36, Mathieu wrote:
> nit: *SelectComboboxRowForValue?

Done

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:69:
combobox->SetSelectedRow(0);
On 2017/05/29 at 16:45:36, Mathieu wrote:
> The SelectBillingAddress code seems to get away with just 1 call to
SetSelectedRow, then OnBlur. Can we do the same?

Yup, done.

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:72: // Hit
enter twice to trigger events.
On 2017/05/29 at 16:45:36, Mathieu wrote:
> Are we doing this to make sure OnBlur is called on the combobox? If so, could
we call combobox->OnBlur() directly?

n/a anymore

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:74:
LOG(ERROR) << "Pressing enter...";
On 2017/05/29 at 16:45:36, Mathieu wrote:
> plz remove :)

n/a

https://codereview.chromium.org/2905733002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:624:
LOG(ERROR) << "Setting billing address";
On 2017/05/29 at 16:45:36, Mathieu wrote:
> please fix

Done

[commit-bot: I haz the power, 2017-05-29 21:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-29 21:19:27]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-05-31 17:50:17]

lgtm, very sorry for the delay

https://codereview.chromium.org/2905733002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2905733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:392: if
(!ValidateModel()) {
no curlies are needed

https://codereview.chromium.org/2905733002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2905733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:143: bool
ValidateModel();
I would consider naming this ValidateInputFields, since it doesn't actually
validate the model (CreditCard or AutofillProfile)

[tmartino, 2017-06-01 15:52:51]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 15:53:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 16:55:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 16:56:00]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-06-01 17:48:13]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-06-01 17:48:13]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2905733002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2017-06-01 17:48:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 17:54:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496339293003460,
"parent_rev": "c3568295e5606b20908b6155fd3cd0052fdb53be", "commit_rev":
"39dd914ed229d7d698521a532630068b6308103e"}

[commit-bot: I haz the power, 2017-06-01 17:54:19]

Description was changed from

==========
[WebPayments] Disabling done button when form invalid

BUG=712224
==========

to

==========
[WebPayments] Disabling done button when form invalid

BUG=712224

Review-Url: https://codereview.chromium.org/2905733002
Cr-Commit-Position: refs/heads/master@{#476345}
Committed:
https://chromium.googlesource.com/chromium/src/+/39dd914ed229d7d698521a532630...
==========

[commit-bot: I haz the power, 2017-06-01 17:54:21]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/39dd914ed229d7d698521a532630...