[Autofill] Improve support for various credit card expiration dates.

[Autofill] Improve support for various credit card expiration dates.

Supports 2-digit years, and various combinations of mm/yyyy and single digit month combinations (e.g. m/yy).

BUG=626537
TEST=CreditCardTest,PersonalDataManagerTest

Committed: https://crrev.com/303f4384d13900b31a5b854843b2a18d36176639
Cr-Commit-Position: refs/heads/master@{#404467}

[Mathieu, 2016-07-08 04:58:42]

Patchset #1 (id:1) has been deleted

[Mathieu, 2016-07-08 04:59:22]

Description was changed from

==========
[Autofill] Improve support for various credit card expiration dates.

Supports 2-digit years, and various combinations of mm/yyyy.

BUG=626537
TEST=CreditCardTest,PersonalDataManagerTest
==========

to

==========
[Autofill] Improve support for various credit card expiration dates.

Supports 2-digit years, and various combinations of mm/yyyy and single digit
month combinations (e.g. m/yy).

BUG=626537
TEST=CreditCardTest,PersonalDataManagerTest
==========

[Mathieu, 2016-07-08 04:59:22]

mathp@chromium.org changed reviewers:
+ jdonnelly@chromium.org, sebsg@chromium.org

[Mathieu, 2016-07-08 04:59:55]

PTAL:

Seb: detailed logic
Justin: Mostly FYI but I want to make sure it fits your expectations.

[sebsg, 2016-07-08 10:01:45]

The logic looks good. Just a couple comments on tests. Thanks!

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

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:728: // Check that it fits the
supported patterns: myy, myyyy, mmyy, mmyyyy, m-yy,
nit: it -> the |text|

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

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:170:
TEST(CreditCardTest, SetExpirationYearFromString) {
Can you add tests for 

1 year digit
3 year digits
string with characters

Thanks!

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:186:
TEST(CreditCardTest, SetExpirationDateFromString) {
Can you add an example with multiple separators? And an example with invalid
values and separator?

ex: 05-/2045
    May-2045
    05_2045
    etc

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:188:
a.SetExpirationDateFromString(ASCIIToUTF16("052045"));
Can you put all the cases with no separator together? It will be easier to see
they are all covered.

myy
mmyy
myyyy
mmyyyy

Thanks!

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

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_unittest.cc:2142:
TEST_F(PersonalDataManagerTest, ImportCreditCard_Month2DigitYearCombination) {
There seems to be a some duplicate code between the tests for the expiration
date. Do you think you could use the array of structs way? That way we can see
the test cases more easily and can add some quickly. Thanks!

[Mathieu, 2016-07-08 14:27:05]

Thanks, more robust now. PTAnL!

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

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:728: // Check that it fits the
supported patterns: myy, myyyy, mmyy, mmyyyy, m-yy,
On 2016/07/08 10:01:45, sebsg wrote:
> nit: it -> the |text|

Done.

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

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:170:
TEST(CreditCardTest, SetExpirationYearFromString) {
On 2016/07/08 10:01:45, sebsg wrote:
> Can you add tests for 
> 
> 1 year digit
> 3 year digits
> string with characters
> 
> Thanks!

Done.

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:186:
TEST(CreditCardTest, SetExpirationDateFromString) {
On 2016/07/08 10:01:45, sebsg wrote:
> Can you add an example with multiple separators? And an example with invalid
> values and separator?
> 
> ex: 05-/2045
>     May-2045
>     05_2045
>     etc

Done.

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:188:
a.SetExpirationDateFromString(ASCIIToUTF16("052045"));
On 2016/07/08 10:01:45, sebsg wrote:
> Can you put all the cases with no separator together? It will be easier to see
> they are all covered.
> 
> myy
> mmyy
> myyyy
> mmyyyy
> 
> Thanks!

Done.

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

https://codereview.chromium.org/2136453003/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_unittest.cc:2142:
TEST_F(PersonalDataManagerTest, ImportCreditCard_Month2DigitYearCombination) {
On 2016/07/08 10:01:45, sebsg wrote:
> There seems to be a some duplicate code between the tests for the expiration
> date. Do you think you could use the array of structs way? That way we can see
> the test cases more easily and can add some quickly. Thanks!

reused code when possible. I think the struct abstracts it a little too much (I
want the reader to exactly see which form is submitted).

[sebsg, 2016-07-08 17:23:15]

lgtm with optional nit. Really good test coverage!

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

https://codereview.chromium.org/2136453003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:757: // There is no separator,
the format is either mmyy or mmyyyy.
optional nit: If there is no separator, the supported formats are mmyy and
mmyyyy.

[Justin Donnelly, 2016-07-08 17:45:22]

lgtm

Great change!

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

https://codereview.chromium.org/2136453003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:747: found_separator =
separator;
break? Obviously not a performance issue but as I read this code my brain can't
help but wonder why it's not there. :)

[sebsg, 2016-07-08 17:48:32]

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

https://codereview.chromium.org/2136453003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:747: found_separator =
separator;
On 2016/07/08 17:45:22, Justin Donnelly wrote:
> break? Obviously not a performance issue but as I read this code my brain
can't
> help but wonder why it's not there. :)

I noticed it too, took me a while to parse the loop even though the break
wouldn't change much effectively :P Thanks for pointing it out, I forgot about
it.

[Mathieu, 2016-07-08 18:32:57]

Thanks, submitting!

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

https://codereview.chromium.org/2136453003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:747: found_separator =
separator;
On 2016/07/08 17:48:31, sebsg wrote:
> On 2016/07/08 17:45:22, Justin Donnelly wrote:
> > break? Obviously not a performance issue but as I read this code my brain
> can't
> > help but wonder why it's not there. :)
> 
> I noticed it too, took me a while to parse the loop even though the break
> wouldn't change much effectively :P Thanks for pointing it out, I forgot about
> it.

Done.

https://codereview.chromium.org/2136453003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:747: found_separator =
separator;
On 2016/07/08 17:45:22, Justin Donnelly wrote:
> break? Obviously not a performance issue but as I read this code my brain
can't
> help but wonder why it's not there. :)

Done.

https://codereview.chromium.org/2136453003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:757: // There is no separator,
the format is either mmyy or mmyyyy.
On 2016/07/08 17:23:15, sebsg wrote:
> optional nit: If there is no separator, the supported formats are mmyy and
> mmyyyy.

Done.

[Mathieu, 2016-07-08 18:33:01]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-07-08 18:33:01]

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

[commit-bot: I haz the power, 2016-07-08 18:33:57]

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, 2016-07-08 19:43:23]

Description was changed from

==========
[Autofill] Improve support for various credit card expiration dates.

Supports 2-digit years, and various combinations of mm/yyyy and single digit
month combinations (e.g. m/yy).

BUG=626537
TEST=CreditCardTest,PersonalDataManagerTest
==========

to

==========
[Autofill] Improve support for various credit card expiration dates.

Supports 2-digit years, and various combinations of mm/yyyy and single digit
month combinations (e.g. m/yy).

BUG=626537
TEST=CreditCardTest,PersonalDataManagerTest
==========

[commit-bot: I haz the power, 2016-07-08 19:43:24]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-08 19:46:41]

Description was changed from

==========
[Autofill] Improve support for various credit card expiration dates.

Supports 2-digit years, and various combinations of mm/yyyy and single digit
month combinations (e.g. m/yy).

BUG=626537
TEST=CreditCardTest,PersonalDataManagerTest
==========

to

==========
[Autofill] Improve support for various credit card expiration dates.

Supports 2-digit years, and various combinations of mm/yyyy and single digit
month combinations (e.g. m/yy).

BUG=626537
TEST=CreditCardTest,PersonalDataManagerTest

Committed: https://crrev.com/303f4384d13900b31a5b854843b2a18d36176639
Cr-Commit-Position: refs/heads/master@{#404467}
==========

[commit-bot: I haz the power, 2016-07-08 19:46:42]

Patchset 3 (id:??) landed as
https://crrev.com/303f4384d13900b31a5b854843b2a18d36176639
Cr-Commit-Position: refs/heads/master@{#404467}