[Payments] Add support for required fields in editor validation

[Payments] Add support for required fields in editor validation

BUG=687601
TEST=PaymentRequest interactive_ui_tests

Review-Url: https://codereview.chromium.org/2694693003
Cr-Commit-Position: refs/heads/master@{#449978}
Committed: https://chromium.googlesource.com/chromium/src/+/3367918b26e434d16f477c75443389e6ec2873aa

[Mathieu, 2017-02-13 15:41:26]

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

[Mathieu, 2017-02-13 15:41:27]

Hi, PTAL

[anthonyvd, 2017-02-13 15:53:53]

lgtm % comments.

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/editor_view_controller.h:36: const
base::string16& label,
Can you add a comment about the parameters? label/required are find but
length_hint is a little obscure.

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
File
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc
(right):

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc:100:
DCHECK(textfield);
Don't we prefer EXPECT to DCHECK in tests? EXPECT_TRUE(textfield) would work I
think.

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/validating_textfield.cc (right):

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/validating_textfield.cc:20: // The first
validation of should be on a blur. The subsequent validations
nit: no "of"

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/validating_textfield.cc:21: // will occur when
the contents changes.
nit: "content changes"

[Mathieu, 2017-02-13 15:58:15]

thanks!

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/editor_view_controller.h:36: const
base::string16& label,
On 2017/02/13 15:53:53, anthonyvd wrote:
> Can you add a comment about the parameters? label/required are find but
> length_hint is a little obscure.

Done.

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
File
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc
(right):

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc:100:
DCHECK(textfield);
On 2017/02/13 15:53:53, anthonyvd wrote:
> Don't we prefer EXPECT to DCHECK in tests? EXPECT_TRUE(textfield) would work I
> think.

Done.

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/validating_textfield.cc (right):

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/validating_textfield.cc:20: // The first
validation of should be on a blur. The subsequent validations
On 2017/02/13 15:53:53, anthonyvd wrote:
> nit: no "of"

Done.

https://codereview.chromium.org/2694693003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/validating_textfield.cc:21: // will occur when
the contents changes.
On 2017/02/13 15:53:53, anthonyvd wrote:
> nit: "content changes"

Done.

[Mathieu, 2017-02-13 15:58:29]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-02-13 15:58:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2694693003/#ps20001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-02-13 15:58:58]

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-02-13 16:46:22]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487001509806020,
"parent_rev": "cb664bc9e179ed2ac87ce32a1d862160243be48b", "commit_rev":
"3367918b26e434d16f477c75443389e6ec2873aa"}

[commit-bot: I haz the power, 2017-02-13 16:46:56]

Description was changed from

==========
[Payments] Add support for required fields in editor validation

BUG=687601
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Add support for required fields in editor validation

BUG=687601
TEST=PaymentRequest interactive_ui_tests

Review-Url: https://codereview.chromium.org/2694693003
Cr-Commit-Position: refs/heads/master@{#449978}
Committed:
https://chromium.googlesource.com/chromium/src/+/3367918b26e434d16f477c754433...
==========

[commit-bot: I haz the power, 2017-02-13 16:46:56]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3367918b26e434d16f477c754433...