Closed
Bug 1371131
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Extend ProfileAutoCompleteResult to support credit cards
Categories
(Toolkit :: Form Manager, enhancement, P3)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: lchang, Assigned: ralin)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M4][ETA:7/26])
Attachments
(4 files)
59 bytes,
text/x-review-board-request
|
lchang
:
review+
steveck
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
steveck
:
review+
lchang
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
lchang
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
lchang
:
review+
selee
:
review+
|
Details |
We need to extend ProfileAutoCompleteResult to return a different style and calculate the secondary label for credit cards.
Updated•7 years ago
|
Assignee: nobody → selee
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
Discussed with @all offline, steal this bug from Sean :P
Assignee: selee → ralin
Assignee | ||
Updated•7 years ago
|
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:7/26]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Similarly to ProfileStorage.jsm, I think converting the original prototype-based constructor to class syntax can bring a better basis of extensible result. The current patch is just a WIP, I'll base on it and continue the work.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8886542 [details] Bug 1371131 - Part 2. Refactor ProfileAutoCompleteResult to using class syntax. https://reviewboard.mozilla.org/r/157354/#review164424 ::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:46 (Diff revision 3) > - this.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS; > + this.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS; > - } else { > + } else { > - this.searchResult = Ci.nsIAutoCompleteResult.RESULT_NOMATCH; > + this.searchResult = Ci.nsIAutoCompleteResult.RESULT_NOMATCH; > - } > + } > > + // An array of primary and secondary labels for each profiles. nit: ...each profile.
Attachment #8886542 -
Flags: review?(schung) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
I shouldn't have use `git mv` to rename and edit the file within a commit, I'll split Part 3 into two commits to avoid the irrational changes.
Comment 13•7 years ago
|
||
Please use `hg mv` instead of `git mv`. `git mv` will cut the relation of the old one and the new one.
Assignee | ||
Comment 14•7 years ago
|
||
thanks, rebooting hg repo... :P
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8889286 [details] Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results. https://reviewboard.mozilla.org/r/160334/#review165598 ::: browser/extensions/formautofill/content/formautofill.xml:273 (Diff revision 1) > + if (this.showWarningText) { > - messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler); > + messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler); > + > + this._updateText(); > + } else { > + this._itemBox.setAttribute("noWarning", "true"); not very sure, I guess I should use either @nowarning or @no-warning instead of camel naming to attribute.
Reporter | ||
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8888648 [details] Bug 1371131 - Part 1. Replace the "cc-number-masked" field name in form autofill storage with "cc-number". https://reviewboard.mozilla.org/r/159674/#review165610
Attachment #8888648 -
Flags: review?(lchang) → review+
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8886542 [details] Bug 1371131 - Part 2. Refactor ProfileAutoCompleteResult to using class syntax. https://reviewboard.mozilla.org/r/157354/#review165612
Attachment #8886542 -
Flags: review?(lchang) → review+
Reporter | ||
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8887354 [details] Bug 1371131 - Part 3. Create address and credit card result subclasses. https://reviewboard.mozilla.org/r/158186/#review165628 ::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:151 (Diff revision 4) > + > +class AddressResult extends ProfileAutoCompleteResult { > + constructor(...args) { > + super(...args); > + > + this._popupLabels.push({ You missed the comment "// Add an empty result entry for footer ...". Please also mention that "categories" and "focusedCategory" are used for "also fill" hint. Thanks. ::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:250 (Diff revision 4) > - return JSON.stringify(this._popupLabels[index]); > + super(...args); > + > + this._popupLabels.push({primary: "", secondary: ""}); > } > > - /** > + _getSecondaryLabel(focusedFieldName, allFieldNames, profile) { Looks like most of the code in `_getSecondaryLabel` is the same between `AddressResult` and `CreditCardResult`. Is it possible to have a simple `_adjustSecondaryLabel(currentFieldName, profile)` interface to deal with the different part? ::: browser/extensions/formautofill/test/unit/test_profileAutocompleteResult.js:219 (Diff revision 4) > defaultIndex: 0, > items: [], > }, > }]; > > +matchingProfiles = [{ I prefer not to reuse `matchingProfiles`. How about splitting it into `matchingAddresses` and `matchingCreditCards`.
Attachment #8887354 -
Flags: review?(lchang)
Reporter | ||
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8889286 [details] Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results. https://reviewboard.mozilla.org/r/160334/#review165634 ::: browser/extensions/formautofill/content/formautofill.xml:273 (Diff revision 1) > + if (this.showWarningText) { > - messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler); > + messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler); > + > + this._updateText(); > + } else { > + this._itemBox.setAttribute("noWarning", "true"); Not sure, either. I prefer "no-warning" though.
Attachment #8889286 -
Flags: review?(lchang) → review+
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8887354 [details] Bug 1371131 - Part 3. Create address and credit card result subclasses. https://reviewboard.mozilla.org/r/158186/#review165628 Thanks for the review :-) > Looks like most of the code in `_getSecondaryLabel` is the same between `AddressResult` and `CreditCardResult`. Is it possible to have a simple `_adjustSecondaryLabel(currentFieldName, profile)` interface to deal with the different part? I tried to refactor it in several ways, but at last could not find a proper approach. Since if we decided to make both GROUP_FIELDS and secondaryLabelOrder as instance's property, we will need a good point to re-assign value before invoking super() in subclass, but it's apparently not feasible. Others I can think of are in some way making code worse or more complicated, so perhaps we should leave it as-is for now. Thanks. > I prefer not to reuse `matchingProfiles`. How about splitting it into `matchingAddresses` and `matchingCreditCards`. discussed with you and decided not to change as object assignment shorthand make code cleaner.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8887354 [details] Bug 1371131 - Part 3. Create address and credit card result subclasses. https://reviewboard.mozilla.org/r/158186/#review166018 Ok. That's fine with me. Thanks.
Attachment #8887354 -
Flags: review?(lchang) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8889286 [details] Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results. https://reviewboard.mozilla.org/r/160334/#review166022 LGTM! ::: browser/extensions/formautofill/content/formautofill.xml:173 (Diff revision 3) > this._warningTextBox = document.getAnonymousElementByAttribute( > this, "anonid", "autofill-warning" > ); > > let {categories: allFieldCategories, focusedCategory} = JSON.parse(this.getAttribute("ac-value")); > + this.showWarningText = allFieldCategories && focusedCategory; Don't you consider to add these two lines like you did in `_adjustAcItem`? ```JS this._allFieldCategories = value.categories; this._focusedCategory = value.focusedCategory; ``` ::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:109 (Diff revision 3) > .autofill-footer > .autofill-option-button { > height: 41px; > background-color: #EDEDED; > } > + > +.autofill-footer[no-warning="true"] > .autofill-warning { Just curious. Is there any reason to use `attribute` instead of `class`?
Attachment #8889286 -
Flags: review?(selee) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8889286 [details] Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results. https://reviewboard.mozilla.org/r/160334/#review166022 > Don't you consider to add these two lines like you did in `_adjustAcItem`? > ```JS > this._allFieldCategories = value.categories; > this._focusedCategory = value.focusedCategory; > > ``` Thanks for the spot, I've fixed this. > Just curious. Is there any reason to use `attribute` instead of `class`? I think both are fine. My idea is since we've already done something similar for layouts(1-line and 2-lines), perhaps, be consist with that would be better: changing appearance with different attribute value based on the pre-determined class sets.
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8887354 [details] Bug 1371131 - Part 3. Create address and credit card result subclasses. https://reviewboard.mozilla.org/r/158186/#review166066 ::: browser/extensions/formautofill/test/unit/test_profileAutocompleteResult.js:246 (Diff revision 5) > + "cc-exp-month", > + "cc-exp-year", > +]; > + > +let creditCardTestCases = [{ > + description: "Focus on an `cc-name` field", nit: a `cc-name` field? ::: browser/extensions/formautofill/test/unit/test_profileAutocompleteResult.js:276 (Diff revision 5) > + }), > + image: "", > + }], > + }, > +}, { > + description: "Focus on an `cc-number` field", nit: a `cc-number` field?
Attachment #8887354 -
Flags: review?(schung) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•7 years ago
|
||
Thank you all for the review :D Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c544f60a4c9baffed93eae9b0832d666a4bad573
Keywords: checkin-needed
Comment 36•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/05dde481973c Part 1. Replace the "cc-number-masked" field name in form autofill storage with "cc-number". r=lchang https://hg.mozilla.org/integration/autoland/rev/6efa0b5aaabe Part 2. Refactor ProfileAutoCompleteResult to using class syntax. r=lchang,steveck https://hg.mozilla.org/integration/autoland/rev/1f85c1bfd4bd Part 3. Create address and credit card result subclasses. r=lchang,steveck https://hg.mozilla.org/integration/autoland/rev/91fd8e1b78e3 Part 4. Hide form autofill footer notes if no warning data in the results. r=lchang,seanlee
Keywords: checkin-needed
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05dde481973c https://hg.mozilla.org/mozilla-central/rev/6efa0b5aaabe https://hg.mozilla.org/mozilla-central/rev/1f85c1bfd4bd https://hg.mozilla.org/mozilla-central/rev/91fd8e1b78e3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•