[Payments] Add validation for unsupported credit card types in editor.

[Payments] Add validation for unsupported credit card types in editor.

BUG=687601
TEST=components_unittests, interactive_ui_tests

Review-Url: https://codereview.chromium.org/2713873002
Cr-Commit-Position: refs/heads/master@{#452557}
Committed: https://chromium.googlesource.com/chromium/src/+/60dfd97938a210a44495710ea75cb100c48cf8fd

[Mathieu, 2017-02-23 16:24:49]

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

[commit-bot: I haz the power, 2017-02-23 16:25:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-02-23 16:26:56]

Patchset #2 (id:20001) has been deleted

[Mathieu, 2017-02-23 16:27:03]

Patchset #1 (id:1) has been deleted

[Mathieu, 2017-02-23 16:27:18]

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

[Mathieu, 2017-02-23 16:27:19]

PTAL!

[Mathieu, 2017-02-23 16:27:25]

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

[commit-bot: I haz the power, 2017-02-23 16:28:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[anthonyvd, 2017-02-23 16:42:28]

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

https://codereview.chromium.org/2713873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:192: //
We do not pass the supported card networks for non-cc-number types to avoid
Nit: the style guide recommends not formulating comments as "We...". Maybe "The
supported card networks [...] is not passed to avoid [...]"

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

https://codereview.chromium.org/2713873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:42: //
passed in if it's thought to be used to validate |field| (the data will
nit: not sure what "if it's thought to be used to validate |field|" means.

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:111: if (error_message) {
I would prefer just commenting in the .h that error_message can't be null and
DCHECK(error_message) at the beginning of this function.

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:299: NOTREACHED() <<
"IsValidCreditCardNumberForBasicCardNetworks should be "
Are you sure this wasn't already used with CREDIT_CARD_NUMBER in any other code
that you'd now be breaking? I'd be surprised if this code was dead previously.

[Mathieu, 2017-02-23 16:48:31]

Thanks for picking up on my shitty comments. PTAL

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

https://codereview.chromium.org/2713873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:192: //
We do not pass the supported card networks for non-cc-number types to avoid
On 2017/02/23 16:42:27, anthonyvd wrote:
> Nit: the style guide recommends not formulating comments as "We...". Maybe
"The
> supported card networks [...] is not passed to avoid [...]"

Done.

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

https://codereview.chromium.org/2713873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:42: //
passed in if it's thought to be used to validate |field| (the data will
On 2017/02/23 16:42:27, anthonyvd wrote:
> nit: not sure what "if it's thought to be used to validate |field|" means.

Done.

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:111: if (error_message) {
On 2017/02/23 16:42:27, anthonyvd wrote:
> I would prefer just commenting in the .h that error_message can't be null and
> DCHECK(error_message) at the beginning of this function.

Good idea!

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:299: NOTREACHED() <<
"IsValidCreditCardNumberForBasicCardNetworks should be "
On 2017/02/23 16:42:28, anthonyvd wrote:
> Are you sure this wasn't already used with CREDIT_CARD_NUMBER in any other
code
> that you'd now be breaking? I'd be surprised if this code was dead previously.

Yes this function is new for PaymentRequest (I wrote it)

[anthonyvd, 2017-02-23 17:00:19]

LGTM!

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2713873002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:299: NOTREACHED() <<
"IsValidCreditCardNumberForBasicCardNetworks should be "
On 2017/02/23 at 16:48:31, Mathieu Perreault wrote:
> On 2017/02/23 16:42:28, anthonyvd wrote:
> > Are you sure this wasn't already used with CREDIT_CARD_NUMBER in any other
code
> > that you'd now be breaking? I'd be surprised if this code was dead
previously.
> 
> Yes this function is new for PaymentRequest (I wrote it)

Oh haha, my bad!

[Mathieu, 2017-02-23 17:37:44]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-02-23 17:38:08]

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-23 18:25:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487871464425300,
"parent_rev": "b57bfa9b558cf020699b7d3587b8331020f82897", "commit_rev":
"60dfd97938a210a44495710ea75cb100c48cf8fd"}

[commit-bot: I haz the power, 2017-02-23 18:27:14]

Description was changed from

==========
[Payments] Add validation for unsupported credit card types in editor.

BUG=687601
TEST=components_unittests, interactive_ui_tests
==========

to

==========
[Payments] Add validation for unsupported credit card types in editor.

BUG=687601
TEST=components_unittests, interactive_ui_tests

Review-Url: https://codereview.chromium.org/2713873002
Cr-Commit-Position: refs/heads/master@{#452557}
Committed:
https://chromium.googlesource.com/chromium/src/+/60dfd97938a210a44495710ea75c...
==========

[commit-bot: I haz the power, 2017-02-23 18:27:15]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/60dfd97938a210a44495710ea75c...