Autofill: Use field maxlength to improve credit card date matching.

Autofill: Use field maxlength to improve credit card date matching.

BUG=471748
Committed: https://crrev.com/def97be0d5af9cda83e71eb0994f596a6aa80af3
Cr-Commit-Position: refs/heads/master@{#323430}

[Lei Zhang, 2015-04-01 01:29:28]

thestig@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2015-04-01 23:08:27]

https://codereview.chromium.org/1053473002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/credit_card_field.cc (right):

https://codereview.chromium.org/1053473002/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_field.cc:366: static int
kMinimum4YearCcExpLength = strlen("2014-12");
I guess it's a nit but why is this 2014-12 instead of 12/2014? We'll actually
fill 12/2014, so that's the example you should use.

also, second nit: define this where it's actually used

https://codereview.chromium.org/1053473002/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_field.cc:388: if
(current_field_max_length > 0 &&
for consistency with the 2digit year case, shouldn't you move this to before the
ParseFieldSpecifics

[Lei Zhang, 2015-04-02 00:30:13]

https://codereview.chromium.org/1053473002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/credit_card_field.cc (right):

https://codereview.chromium.org/1053473002/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_field.cc:366: static int
kMinimum4YearCcExpLength = strlen("2014-12");
On 2015/04/01 23:08:27, Evan Stade wrote:
> I guess it's a nit but why is this 2014-12 instead of 12/2014? We'll actually
> fill 12/2014, so that's the example you should use.
> 
> also, second nit: define this where it's actually used

Done.

https://codereview.chromium.org/1053473002/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_field.cc:388: if
(current_field_max_length > 0 &&
On 2015/04/01 23:08:26, Evan Stade wrote:
> for consistency with the 2digit year case, shouldn't you move this to before
the
> ParseFieldSpecifics

|kExpirationDateRe| isn't only for 4 digit years. Previously, we've assumed this
will always be a 4 digit year field, but now we check the length and change the
type if necessary.

[Evan Stade, 2015-04-02 00:42:52]

no new form structure browser test?

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

https://codereview.chromium.org/1053473002/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:382: 
remove \n

[Lei Zhang, 2015-04-02 01:28:34]

On 2015/04/02 00:42:52, Evan Stade wrote:
> no new form structure browser test?

The form is really straight-forward and not interesting, so it's not worth
adding a browser test that takes a few seconds to run. Thus I added the much
quicker unit test.

>
https://codereview.chromium.org/1053473002/diff/20001/components/autofill/cor...
> File components/autofill/core/browser/credit_card_field.cc (right):
> 
>
https://codereview.chromium.org/1053473002/diff/20001/components/autofill/cor...
> components/autofill/core/browser/credit_card_field.cc:382: 
> remove \n

Done.

[Evan Stade, 2015-04-02 01:32:22]

On 2015/04/02 01:28:34, Lei Zhang wrote:
> On 2015/04/02 00:42:52, Evan Stade wrote:
> > no new form structure browser test?
> 
> The form is really straight-forward and not interesting, so it's not worth
> adding a browser test that takes a few seconds to run. Thus I added the much
> quicker unit test.

Since we rely on those form structure tests to verify that our changes aren't
breaking sites we've tested in the past, I think you should add one.

> 
> >
>
https://codereview.chromium.org/1053473002/diff/20001/components/autofill/cor...
> > File components/autofill/core/browser/credit_card_field.cc (right):
> > 
> >
>
https://codereview.chromium.org/1053473002/diff/20001/components/autofill/cor...
> > components/autofill/core/browser/credit_card_field.cc:382: 
> > remove \n
> 
> Done.

[Lei Zhang, 2015-04-02 01:42:32]

On 2015/04/02 01:32:22, Evan Stade wrote:
> Since we rely on those form structure tests to verify that our changes aren't
> breaking sites we've tested in the past, I think you should add one.

Ok, patch set 4. Keep the unit test?

[Evan Stade, 2015-04-02 02:01:06]

sure, unit test is fine to keep. thanks. lgtm.

[Lei Zhang, 2015-04-02 02:03:31]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-04-02 02:04:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053473002/60001

[commit-bot: I haz the power, 2015-04-02 05:12:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-03 20:22:50]

Patchset 4 (id:??) landed as
https://crrev.com/def97be0d5af9cda83e71eb0994f596a6aa80af3
Cr-Commit-Position: refs/heads/master@{#323430}