1367297 - Helper functions for summarizing field names into groups in FormAutofillUtils.

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P1)

Description

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

Since the credit card and addresses are implementing, the information of a field belongs to which groups is useful in many scenarios.

There would be some new functions in FormAutofillUtils:
isAddressField(fieldName)

isCreditCardField(fieldName)

getCategoriesForFieldNames(fieldNames)

Comment 1

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

Attachment: Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

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

Comment 2

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

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

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

Comment 3

[Ray Lin[:ralin]]

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

https://reviewboard.mozilla.org/r/142212/#review145890

r=me with the nit addressed (Thanks, LGTM)

::: browser/extensions/formautofill/FormAutofillUtils.jsm:120
(Diff revision 2)
> +
> +  isCreditCardField(fieldName) {
> +    return this._fieldNameInfo[fieldName].isCreditCardField;
> +  },
> +
> +  getCategoriesFormFieldNames(fieldNames) {

nit: do you mean getCategoriesFromFieldNames?

Comment 4

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

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

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

Comment 5

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

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

https://reviewboard.mozilla.org/r/142212/#review145890

> nit: do you mean getCategoriesFromFieldNames?

Nice catch! thanks.

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

https://reviewboard.mozilla.org/r/142212/#review145938

::: browser/extensions/formautofill/FormAutofillUtils.jsm:14
(Diff revision 3)
> +  _fieldNameInfo: {
> +    "name": {
> +      category: "name",
> +      isAddressField: true,
> +      isCreditCardField: false,
> +    },

IMO, we might not need such a complex structure. I'd prefer something like follows.

```js
_fieldNameInfo: {
  "name": "name",
  "given-name": "name",
  ...
  "cc-exp-year": "credit-card",
},

isAddressField(fieldName) {
  return !isCreditCardField(fieldName);
  
  or
  
  return ["name", "address", ... ].includes(this._fieldNameInfo[fieldName]);
}

isCreditCardField(fieldName) {
  return this._fieldNameInfo[fieldName] === "creditCard";
}
```

Any thought?

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

https://reviewboard.mozilla.org/r/142212/#review146750

::: browser/extensions/formautofill/FormAutofillUtils.jsm:14
(Diff revision 3)
> +  _fieldNameInfo: {
> +    "name": {
> +      category: "name",
> +      isAddressField: true,
> +      isCreditCardField: false,
> +    },

I agree with Luke that this `_fieldNameInfo` could be simplified, but it's up to you.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:112
(Diff revision 3)
> +      isAddressField: false,
> +      isCreditCardField: true,
> +    },
> +  },
> +
> +  isAddressField(fieldName) {

nit: If you keep this structure, maybe you will need to check if the field name is in the `_fieldNameInfo`, otherwise it'll return error(but I guess it seems unlikely to happen)

::: browser/extensions/formautofill/test/unit/test_getCategoriesFromFieldNames.js:6
(Diff revision 3)
> +
> +"use strict";
> +
> +Cu.import("resource://formautofill/FormAutofillUtils.jsm");
> +
> +add_task(function* () {

nit: You can apply async here, and adding function name would produce more information for the test log.

::: browser/extensions/formautofill/test/unit/test_getCategoriesFromFieldNames.js:52
(Diff revision 3)
> +                 info.isCreditCardField,
> +                 "isCreditCardField");
> +  }
> +});
> +
> +add_task(function* () {

nit: Same here

Comment 8

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

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

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

Comment 9

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

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

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

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

https://reviewboard.mozilla.org/r/142212/#review146778

Looks good except one comment.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:37
(Diff revision 5)
> +    "cc-exp-month": "creditCard",
> +    "cc-exp-year": "creditCard",
> +  },
> +
> +  isAddressField(fieldName) {
> +    return !this.isCreditCardField(fieldName);

Agree with Steve that we should check if `fieldName` is in `_fieldNameInfo` first.

Comment 11

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

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142212/diff/5-6/

Comment 12

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

Comment on attachment 8870725 [details]
Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;

https://reviewboard.mozilla.org/r/142212/#review146778

> Agree with Steve that we should check if `fieldName` is in `_fieldNameInfo` first.

Hey Guys, Thanks for reviewing this patch!
The check and related test are added in the latest patch.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e4657f3f91c
Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.; r=lchang,ralin,steveck

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e4657f3f91c