Helper functions for summarizing field names into groups in FormAutofillUtils.

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Form Manager
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: seanlee, Assigned: seanlee)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill:M3])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Assignee: nobody → selee
Status: NEW → ASSIGNED

Comment 3

5 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 5

5 months ago
mozreview-review-reply
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

5 months ago
mozreview-review
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

5 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

5 months ago
mozreview-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 hidden (mozreview-request)
(Assignee)

Comment 12

5 months ago
mozreview-review-reply
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.
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 13

5 months ago
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

Comment 14

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e4657f3f91c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

4 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.