[Autofill] Fix heuristics to detect consecutive CVCs.

[Autofill] Fix heuristics to detect consecutive CVCs.

Added a check after parsing a cvc. If it's the only type parsed so far, check the previous field. If it's also a cvc, just label the current field as a cvc without looking at the next fields. This also fixes some cases where we would label some names as cc-names.

BUG=591498
Committed: https://crrev.com/a9224ac0cc140b536c8e13099fcf3cfc4cc4a80d
Cr-Commit-Position: refs/heads/master@{#380916}

[sebsg, 2016-03-02 23:10:48]

Description was changed from

==========
[Autofill] Fix heuristics to detect consecutive CVCs.

Added a check after parsing a cvc. If it's the only type parsed so far, check
the previous field. If it's also a cvc, just label the current field as a cvc
without looking at the next fields. This also fixes some cases where we would
label some names as cc-names.

BUG=591498
==========

to

==========
[Autofill] Fix heuristics to detect consecutive CVCs.

Added a check after parsing a cvc. If it's the only type parsed so far, check
the previous field. If it's also a cvc, just label the current field as a cvc
without looking at the next fields. This also fixes some cases where we would
label some names as cc-names.

BUG=591498
==========

[sebsg, 2016-03-02 23:10:48]

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

[sebsg, 2016-03-03 16:14:28]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-03-03 16:15:22]

Could you please take a look? Thanks!

[Mathieu, 2016-03-04 18:54:38]

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

[Mathieu, 2016-03-04 18:54:39]

Looks OK, would like Daniel to have a look.

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

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:143: kMatchNumAndTel |
MATCH_PASSWORD,
note: add MATCH_PASSWORD to the constant above.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:147: AutofillField* cvv =
credit_card_field->verification_;
rename to |saved_cvv|?

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:149: // Make sure that
only a verification code was found.
Combine the previous comment with this:

// If the verification code is the first detected field in the newly started
card,

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:162: return
std::move(credit_card_field);
wait, isn't |credit_card_field| mostly empty here? Why are we returning?

[Toyama, 2016-03-04 19:08:50]

Hi Seb, I don't completely understand the intuition behind the solution, but the
diffs look good.

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

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:145: // A couple of sites
have multiple verification codes right after an
s/an another/another/

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:147: AutofillField* cvv =
credit_card_field->verification_;
const

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:149: // Make sure that
only a verification code was found.
s/a/one/

[Toyama, 2016-03-04 19:29:41]

lgtm

Chatted offline with Seb. This is good and should decrease the number of unknown
types.

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

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:143: kMatchNumAndTel |
MATCH_PASSWORD,
On 2016/03/04 18:54:38, Mathieu Perreault wrote:
> note: add MATCH_PASSWORD to the constant above.

