Breaking down Story into multiple smaller, easier-to-estimate bugs. These individual bugs are dependencies which block the completion of the whole story. The team provides point estimates to each of the individual bugs instead of the entire story.

This is quite difficult to do for all locales if the page isn't using the new attributes Google proposed[1] as it would rely on many heuristics. I would suggest we implement this in toolkit so Android can use the same backend. [1] http://wiki.whatwg.org/wiki/Autocomplete_Types

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. Hi MattN/Luke This patch is simply about applying profile saving while form submitted. Profile updating and doorhanger will be introduced in another bugs. Test will be added once you're ok with the patch(or you prefer to address bug 1348193 first), thanks!

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review125828 ::: browser/extensions/formautofill/FormAutofillContent.jsm:394 (Diff revision 2) > formFillController.markAsAutofillField(field); > }, > + > + _maybeSaveProfile(guid, details) { > + let profile = this._detailsToProfile(details); > + if (Object.keys(profile).length < AUTOFILL_FIELDS_THRESHOLD) { Does it mean that we don't save the profile unless 3 or more fields are actually filled?

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review125828 > Does it mean that we don't save the profile unless 3 or more fields are actually filled? My original idea is for preventing meaningless profile saved in storage, since it shouldn't be a common case that the valid profile is created by less than 3 fields. I'll ni Juwei for more insight form UX's perspective.

Hi Juwei, do you think we should set a threshold while saving the profile after form submitted? I left my thoughts in comment 6, but I'm not quite sure it's ok because we don't have any limitation while saving the profile via preference.

I think 3 fields is a reasonable number. If it's less than 3 fields, users still can use form history to fill out the form.

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. Looks good. Thanks.

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review127488 I guess this mostly needs tests but otherwise it's not too complicated. ::: browser/extensions/formautofill/FormAutofillContent.jsm:382 (Diff revision 2) > this._formsDetails.set(form.rootElement, formHandler); > + // TODO: should be able to support non-formElement form in bug 1348193. > + form.rootElement.addEventListener("submit", () => { > + let {filledProfileGUID, fieldDetails} = formHandler; > + this._maybeSaveProfile(filledProfileGUID, fieldDetails); > + }, {once: true}); I don't think `once` is what you want. If I submit a form, get an error, then click back to make a correction and the page is in the back-forward (bfcache) then I think you won't get the event when I submit again. You should also consider if the pages calls preventDefault for the "submit" event. I forget the timing of the events because no form code in mozilla-central uses the submit event, they all use a "earlyformsubmit" observer before the submit event which also means it doesn't hold a <form> reference. For consistency and to make it easier to avoid leaks I think it may be worth switching to a form submit observer which then checks if the form element is a relevant one. See https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/toolkit/components/passwordmgr/LoginManagerContent.jsm#53-69,146 for an example. ::: browser/extensions/formautofill/FormAutofillContent.jsm:399 (Diff revision 2) > + if (Object.keys(profile).length < AUTOFILL_FIELDS_THRESHOLD) { > + return; > + } > + > + if (guid) { > + // TODO: get the profile and check if user update the fields. This can be done in a different bug/commit if you want… I think it will be easier to review separately. ::: browser/extensions/formautofill/FormAutofillContent.jsm:401 (Diff revision 2) > + } > + > + if (guid) { > + // TODO: get the profile and check if user update the fields. > + } else { > + Services.cpmm.sendAsyncMessage("FormAutofill:SaveProfile", {profile}); Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that.

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review127488 > Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that. You mean we shouldn't save the profile if the data is not from autofill storage directly? I didn't know that we have such requirement in MVP. Let me doublecheck with Luke again.

> > Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that. > > You mean we shouldn't save the profile if the data is not from autofill > storage directly? I didn't know that we have such requirement in MVP. Let me > doublecheck with Luke again. I think Matt was saying that we shouldn't save the profile which is identical to one of the saved profiles in the storage but wasn't autofilled by Form Autofill feature. I personally think we can do this in MVP.

(In reply to Luke Chang [:lchang] from comment #12) > > > Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that. > > > > You mean we shouldn't save the profile if the data is not from autofill > > storage directly? I didn't know that we have such requirement in MVP. Let me > > doublecheck with Luke again. > > I think Matt was saying that we shouldn't save the profile which is > identical to one of the saved profiles in the storage but wasn't autofilled > by Form Autofill feature. I personally think we can do this in MVP. Oh yes, I think it's not difficult if we only perform strict comparison. I'll implement this in ProfileStorage add API(maybe redirect to update API since UX prefers to treat it as "update" than doing nothing).

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review133052 ::: commit-message-e9e23:1 (Diff revision 4) > +Bug 990219 - Part 1: Add submit event listener and dispatch saveProfile message. r?MattN The commit message is now stale since the submit event listener isn't added/used

Hi Steve, We should avoid saving user's profile in the private browsing mode. Is it possible to cover that part in this bug?

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review137060 Please add the private browsing check and add a mochitest for it. ::: browser/extensions/formautofill/FormAutofillContent.jsm:322 (Diff revision 5) > + _detailsToProfile(details) { > + let profile = {}; > + > + details.forEach(detail => { Have you considered putting this method on the handler instead of FormAutofillContent? ::: browser/extensions/formautofill/FormAutofillContent.jsm:363 (Diff revision 5) > + _saveProfile(guid, profile) { > + if (guid) { As Luke said, we need to use PrivateBrowsingUtils.isContentWindowPrivate to not save anything to storage in PB mode. You will need to do that check in a method which has a reference to the window e.g. use the .ownerDocument.defaultView from the form rootElement. ::: browser/extensions/formautofill/FormAutofillContent.jsm:365 (Diff revision 5) > + * @param {Object} profile > + * The new profile used to add or overwrite the old one. > + */ > + _saveProfile(guid, profile) { > + if (guid) { > + // TODO: get the profile and check if user update the fields. Please add a bug number ::: browser/extensions/formautofill/test/unit/test_onFormSubmitted.js:77 (Diff revision 5) > + }, > + }, > + }, > +]; > + > add_task(function* () { Please add a name to this function so it shows in logs. I didn't notice it was missing before

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review137068 ::: browser/extensions/formautofill/ProfileStorage.jsm:323 (Diff revision 2) > _saveImmediately() { > return this._store._save(); > }, > + > + /** > + * Check if storage has the same profile saved. s/the same/a matching/ ::: browser/extensions/formautofill/ProfileStorage.jsm:327 (Diff revision 2) > + /** > + * Check if storage has the same profile saved. > + * @param {Profile} targetProfile > + * The profile for duplication check before saving. > + * @returns {string|null} > + * Return guid string if there's a matched profile in storage or null if not. s/matched/matching/ ::: browser/extensions/formautofill/ProfileStorage.jsm:329 (Diff revision 2) > + * @param {Profile} targetProfile > + * The profile for duplication check before saving. > + * @returns {string|null} > + * Return guid string if there's a matched profile in storage or null if not. > + */ > + _dupProfileChecking(targetProfile) { Nit: `_findMatchingProfile` ::: browser/extensions/formautofill/ProfileStorage.jsm:332 (Diff revision 2) > + if (size != Object.keys(profile).length - this.INTERNAL_FIELDS.length) { > + continue; > + } > + > + let data = this._clone(profile); > + this.INTERNAL_FIELDS.forEach((field) => { > + delete data[field]; > + }); > + > + for (let key in targetProfile) { > + if (!data[key] || data[key] != targetProfile[key]) { > + break; > + } > + delete data[key]; > + } > + if (Object.keys(data).length == 0) { > + return profile.guid; > + } Can you factor this out into a separate `_areProfilesMatching` method in this JSM? Sync also needs the same type of method.

(In reply to Luke Chang [:lchang] from comment #20) > Hi Steve, > > We should avoid saving user's profile in the private browsing mode. Is it > possible to cover that part in this bug? I added the private browsing mode checking in the first patch. Hi Matt, do you prefer to land the Bug 1325538 first then add mochitest on top of it, or land this patch first then add private browsing mochitest in Bug 1325538? Since you've reviewed both patches and the they are all updated, I'm fine for both options if we could land one of the patches first.

Comment on attachment 8863672 [details] Bug 990219 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/135476/#review138454 ::: browser/extensions/formautofill/test/mochitest/test_submitting_profile.html:1 (Diff revision 1) > +<!DOCTYPE HTML> Hi Matt, do you know how we could trigger a test under private mode window directly? I saw some tests opened private window manually with another specific url and verify by manipulating the private window instance. Should we also open another testing html using private window with form inside and verify if the storage changed in the original testing file?

Comment on attachment 8863672 [details] Bug 990219 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/135476/#review138454 > Hi Matt, do you know how we could trigger a test under private mode window directly? I saw some tests opened private window manually with another specific url and verify by manipulating the private window instance. Should we also open another testing html using private window with form inside and verify if the storage changed in the original testing file? I added another browser test for private browsing mode and normal mode. I'm not sure if it's still necessary to keep the plain mochitest one, so you could see the tests were verifying the form submitting in the normal window. Just let me know if you feel keeping browser one is sufficient.

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review140510 ::: browser/extensions/formautofill/ProfileStorage.jsm:344 (Diff revision 4) > + * @param {Profile} ProfileB > + * Another profile for complete data match checking. > + * @returns {boolean} > + * Return true if 2 profiles have same content and false if not. > + */ > + _areProfilesMatching(profileA, profileB) { I feel that what we need isn't an exactly-matching function. In my imagination, two profiles will be merged if every field in the existing profile either doesn't exist in the incoming profile or be identical to the same field in the incoming profile. For example: (`profileA` the existing one and `profileB` is the incoming one.) - profileA has {A, B, C} and profileB has {B, C}: merge to {A, B, C} - profileA has {A, B, C} and profileB has {A, B, D}: merge to {A, B, C, D} - profileA has {A, B, C} and profileB has {A, B, C'}: conflicts If so, what we need might be a `_canMerge(profileA, profileB)` function. Could you confirm that with UX?

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review140510 > I feel that what we need isn't an exactly-matching function. In my imagination, two profiles will be merged if every field in the existing profile either doesn't exist in the incoming profile or be identical to the same field in the incoming profile. > > For example: > > (`profileA` the existing one and `profileB` is the incoming one.) > > - profileA has {A, B, C} and profileB has {B, C}: merge to {A, B, C} > - profileA has {A, B, C} and profileB has {A, B, D}: merge to {A, B, C, D} > - profileA has {A, B, C} and profileB has {A, B, C'}: conflicts > > If so, what we need might be a `_canMerge(profileA, profileB)` function. > > Could you confirm that with UX? I agree with Luke on this.

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review140750 ::: browser/extensions/formautofill/ProfileStorage.jsm:125 (Diff revision 4) > + let dupGuid = this._findMatchingProfile(profileToSave); > + > + if (dupGuid) { > + this.update(dupGuid, profileToSave); > + } else { I'm not sure this should be done in the storage layer… it seems like consumers should know whether they want to add or update and not automatically have an add turn into an update. What do you think?

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review140778 ::: browser/extensions/formautofill/FormAutofillContent.jsm:351 (Diff revision 7) > + if (PrivateBrowsingUtils.isContentWindowPrivate(domWin)) { > + return; Please add `this.log.debug("Ignoring submission in a private window")` to make it easier to diagnose reports of not saving in a private window.

Comment on attachment 8863672 [details] Bug 990219 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/135476/#review140752 ::: browser/extensions/formautofill/test/browser/browser.ini:6 (Diff revision 3) > [DEFAULT] > +support-files = > + test_basic_form.html > > [browser_check_installed.js] > +[browser_form_submitting.js] Nit: browser_form_submission.js ::: browser/extensions/formautofill/test/browser/browser_form_submitting.js:16 (Diff revision 3) > + resolve(result.data); > + }); > + }); > +} > + > +function formAutoFillCommonSetup() { Please use add_task for this setup ::: browser/extensions/formautofill/test/browser/browser_form_submitting.js:17 (Diff revision 3) > + SpecialPowers.setBoolPref("dom.forms.autocomplete.experimental", true); > + > + SimpleTest.registerCleanupFunction(() => { > + SpecialPowers.clearUserPref("dom.forms.autocomplete.experimental"); You can use `yield SpecialPowers.pushPrefEnv` to have it automatically cleanup at the end. ::: browser/extensions/formautofill/test/browser/browser_form_submitting.js:28 (Diff revision 3) > + > +formAutoFillCommonSetup(); > + > +add_task(function* check_storage_in_private_mode() { > + let privateWin = yield BrowserTestUtils.openNewBrowserWindow({private: true}); > + let privateBrowser = privateWin.getBrowser().selectedBrowser; I've never hear of getBrowser() before… I would normally use `.gBrowser` ::: browser/extensions/formautofill/test/browser/browser_form_submitting.js:44 (Diff revision 3) > +add_task(function* check_storage_in_normal_window() { > + let win = yield BrowserTestUtils.openNewBrowserWindow(); > + let browser = win.getBrowser().selectedBrowser; > + let profiles = yield getAllProfiles(); > + > + is(profiles.length, 0, "No profile in storage"); > + > + BrowserTestUtils.loadURI(browser, FORM_URL); > + > + yield TestUtils.topicObserved("formautofill-storage-changed"); > + profiles = yield getAllProfiles(); > + is(profiles.length, 1, "1 Profile saved"); > + > + yield BrowserTestUtils.closeWindow(win); > +}); I don't think we need this test duplicated in mochitest-browser-chrome since it's covered by mochitest-plain. It's best in mochitest-plain since it will also work on Android in the future. Since Private Tabs/Windows is application-specific, it's fine to have the PB test in m-bc for desktop-only. I would then rename this test to make it clear it's only for testing submission in private windows. ::: browser/extensions/formautofill/test/browser/test_basic_form.html:14 (Diff revision 3) > +function load() { > + document.getElementById("organization").value = "Mozilla"; > + document.getElementById("street-address").value = "331 E. Evelyn Avenue"; > + document.getElementById("tel").value = "1-650-903-0800"; > + > + setTimeout(() => { > + document.querySelector("input[type=submit]").click(); > + }, 0); > +} Can you use browser/extensions/formautofill/test/fixtures/autocomplete_basic.html (which is made to be shared between harnesses) instead of making a new file? Then you can use ContentTask.spawn to inject this code ::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js:61 (Diff revision 3) > +addMessageListener("FormAutofillTest:GetProfiles", () => { > + Services.cpmm.addMessageListener("FormAutofill:Profiles", function getResult(result) { This will need rebasing now for the profile=>address rename… sorry ::: browser/extensions/formautofill/test/mochitest/mochitest.ini:8 (Diff revision 3) > [test_basic_autocomplete_form.html] > - > +[test_submitting_profile.html] Can you move the mochitest-plain changes to their own commit please?

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. I didn't fully test this patch yet and the test needs to be rewrote, so it's WIP for the idea about adding a new "merge" method for storage. The reason I kept it in the storage is because some private method/const like normalize/valid fields are required. It will merge the given address into the address with specific guid, and throw error if it's not mergeable.

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review142408 ::: browser/extensions/formautofill/ProfileStorage.jsm:440 (Diff revision 6) > + * @param {Address} targetAddress > + * The address for merge. > + * @returns {boolean} > + * Return true if the target address is mergeable or false if not. > + */ > + _mergeToSrorage(targetAddress) { typo `mergeToStorage`

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review142936 ::: browser/extensions/formautofill/test/unit/test_transformFields.js:114 (Diff revision 7) > description: "Has both \"name\" and split names", > address: { > "name": "John Doe", > "given-name": "Timothy", > "additional-name": "John", > - "family-name": "Berners-Lee", > + "family-name": "Berners", I changed one of the fields because the merge will affect number of addresses and break the test logic. It maybe safer if we can test normalizeRecord method directly, just my 2 cents.

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review143730 ::: browser/extensions/formautofill/ProfileStorage.jsm:282 (Diff revision 8) > + * @param {string} guid > + * Indicates which record to merge. > + * @param {Object} record > + * The new record used to merge into the old one. > + */ > + merge(guid, record) { I would prefer to rename this method to something like `mergeIfPossible` and return a boolean instead of throw an error which happens in most cases. ::: browser/extensions/formautofill/ProfileStorage.jsm:304 (Diff revision 8) > + hasMatchingField = true; > + } > + } > + > + if (!hasMatchingField) { > + throw new Error("No matching field"); This error happens when two records are identical. Is it correct? ::: browser/extensions/formautofill/ProfileStorage.jsm:308 (Diff revision 8) > + if (this.VALID_FIELDS.indexOf(field) == -1) { > + continue; > + } > + recordFound[field] = recordToMerge[field]; nit: ```js if (this.VALID_FIELDS.includes(field)) { recordFound[field] = recordToMerge[field]; } ```

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review143730 > This error happens when two records are identical. Is it correct? It's for the 2 records have no overlap. We should only merge the record if they have at least 1 field that has same value. I add more comment about this part.

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review143920 Looks good. Thanks. ::: browser/extensions/formautofill/ProfileStorage.jsm:299 (Diff revision 9) > + let hasMatchingField = false; > + > + for (let field of this.VALID_FIELDS) { > + if (recordToMerge[field] !== undefined && recordFound[field] !== undefined) { > + if (recordToMerge[field] != recordFound[field]) { > + this.log.debug("Conflicts: field ", field, "has different value."); nit: missing one space before "has". ::: browser/extensions/formautofill/test/unit/test_addressRecords.js:325 (Diff revision 9) > + let subset = profileStorage.addresses._clone(TEST_ADDRESS_1); > + delete subset.tel; > + do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[0].guid, subset), true); > + do_check_eq(profileStorage.addresses.getAll()[0].tel, TEST_ADDRESS_1.tel); > + > + // Merge a address has some overlap nit: Merge an address with partial overlaps.

Comment on attachment 8863672 [details] Bug 990219 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/135476/#review143930 ::: browser/extensions/formautofill/test/mochitest/test_address_submission.html:32 (Diff revision 8) > + organization: "Mozilla", > + "street-address": "331 E. Evelyn Avenue", > + tel: "1-650-903-0800", > +}]; > + > +// Autofill the address from dropdown menu. The comment needs an update. ::: browser/extensions/formautofill/test/mochitest/test_address_submission.html:38 (Diff revision 8) > +add_task(async function check_storage_after_form_submitted() { > + for (let key in TEST_ADDRESSES[0]) { > + setInput("#" + key, TEST_ADDRESSES[0][key]); > + } > + > + setTimeout(() => { Why do we need `setTimeout` here?

Comment on attachment 8867127 [details] Bug 990219 - Part 2: Add mochitest-browser-chrome for private browsing check. https://reviewboard.mozilla.org/r/138724/#review143974

Comment on attachment 8847056 [details] Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message. https://reviewboard.mozilla.org/r/120096/#review144460 ::: browser/extensions/formautofill/FormAutofillHandler.jsm:137 (Diff revision 9) > + /** > + * Return the profile that is converted from fieldDetails. > + * > + * @returns {Object} The new profile that convert from details with trimmed result. > + */ Please document that only non-empty fields are included (after trimming)

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review143920 > nit: missing one space before "has". Actually it's the opposite problem… the space after "field" should be deleted since spaces are inserted automatically.

Comment on attachment 8867127 [details] Bug 990219 - Part 2: Add mochitest-browser-chrome for private browsing check. https://reviewboard.mozilla.org/r/138724/#review144478 r+ with fixes ::: browser/extensions/formautofill/test/browser/browser_submission_in_private_mode.js:5 (Diff revision 5) > +function getAllAddresses() { > + Services.cpmm.sendAsyncMessage("FormAutofill:GetAddresses", {}); > + > + return new Promise(resolve => { > + Services.cpmm.addMessageListener("FormAutofill:Addresses", function getResult(result) { > + Services.cpmm.removeMessageListener("FormAutofill:Addresses", getResult); > + resolve(result.data); > + }); > + }); > +} Is there a reason for not using the storage JSM directly? You also already have `getAddresses` in scope from head.js which I think does the same thing. ::: browser/extensions/formautofill/test/browser/browser_submission_in_private_mode.js:16 (Diff revision 5) > +add_task(async function formAutoFillCommonSetup() { > + await SpecialPowers.pushPrefEnv({ > + "set": [ > + ["dom.forms.autocomplete.experimental", true], > + ], > + }); > +}); You don't need this anymore ::: browser/extensions/formautofill/test/browser/browser_submission_in_private_mode.js:24 (Diff revision 5) > + ["dom.forms.autocomplete.experimental", true], > + ], > + }); > +}); > + > +add_task(async function check_storage_in_private_mode() { This can just be be `test_add` since the file is already about PB. You should have another to ensure that we also don't update metadata on existing records in PB mode (if that's what we decided?). I guess it's fine to do that in another bug if you want. ::: browser/extensions/formautofill/test/browser/browser_submission_in_private_mode.js:31 (Diff revision 5) > + BrowserTestUtils.loadURI(privateBrowser, FORM_URL); > + await BrowserTestUtils.browserLoaded(privateBrowser); > + await ContentTask.spawn(privateBrowser, null, async function() { `BrowserTestUtils.withNewTab{{gBrowser: privateWin.gBrowser, …})` will handle the loading for you. It also avoids any intermittent issues related to the about:privatebrowsing load ::: browser/extensions/formautofill/test/browser/browser_submission_in_private_mode.js:39 (Diff revision 5) > + content.document.getElementById("organization").focus(); > + content.document.getElementById("organization").value = "Mozilla"; > + content.document.getElementById("street-address").value = "331 E. Evelyn Avenue"; > + content.document.getElementById("tel").value = "1-650-903-0800"; > + > + setTimeout(() => { Use SimpleTest.executeSoon instead. Is this because we need to wait for a change event or something? ::: browser/extensions/formautofill/test/browser/browser_submission_in_private_mode.js:44 (Diff revision 5) > + setTimeout(() => { > + content.document.querySelector("input[type=submit]").click(); > + }, 0); > + }); > + > + // Wait 1 sec to make sure the profile is not been saved Nit: s/is/has/ s/sec/second/

Comment on attachment 8863672 [details] Bug 990219 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/135476/#review144488 ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:43 (Diff revision 8) > + > +function onAddressChanged() { > + return new Promise(resolve => { > + formFillChromeScript.addMessageListener("formautofill-storage-changed", function onChanged(data) { > + formFillChromeScript.removeMessageListener("formautofill-storage-changed", onChanged); > + is(data.data, "add", "Receive add storage changed event"); I guess you'll want to make "add" an argument to onAddressChanged eventually. ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:111 (Diff revision 8) > +function checkAddresses(expectedAddresses) { > + return getAddresses().then((addresses => { > + is(addresses.length, expectedAddresses.length, "Number of address are matching"); > + for (let address of addresses) { > + for (let expectedAddress of expectedAddresses) { > + ok(areAddressesMatching(address, expectedAddress), "2 Addresses' content are matching"); > + } > + } > + })); > +} If you do the check in the parent process then you won't need to duplicate VALID_ADDRESS_FIELDS in this file as the parent process can use the original VALID_ADDRESS_FIELDS

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review144454 After folding this into part 1 (see below), I think you should split the content process changes from the storage changes. You can then land the content process changes (FormAutofillContent.jsm + FormAutofillHandler.jsm + test_onFormSubmitted.js) in a separate bug after addressing comments. ::: browser/extensions/formautofill/FormAutofillContent.jsm:322 (Diff revision 9) > - _onFormSubmit(handler) { > - let address = handler.createProfile(); > + _onFormSubmit(profile) { > + Services.cpmm.sendAsyncMessage("FormAutofill:OnFormSubmit", profile); > - > - if (Object.keys(address).length < AUTOFILL_FIELDS_THRESHOLD) { > - return; Please fold this whole commit into part 1 since it's not useful to land intermediate functions that are then replace in part 2. ::: browser/extensions/formautofill/FormAutofillContent.jsm:326 (Diff revision 9) > - } else { > - Services.cpmm.sendAsyncMessage("FormAutofill:SaveAddress", {address}); > - } > }, > > notify(formElement, domWin) { Can you add a JSDoc comment that defines that this method always returns True so form submission isn't canceled. ::: browser/extensions/formautofill/FormAutofillContent.jsm:330 (Diff revision 9) > > notify(formElement, domWin) { > this.log.debug("Notifying form early submission"); > > if (domWin && PrivateBrowsingUtils.isContentWindowPrivate(domWin)) { > - this.log.debug("Ignoring submission in a private window"); > + this.log.debug("Avoid any profile saving in private mode submission"); Nit: I think the old text was more clear ::: browser/extensions/formautofill/FormAutofillContent.jsm:342 (Diff revision 9) > - } else { > - this._onFormSubmit(handler); > + return true; > + } > + > + let pendingAddress = handler.createProfile(); > + if (Object.keys(pendingAddress).length < AUTOFILL_FIELDS_THRESHOLD) { > + this.log.debug("Avoid the address saving if the valid fields in under the threshold"); I think this can be more terse: Not saving since there are only $N usable fields ::: browser/extensions/formautofill/ProfileStorage.jsm:174 (Diff revision 9) > - this.log.debug("add:", record); > + // Try to merge to existing record before adding new record > + if (this._mergeToStorage(record)) { > + return; > + } I thought I said this somewhere before but maybe not… I don't like the idea of an add method magically doing something other than an add. It makes unit testing less accurate since you may not catch that an add turned into an update. There are only three consumers to call add (form submission, sync and prefs) so those consumers can handle merging if necessary. I'm fine with having a separate public method or an argument for an addOrUpdate behaviour though (I just don't think it should be the automatic default). ::: browser/extensions/formautofill/ProfileStorage.jsm:284 (Diff revision 9) > + mergeIfPossible(guid, record) { > + this.log.debug("merge:", guid, record); Nit: the method name doesn't match the logging ::: browser/extensions/formautofill/ProfileStorage.jsm:284 (Diff revision 9) > + * @param {Object} record > + * The new record used to merge into the old one. > + * @returns {boolean} > + * Return true if record is merged into target with specific guid or false if not. > + */ > + mergeIfPossible(guid, record) { With OOP this would probably be a method on the address record. I think it would make it easier to read compared to recordToMerge vs. recordFound naming… ::: browser/extensions/formautofill/ProfileStorage.jsm:318 (Diff revision 9) > + if (this.VALID_FIELDS.includes(field)) { > + recordFound[field] = recordToMerge[field]; > + } > + } > + > + recordFound.timeLastModified = Date.now(); timeLastUsed also needs to be updated if this was from a form submission ::: browser/extensions/formautofill/ProfileStorage.jsm:321 (Diff revision 9) > + } > + > + recordFound.timeLastModified = Date.now(); > + > + this._store.saveSoon(); > + Services.obs.notifyObservers(null, "formautofill-storage-changed", "merge"); Plase send the guid as the first argument so we can verify it in tests… we still need to do that for the others but that can be done later. ::: browser/extensions/formautofill/ProfileStorage.jsm:421 (Diff revision 9) > + * Merge the record if storage has a mergeable record. > + * @param {Object} targetRecord > + * The record for merge. > + * @returns {boolean} > + * Return true if the target record is mergeable or false if not. > + */ > + _mergeToStorage(targetRecord) { > + for (let record of this._store.data[this._collectionName]) { > + if (this.mergeIfPossible(record.guid, targetRecord)) { > + return true; > + } > + } I don't think this is the best UX… we should merge with the record that has the most fields the same, not the first one that has one field the same and no conflicts. If there are multiple with the same number of fields in-common, we could also prefer updating the one more recently used.

Comment on attachment 8858733 [details] Bug 990219 - Part 2: Add merge function in storage. https://reviewboard.mozilla.org/r/130768/#review144454 > I don't think this is the best UX… we should merge with the record that has the most fields the same, not the first one that has one field the same and no conflicts. If there are multiple with the same number of fields in-common, we could also prefer updating the one more recently used. yeah I agree that we should have a better strategy for merging and I planed to implement this in another bug since the timing for showing the update doorhanger won't be blocked by the merge strategy... Since we might leave entire merge to another bug, we could discuss more in the follow up(or maybe even 1 for merge timing and 1 for merge strategy).

Comment on attachment 8863672 [details] Bug 990219 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/135476/#review143930 > Why do we need `setTimeout` here? I think we don't need this for mochitest plain

Comment on attachment 8867127 [details] Bug 990219 - Part 2: Add mochitest-browser-chrome for private browsing check. https://reviewboard.mozilla.org/r/138724/#review144478 > This can just be be `test_add` since the file is already about PB. > > You should have another to ensure that we also don't update metadata on existing records in PB mode (if that's what we decided?). I guess it's fine to do that in another bug if you want. Yeah I guess we could add this in merge one if we decide to split it. > Use SimpleTest.executeSoon instead. Is this because we need to wait for a change event or something? I vaguely remember I saw the field is not set in browser test if I trigger submit immediately... But not I could not reproduce it:/

Comment on attachment 8869513 [details] Bug 990219 - Part 3: Disable the address saving until doorhanger is ready. https://reviewboard.mozilla.org/r/141114/#review144648 ::: browser/extensions/formautofill/FormAutofillParent.jsm:274 (Diff revision 1) > // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200) > // if (!profileStorage.addresses.mergeIfPossible(address.guid, address.record)) { > // } > } else { > // TODO: Add first time use probe(bug 990199) and doorhanger(bug 1303510) > - profileStorage.addresses.add(address.record); > + // profileStorage.addresses.add(address.record); I disable the address saving if you think we shouldn't save any information without notification. It won't breake the tests in part 1 and part 4, so maybe we can land 1,4 and 5 to create an entry for doorhanger and metrics.

Patch rebased and move the merge/mochitest to separate bugs.

Comment on attachment 8869513 [details] Bug 990219 - Part 3: Disable the address saving until doorhanger is ready. https://reviewboard.mozilla.org/r/141114/#review145792

Thanks for all the reviews!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ac22e8051ada Part 1: Add earlyformsubmit observer and dispatch saveProfile message. r=MattN https://hg.mozilla.org/integration/autoland/rev/51dd1439fb05 Part 2: Add mochitest-browser-chrome for private browsing check. r=lchang,MattN https://hg.mozilla.org/integration/autoland/rev/c640813b5c4e Part 3: Disable the address saving until doorhanger is ready. r=MattN

Backed out for browser_submission_in_private_mode.js leaks. https://hg.mozilla.org/integration/autoland/rev/e39afbf1ac02808334b2aba551c94591d9214244 https://treeherder.mozilla.org/logviewer.html#?job_id=101581997&repo=autoland

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cdeb21e99e3e Part 1: Add earlyformsubmit observer and dispatch saveProfile message. r=MattN https://hg.mozilla.org/integration/autoland/rev/d58a5e1b798d Part 2: Add mochitest-browser-chrome for private browsing check. r=lchang,MattN https://hg.mozilla.org/integration/autoland/rev/2d5c0e380b22 Part 3: Disable the address saving until doorhanger is ready. r=MattN

https://hg.mozilla.org/mozilla-central/rev/cdeb21e99e3e https://hg.mozilla.org/mozilla-central/rev/d58a5e1b798d https://hg.mozilla.org/mozilla-central/rev/2d5c0e380b22