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)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: selee, Assigned: selee)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M3])
Attachments
(1 file)
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•7 years ago
|
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment 3•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Keywords: checkin-needed
Comment 13•7 years 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•7 years ago
|
||
bugherder |
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
Updated•7 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•