A single view with editable <input>s for an initial set of fields. The dialog would be passed an optional credit card identifier for the edit/view case. The dialog will initially do no validation and simply load/save credit cards. Simple tests for loading/saving should be written.

Comment on attachment 8899756 [details] Bug 1370764 - (Part 1) Rename editAddress files to editDialog. https://reviewboard.mozilla.org/r/171098/#review176640

Comment on attachment 8899758 [details] Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card. https://reviewboard.mozilla.org/r/171102/#review176664 We might need a test case to check that a profile with invalid (or omitted) cc-number won't be saved. ::: browser/extensions/formautofill/content/editDialog.js:37 (Diff revision 2) > async init() { > if (this._record) { > await this.loadInitialValues(this._record); > } > this.attachEventListeners(); > + window.dispatchEvent(new CustomEvent("FormReady")); Comment on this about its meaning and it's for test only. ::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:54 (Diff revision 2) > + }, {once: true}); > + }); > + let creditCards = await getCreditCards(); > + > + is(creditCards.length, 1, "only one credit card is in storage"); > + is(Object.keys(TEST_CREDIT_CARD_2).length, 4, "Sanity check number of properties"); It seems not to make sense to check the number of properties of a test case. ::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:56 (Diff revision 2) > + if (fieldName === "cc-number") { > + fieldValue = "*".repeat(fieldValue.length - 4) + fieldValue.substr(-4); > + } How about also checking the existence of "cc-number-encrypted"? ::: browser/extensions/formautofill/test/browser/head.js:69 (Diff revision 2) > - "cc-exp-month": 12, > - "cc-exp-year": 2022, > + "cc-exp-month": "12", > + "cc-exp-year": "2022", They are actually numbers in the storage. We shouldn't change its type here.

Comment on attachment 8899758 [details] Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card. https://reviewboard.mozilla.org/r/171102/#review176664 Thanks for the suggestions. I've added a test case to check for invalid credit card entry.

Comment on attachment 8899757 [details] Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. https://reviewboard.mozilla.org/r/171100/#review176782 ::: browser/extensions/formautofill/content/editAddress.xhtml:71 (Diff revision 3) > - // Localize strings before DOMContentLoaded to prevent flash > - window.dialog.localizeDocument(); > + /* global EditAddress */ > + new EditAddress({ I don't see the benefit of passing the elements from xhtml instead of getting them in js directly. The amount of properties and their naming differ between address and credit card. The classes they use are also different. ::: browser/extensions/formautofill/content/editDialog.js:247 (Diff revision 3) > + this._elements.title.dataset.localization = "editCreditCardTitle"; > + } > + FormAutofillUtils.localizeMarkup(AUTOFILL_BUNDLE_URI, document); > + } > + > + isCcNumber(ccNumber = "") { nit: `isCCNumber`. Also, I prefer no default value and to check if `ccNumber` is empty inside the function. ::: browser/extensions/formautofill/content/editDialog.js:262 (Diff revision 3) > + super.loadInitialValues(Object.assign({}, creditCard, {"cc-number": decryptedCC})); > + } > + > + async handleSubmit() { > + let creditCard = this.buildFormObject(); > + // Show error on the cc-number field if it's empty "... if it's empty or invalid."

Comment on attachment 8899758 [details] Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card. https://reviewboard.mozilla.org/r/171102/#review177168 ::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:102 (Diff revision 3) > + setTimeout(() => { > + win.removeEventListener("unload", unloadHandler); > + win.close(); > + resolve(); > + }, 500); Here is a lint error.

Comment on attachment 8899758 [details] Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card. https://reviewboard.mozilla.org/r/171102/#review177168 > Here is a lint error. Thanks. Added `SimpleTest.requestFlakyTimeout`.

Comment on attachment 8899757 [details] Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. https://reviewboard.mozilla.org/r/171100/#review176782 > I don't see the benefit of passing the elements from xhtml instead of getting them in js directly. The amount of properties and their naming differ between address and credit card. The classes they use are also different. I think it's simpler to look at the markup and query them in the JS below, rather than in a separate file. If the markup is changed, it would be more obvious that the code below should also be changed. > nit: `isCCNumber`. > > Also, I prefer no default value and to check if `ccNumber` is empty inside the function. Done. > "... if it's empty or invalid." Thanks.

Comment on attachment 8899757 [details] Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. https://reviewboard.mozilla.org/r/171100/#review177206 ::: browser/extensions/formautofill/content/editDialog.js:248 (Diff revision 4) > + } > + FormAutofillUtils.localizeMarkup(AUTOFILL_BUNDLE_URI, document); > + } > + > + isCCNumber(ccNumber) { > + return ccNumber ? ccNumber.match(/(\s*\d){12,}$/) : false; `ccNumber.replace(/\s/g, "").match(/^\d{12,}$/)` On a second thought, could you put this function in FormAutofillUtils and leverage it in ProfileStorage as well so we won't make different rules in the future.

Comment on attachment 8899757 [details] Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. https://reviewboard.mozilla.org/r/171100/#review176782 > I think it's simpler to look at the markup and query them in the JS below, rather than in a separate file. If the markup is changed, it would be more obvious that the code below should also be changed. Ok, I'm fine with that.

Comment on attachment 8899757 [details] Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. https://reviewboard.mozilla.org/r/171100/#review177206 > `ccNumber.replace(/\s/g, "").match(/^\d{12,}$/)` > > On a second thought, could you put this function in FormAutofillUtils and leverage it in ProfileStorage as well so we won't make different rules in the future. Good idea. Thanks.

Comment on attachment 8899757 [details] Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. https://reviewboard.mozilla.org/r/171100/#review177228 Thanks.

Thanks Luke!

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s dd6b014dffbf -d 8d1350135a04: rebasing 415517:dd6b014dffbf "Bug 1370764 - (Part 1) Rename editAddress files to editDialog. r=lchang" rebasing 415518:366a2aa68c1f "Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. r=lchang" merging browser/extensions/formautofill/FormAutofillUtils.jsm merging browser/extensions/formautofill/ProfileStorage.jsm merging browser/extensions/formautofill/test/unit/test_creditCardRecords.js warning: conflicts while merging browser/extensions/formautofill/ProfileStorage.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Rebased to central.

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c15e0df1c51e (Part 1) Rename editAddress files to editDialog. r=lchang https://hg.mozilla.org/integration/autoland/rev/5e9fefab66fb (Part 2) Create a dialog to add/edit/view a credit card entry. r=lchang https://hg.mozilla.org/integration/autoland/rev/6fb1b3230adf (Part 3) Add browser chrome test for adding and editing credit card. r=lchang

https://hg.mozilla.org/mozilla-central/rev/c15e0df1c51e https://hg.mozilla.org/mozilla-central/rev/5e9fefab66fb https://hg.mozilla.org/mozilla-central/rev/6fb1b3230adf