I think that makes sense, but it might introduce diffs. Because there's an extra
call to parse using just that match type. I think it'd be better to do this in a
separate CL.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:227: bool
has_cc_number_or_verification = (credit_card_field->verification_ ||
This is not your change, but this should be const.

[sebsg, 2016-03-09 16:19:14]

Patchset #3 (id:60001) has been deleted

[sebsg, 2016-03-09 16:38:36]

Could you please take another look? Thanks!

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

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:143: kMatchNumAndTel |
MATCH_PASSWORD,
On 2016/03/04 18:54:38, Mathieu Perreault wrote:
> note: add MATCH_PASSWORD to the constant above.

Once I rebase the other will use password also, so I'll do it then!

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:143: kMatchNumAndTel |
MATCH_PASSWORD,
On 2016/03/04 19:29:40, Toyama wrote:
> On 2016/03/04 18:54:38, Mathieu Perreault wrote:
> > note: add MATCH_PASSWORD to the constant above.
> 
> I think that makes sense, but it might introduce diffs. Because there's an
extra
> call to parse using just that match type. I think it'd be better to do this in
a
> separate CL.

Done.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:145: // A couple of sites
have multiple verification codes right after an
On 2016/03/04 19:08:49, Toyama wrote:
> s/an another/another/

Done.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:147: AutofillField* cvv =
credit_card_field->verification_;
On 2016/03/04 18:54:38, Mathieu Perreault wrote:
> rename to |saved_cvv|?

Done.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:147: AutofillField* cvv =
credit_card_field->verification_;
On 2016/03/04 19:08:50, Toyama wrote:
> const

Done.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:149: // Make sure that
only a verification code was found.
On 2016/03/04 18:54:38, Mathieu Perreault wrote:
> Combine the previous comment with this:
> 
> // If the verification code is the first detected field in the newly started
> card, 

Done.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:149: // Make sure that
only a verification code was found.
On 2016/03/04 19:08:49, Toyama wrote:
> s/a/one/

Done.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:162: return
std::move(credit_card_field);
On 2016/03/04 18:54:38, Mathieu Perreault wrote:
> wait, isn't |credit_card_field| mostly empty here? Why are we returning?

At this point it contains only a CVV. The effect of this will be that this cvv
will be classified as such and won't actually be the start of a new credit card.

https://codereview.chromium.org/1753253003/diff/20001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:227: bool
has_cc_number_or_verification = (credit_card_field->verification_ ||
On 2016/03/04 19:29:40, Toyama wrote:
> This is not your change, but this should be const.

Done.

[Mathieu, 2016-03-09 16:52:35]

lgtm

[sebsg, 2016-03-09 19:05:53]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-03-09 19:05:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
kenjitoyama@chromium.org
Link to the patchset: https://codereview.chromium.org/1753253003/#ps100001
(title: "After rebase adjustments")

[commit-bot: I haz the power, 2016-03-09 19:06:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1753253003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1753253003/100001

[commit-bot: I haz the power, 2016-03-09 22:39:49]

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

[commit-bot: I haz the power, 2016-03-09 22:39:50]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sebsg, 2016-03-13 19:59:36]

Hey Math could you take another look? I added an obvious scanner check. Sorry
about that!

[Mathieu, 2016-03-14 00:16:14]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-03-14 00:16:14]

lgtm

[Mathieu, 2016-03-14 00:16:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
kenjitoyama@chromium.org
Link to the patchset: https://codereview.chromium.org/1753253003/#ps120001
(title: "Added scanner check")

[commit-bot: I haz the power, 2016-03-14 00:16:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1753253003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1753253003/120001

[commit-bot: I haz the power, 2016-03-14 01:41:45]

Description was changed from

==========
[Autofill] Fix heuristics to detect consecutive CVCs.

Added a check after parsing a cvc. If it's the only type parsed so far, check
the previous field. If it's also a cvc, just label the current field as a cvc
without looking at the next fields. This also fixes some cases where we would
label some names as cc-names.

BUG=591498
==========

to

==========
[Autofill] Fix heuristics to detect consecutive CVCs.

Added a check after parsing a cvc. If it's the only type parsed so far, check
the previous field. If it's also a cvc, just label the current field as a cvc
without looking at the next fields. This also fixes some cases where we would
label some names as cc-names.

BUG=591498
==========

[commit-bot: I haz the power, 2016-03-14 01:41:46]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-03-14 01:43:02]

Description was changed from

==========
[Autofill] Fix heuristics to detect consecutive CVCs.

Added a check after parsing a cvc. If it's the only type parsed so far, check
the previous field. If it's also a cvc, just label the current field as a cvc
without looking at the next fields. This also fixes some cases where we would
label some names as cc-names.

BUG=591498
==========

to

==========
[Autofill] Fix heuristics to detect consecutive CVCs.

Added a check after parsing a cvc. If it's the only type parsed so far, check
the previous field. If it's also a cvc, just label the current field as a cvc
without looking at the next fields. This also fixes some cases where we would
label some names as cc-names.

BUG=591498

Committed: https://crrev.com/a9224ac0cc140b536c8e13099fcf3cfc4cc4a80d
Cr-Commit-Position: refs/heads/master@{#380916}
==========

[commit-bot: I haz the power, 2016-03-14 01:43:03]

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