1361237 - [Form Autofill] Implement offline heuristics to determine Credit Card related fields

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P1)

Description

[Sean Lee [:seanlee][:weilonge]]

Implement the offline heuristics to determine the following Credit Card fields:
* cc-name
* cc-number
* cc-exp
* cc-exp-month
* cc-exp-year

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1361237 - Part 1: Use a set of regexp to recognize Credit Card autofill types.;

Review commit: https://reviewboard.mozilla.org/r/135346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135346/

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8863584 [details]
Bug 1361237 - Part 1: Use a set of regexp to recognize Credit Card autofill types.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135346/diff/1-2/

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1361237 - Part 2: Separate the filter criteria of Credit Card and Address fields.;

Review commit: https://reviewboard.mozilla.org/r/146012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146012/

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1361237 - Part 2: Fix all Credit Card related test cases.;

Review commit: https://reviewboard.mozilla.org/r/146014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146014/

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8863584 [details]
Bug 1361237 - Part 1: Use a set of regexp to recognize Credit Card autofill types.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135346/diff/2-3/

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8863584 [details]
Bug 1361237 - Part 1: Use a set of regexp to recognize Credit Card autofill types.;

https://reviewboard.mozilla.org/r/135346/#review168592

I think we should match Chromium's logic/algorithm which I believe we discussed at the All Hands.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8874664 [details]
Bug 1361237 - Part 2: Fix all Credit Card related test cases.;

https://reviewboard.mozilla.org/r/146014/#review168596

I dont' understand what all the FIXME are for on uncommented fields.

::: browser/extensions/formautofill/test/unit/heuristics/third_party/test_CDW.js:38
(Diff revision 1)
> -      [
> +//      {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-number"}, // ac-off
> - /* TODO: Credit Card
> -        {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-type"},
> -        {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-number"},
>          {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-month"},
> -        {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-year"},
> +//      {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-year"}, // FIXME

This should probably be addressed or the comment should be improved ideally pointing to a bug

::: browser/extensions/formautofill/test/unit/heuristics/third_party/test_CostCo.js:52
(Diff revision 1)
>        [
> -/* TODO: credit card
> -        {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-type"},
> +//      {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-type"},
> +//      {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-number"}, // ac-off
> -        {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-number"}, // ac-off
>          {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-month"},
> -        {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-year"},
> +//      {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-year"}, // FIXME

This also doesn't look good. Can you explain the problem

::: browser/extensions/formautofill/test/unit/heuristics/third_party/test_HomeDepot.js:19
(Diff revision 1)
>          {"section": "", "addressType": "", "contactType": "", "fieldName": "address-line1"},
>          {"section": "", "addressType": "", "contactType": "", "fieldName": "postal-code"},
>          {"section": "", "addressType": "billing", "contactType": "", "fieldName": "street-address"}, // <select>
>          {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-month"},
>          {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-exp-year"},
> -//      {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-number"},
> +        {"section": "", "addressType": "", "contactType": "", "fieldName": "cc-number"}, // FIXME

Ditto

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8874663 [details]
Bug 1361237 - Part 2: Separate the filter criteria of Credit Card and Address fields.;

https://reviewboard.mozilla.org/r/146012/#review168598

Please rebase this.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8863584 [details]
Bug 1361237 - Part 1: Use a set of regexp to recognize Credit Card autofill types.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135346/diff/3-4/

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8874664 [details]
Bug 1361237 - Part 2: Fix all Credit Card related test cases.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146014/diff/1-2/

Comment 11

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8874664 [details]
Bug 1361237 - Part 2: Fix all Credit Card related test cases.;

https://reviewboard.mozilla.org/r/146014/#review168596

all FIXME parts have been refined.

> This also doesn't look good. Can you explain the problem

The comment is fixed.
cc-exp-month works fine, but cc-exp-year can not be recognized by the regexp directly. I am thinking if I should implement a parser to fix this.

Comment 12

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8874664 [details]
Bug 1361237 - Part 2: Fix all Credit Card related test cases.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146014/diff/2-3/

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8863584 [details]
Bug 1361237 - Part 1: Use a set of regexp to recognize Credit Card autofill types.;

https://reviewboard.mozilla.org/r/135346/#review176282

rs=me assuming these came straight from Chromium

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8874664 [details]
Bug 1361237 - Part 2: Fix all Credit Card related test cases.;

https://reviewboard.mozilla.org/r/146014/#review176288

r=me if each class of fixme issue gets it's own bug filed and the bug number is in the test comment. We shouldn't be leaving fixme/todo in code anymore without bug numbers now that we're passed laying the foundation. Each of the issues affecting the 12 target sites should be considered for the MVP since they're the goal of the project.

Comment 15

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8874664 [details]
Bug 1361237 - Part 2: Fix all Credit Card related test cases.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146014/diff/3-4/

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Hey MattN, thanks for the review! Each FIXME comment is with a follow-up bug number. I marked all of them as MVP, and it can be adjusted later.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f452e201cbd9
Part 1: Use a set of regexp to recognize Credit Card autofill types.; r=MattN
https://hg.mozilla.org/integration/autoland/rev/e6c68af17717
Part 2: Fix all Credit Card related test cases.; r=MattN

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f452e201cbd9
https://hg.mozilla.org/mozilla-central/rev/e6c68af17717