Closed Bug 1367297 Opened 7 years ago Closed 7 years ago

Helper functions for summarizing field names into groups in FormAutofillUtils.

Categories

(Toolkit :: Form Manager, enhancement, P1)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 file)

Bug 1367297 - Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.;
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
steveck
: review+
Details 
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)
Assignee: nobody → selee
Status: NEW → ASSIGNED
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?
Attachment #8870725 - Flags: review?(ralin) → review+
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 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 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
Attachment #8870725 - Flags: review?(schung) → review+
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.
Attachment #8870725 - Flags: review?(lchang) → review+
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.
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e4657f3f91c
Implement getCategoriesFromFieldNames, isAddressField and isCreditCardField in FormAutofillUtils.; r=lchang,ralin,steveck
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4e4657f3f91c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: