Refactors cvc and expiration date validation in CC upload logic as well as unmask prompt.

Refactors cvc and expiration date validation in CC upload logic as well as unmask prompt.

BUG=677519
Signed-off-by: Mohamad Ahmadi <mahmadi@chromium.org>;
Review-Url: https://codereview.chromium.org/2611863002
Cr-Commit-Position: refs/heads/master@{#441663}
Committed: https://chromium.googlesource.com/chromium/src/+/cff22de0743ef3de687e55c376b9733a0ff560a7

[Moe, 2017-01-03 22:40:22]

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

[commit-bot: I haz the power, 2017-01-03 22:40:51]

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-01-03 23:02:54]

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

[commit-bot: I haz the power, 2017-01-03 23:02:55]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Moe, 2017-01-04 16:05:56]

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

[commit-bot: I haz the power, 2017-01-04 16:06:17]

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

[Moe, 2017-01-04 16:11:26]

Patchset #1 (id:1) has been deleted

[Moe, 2017-01-04 16:12:11]

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

[Moe, 2017-01-04 16:12:12]

Hi Mathieu, Please take a look at this CL.

[Mathieu, 2017-01-04 16:37:22]

https://codereview.chromium.org/2611863002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2611863002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1168:
upload_request_.card.type() == kAmericanExpressCard ? 4 : 3;
Could you use (or extend) this code?

https://cs.chromium.org/chromium/src/components/autofill/core/browser/validat...

[commit-bot: I haz the power, 2017-01-04 17:09:42]

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

[commit-bot: I haz the power, 2017-01-04 17:09:43]

Dry run: This issue passed the CQ dry run.

[Moe, 2017-01-04 18:06:56]

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

[Moe, 2017-01-04 18:07:07]

Description was changed from

==========
Adds a length check for CVC to the Credit Card upload logic.

BUG=677519

Signed-off-by: Mohamad Ahmadi <mahmadi@chromium.org>
==========

to

==========
Refactors cvc and expiration date validation in CC upload logic as well as
unmask prompt.

BUG=677519

Signed-off-by: Mohamad Ahmadi <mahmadi@chromium.org>
==========

[commit-bot: I haz the power, 2017-01-04 18:07:20]

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

[Moe, 2017-01-04 18:07:31]

https://codereview.chromium.org/2611863002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2611863002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1168:
upload_request_.card.type() == kAmericanExpressCard ? 4 : 3;
On 2017/01/04 16:37:22, Mathieu Perreault wrote:
> Could you use (or extend) this code?
> 
>
https://cs.chromium.org/chromium/src/components/autofill/core/browser/validat...

Done.

[Mathieu, 2017-01-04 18:20:17]

woot thanks for the cleanup, couple of nits

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc
(right):

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc:301:
base::Time::Exploded now;
move to if block?

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

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:90: bool
IsValidCreditCardSecurityCode(const base::string16& text) {
From quick codesearch, looks like this is unused. Can you do a cleanup of the
unused IsValidCreditCardSecurityCode function(s)?

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:93: return
base::ContainsOnlyChars(text, base::ASCIIToUTF16("0123456789"));;
nit: double ;

[Moe, 2017-01-04 18:31:28]

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc
(right):

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc:301:
base::Time::Exploded now;
On 2017/01/04 18:20:17, Mathieu Perreault wrote:
> move to if block?

Done.

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

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:90: bool
IsValidCreditCardSecurityCode(const base::string16& text) {
On 2017/01/04 18:20:17, Mathieu Perreault wrote:
> From quick codesearch, looks like this is unused. Can you do a cleanup of the
> unused IsValidCreditCardSecurityCode function(s)?

Looks like it's being used in
https://cs.chromium.org/chromium/src/components/autofill/core/browser/validat...
as far as I can tell there's no unused function here.

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:93: return
base::ContainsOnlyChars(text, base::ASCIIToUTF16("0123456789"));;
On 2017/01/04 18:20:17, Mathieu Perreault wrote:
> nit: double ;

Done.

[Mathieu, 2017-01-04 19:12:38]

lgtm with comment

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

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:90: bool
IsValidCreditCardSecurityCode(const base::string16& text) {
On 2017/01/04 18:31:28, moe wrote:
> On 2017/01/04 18:20:17, Mathieu Perreault wrote:
> > From quick codesearch, looks like this is unused. Can you do a cleanup of
the
> > unused IsValidCreditCardSecurityCode function(s)?
> 
> Looks like it's being used in
>
https://cs.chromium.org/chromium/src/components/autofill/core/browser/validat...
> as far as I can tell there's no unused function here.

Yes but I would say the test doesn't count. Let's remove the function and its
unittest

[Moe, 2017-01-05 05:25:03]

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

[Moe, 2017-01-05 05:25:20]

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

https://codereview.chromium.org/2611863002/diff/40001/components/autofill/cor...
components/autofill/core/browser/validation.cc:90: bool
IsValidCreditCardSecurityCode(const base::string16& text) {
On 2017/01/04 19:12:37, Mathieu Perreault wrote:
> On 2017/01/04 18:31:28, moe wrote:
> > On 2017/01/04 18:20:17, Mathieu Perreault wrote:
> > > From quick codesearch, looks like this is unused. Can you do a cleanup of
> the
> > > unused IsValidCreditCardSecurityCode function(s)?
> > 
> > Looks like it's being used in
> >
>
https://cs.chromium.org/chromium/src/components/autofill/core/browser/validat...
> > as far as I can tell there's no unused function here.
> 
> Yes but I would say the test doesn't count. Let's remove the function and its
> unittest

Oh i see. My bad. Removed the unused functions and modified the tests to cover
the new one.

[commit-bot: I haz the power, 2017-01-05 05:25:22]

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-01-05 06:13:08]

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

[commit-bot: I haz the power, 2017-01-05 06:13:09]

Dry run: This issue passed the CQ dry run.

[Moe, 2017-01-05 15:56:37]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-01-05 15:56:38]

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/2611863002/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-01-05 15:56:54]

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-01-05 16:04:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483631797722930,
"parent_rev": "54b068fa0ee84f3bdfcdf41b9f36cb47e5001104", "commit_rev":
"cff22de0743ef3de687e55c376b9733a0ff560a7"}

[commit-bot: I haz the power, 2017-01-05 16:04:57]

Description was changed from

==========
Refactors cvc and expiration date validation in CC upload logic as well as
unmask prompt.

BUG=677519

Signed-off-by: Mohamad Ahmadi <mahmadi@chromium.org>
==========

to

==========
Refactors cvc and expiration date validation in CC upload logic as well as
unmask prompt.

BUG=677519

Signed-off-by: Mohamad Ahmadi <mahmadi@chromium.org>
Review-Url: https://codereview.chromium.org/2611863002
Cr-Commit-Position: refs/heads/master@{#441663}
Committed:
https://chromium.googlesource.com/chromium/src/+/cff22de0743ef3de687e55c376b9...
==========

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

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cff22de0743ef3de687e55c376b9...