There will be a list of contact profiles (no credit card ones for now) with the ability to open the view/edit page for it. The ability to add/delete profiles will be implemented separately.

Just confirmed with UX that the list dialog would neither has multiple columns nor has requirement for sorting by column. So we'll implement it as new HTML dialog instead of reusing existing xul dialog.

It's just a quick experiment that modify the existing dialogs to simulate the nested dialog behavior. The method here is calling gSubDialog.open with another url for showing 2nd dialog. Since UX might have some concern about dialog switching, please reference the attached clip for the flickering issue.

It's another solution that we insert the second dialog in the same xul as a form element, and control 2 dialog forms' display by CSS. It could avoid the flickering in the 1st attachment. gSubDialog might need to expose 1 more additional API for dialog resizing, because the content size will not affect the outer frame's size. And the close button in the caption bar will need more logic for handling back/close action. Juwei proposed that maybe we can simply hide the close button, I think it might be simpler. Hi Matt, please let me know if you have any concern about this approach.

Comment on attachment 8806645 [details] clip2.ogv It looks great to me! Steve u are the hero!

Hi Steve, I think clip2 looks good though I'm not sure it would look as good if the 2nd dialog wasn't larger than the first. Since you're test 2nd dialog is larger it's hiding the fact that we're not actually stacking windows. If we're fine with forcing our add/edit dialog to be larger than the list dialog then this could work. I'm not sure how you're implementing this though. It sounds like you're toggling visibility but is that any better than doing a window.open("edit_dialog.xhtml", "_self") in the inner frame? Also, the harder part is handling the close button in the subdialog header. Perhaps an unload handler could be used for that?

A few known issues: - When there is no <option> in <select> element, a long vertical focus ring would appear. - Highlight color on option is different from what xul menuitem uses. Tests coming as part 2.

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review122258 I still need to take a closer look at the CSS and manageProfiles.js but I think I covered enough for this pass as some things will change. ::: browser/extensions/formautofill/FormAutofillParent.jsm (Diff revision 1) > - if (this._enabled) { > - Services.ppmm.addMessageListener("FormAutofill:GetProfiles", this); Can you double-check with Steve that it's okay to always listen for this now? ::: browser/extensions/formautofill/FormAutofillPreferences.jsm:131 (Diff revision 1) > + new target.ownerGlobal.CustomEvent("OpenDialog", { > + detail: { > + URL: MANAGE_PROFILES_URL, > + params: { > + ownerDocument: target.ownerDocument, > + }, Why aren't you calling `target.ownerGlobal.gSubDialog.open` directly? ::: browser/extensions/formautofill/content/manageProfiles.css:30 (Diff revision 1) > + background-color: var(--in-content-box-background-hover); > + border: 1px solid var(--in-content-box-border-color); > + border-radius: 2px 2px 0 0; > +} > + > +fieldset > select[size] { Is this something that would be useful outside of this dialog or for more than one `select`? If not, why not use `#profiles`? ::: browser/extensions/formautofill/content/manageProfiles.css:38 (Diff revision 1) > + height: 16.6em; > + border-top: none; > + border-radius: 0 0 2px 2px; > +} > + > +select[size] > option { Same here. Why not `#profiles > option`? ::: browser/extensions/formautofill/content/manageProfiles.js:18 (Diff revision 1) > + > +this.log = null; > +FormAutofillUtils.defineLazyLogGetter(this, "manageProfiles"); > + > +function ManageProfileDialog(params) { > + this.ownerDocument = params.ownerDocument; Can you define this in the prototype object below (keep the assignment here) and explain what it's for. Ignore this if we can use gSubDialog directly. ::: browser/extensions/formautofill/content/manageProfiles.js:86 (Diff revision 1) > + renderProfileElements(profiles) { > + this.clearProfileElements(); > + profiles.forEach(profile => { This approach will lose the selected option(s) when storage changes. You should probably persist the selected options before re-rendering and re-select them or don't clear and instead selectively add/remove only as necessary. I wonder if we should have tried using React here… ::: browser/extensions/formautofill/content/manageProfiles.js:156 (Diff revision 1) > + openEditDialog(profile) { > + this.ownerDocument.dispatchEvent(new CustomEvent("OpenDialog", { I thought we were going to change subdialogs.js to support multiple stacked dialogs. If this is just for the short term then I think we can just do a window.open(Dialog) for the short term until subdialogs gain that support rather than adding a temporary ownerDocument idea. ::: browser/extensions/formautofill/content/manageProfiles.js:171 (Diff revision 1) > + * Enable/disable the Edit and Remove buttons based on selected index. > + * > + * @param {number} index > + */ > + updateButtonsStates(index) { > + log.debug(index); Nit: add some text before the index e.g. "updateButtonsStates" ::: toolkit/themes/shared/in-content/common.inc.css:199 (Diff revision 1) > border-radius: 2px; > background-color: var(--in-content-page-background); > } > > html|button:enabled:hover, > -html|select:enabled:hover, > +html|select:not([size]):enabled:hover, I think you also want `:not([size][multiple])` ::: toolkit/themes/shared/in-content/common.inc.css:720 (Diff revision 1) > border-inline-start: none; > } > > /* List boxes */ > > +html|select[size], Should these be `html|select[size][multiple]`? Consider size="0" or size="1".

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review122258 Thanks Matt, I've made some changes as suggested, and I'm leaving the issues of which I have questions open. I also noticed that I didn't implement the ability to remove multiple profiles at once.. so that's also included in this revision. I've added a new method in ProfileStorage that accepts an array of guids and remove all at once. I considered changing the .remove method to support both an array and a string as argument, do you think that's a better way? > Can you double-check with Steve that it's okay to always listen for this now? Yes I talked to Steve about this change and he's okay with it. > Why aren't you calling `target.ownerGlobal.gSubDialog.open` directly? Oh yes I should! It'd make it much simpler.. > Is this something that would be useful outside of this dialog or for more than one `select`? If not, why not use `#profiles`? Yeah I can't be sure this is a robust reimplementation for menulist yet. So for now I think I'll just use #profiles as the selector. > Can you define this in the prototype object below (keep the assignment here) and explain what it's for. Ignore this if we can use gSubDialog directly. Ok. I'll move it to the prototype object. I need the reference to the preferences window object in order to get to gSubDialog. > This approach will lose the selected option(s) when storage changes. You should probably persist the selected options before re-rendering and re-select them or don't clear and instead selectively add/remove only as necessary. I wonder if we should have tried using React here… I've taken a very naive approach to updating the elements (kill everything and add everything). It's true that React would be able to keep track of the states more elegantly but I wonder if it's worth introducing a dependency for something this small. I could implement something simple by saving the selected options before clearing them, and setting selected states back to the new elements. Probably not as performant than reusing the DOM nodes but saves the need for diffing each profile object. > I thought we were going to change subdialogs.js to support multiple stacked dialogs. If this is just for the short term then I think we can just do a window.open(Dialog) for the short term until subdialogs gain that support rather than adding a temporary ownerDocument idea. Current subdialogs implementation closes the existing window before opening a new one. It works now except with the annoying flash from the overlay disappearing and reappearing. I'm going to change subdialogs.js to allow stacking (hopefully w/o too many surprises), and I expect this is how gSubDialog will be used. > Nit: add some text before the index e.g. "updateButtonsStates" Will do. > I think you also want `:not([size][multiple])` In this case we could do `[size][multiple]` but I don't think `[multiple]` should be a required attribute for this style. I could imagine other menulist w/o multi-select capability also use this style. > Should these be `html|select[size][multiple]`? Consider size="0" or size="1". I wonder if it's reasonable to assume that when a select has `[size]` present, it is trying to be a flat select element rather than a dropdown one? With `[multiple]` it'd be clear, but I don't think people always want multi-select capability.

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review122734 ::: browser/extensions/formautofill/ProfileStorage.jsm:223 (Diff revision 2) > + this._store.data.profiles = > + this._store.data.profiles.filter(profile => { > + return guids.indexOf(profile.guid) == -1; > + }); > + this._store.saveSoon(); > + Services.obs.notifyObservers(null, "formautofill-storage-changed", "remove"); Consumers (e.g. Sync) may want to know what was removed so we will probably need to include the GUID eventually. Because of that I think we may want to just call the existing `remove` for each profile instead of implementing this new method. You can still have the message support multiple at once but have the receiver loop over guids

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review122734 > Consumers (e.g. Sync) may want to know what was removed so we will probably need to include the GUID eventually. Because of that I think we may want to just call the existing `remove` for each profile instead of implementing this new method. You can still have the message support multiple at once but have the receiver loop over guids Ok I've removed the method and use the existing `remove` instead. However I ended up having to track the number of "formautofill-storage-changed" messages so that it only reloads profiles once.

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review123174 ::: browser/extensions/formautofill/FormAutofillParent.jsm:134 (Diff revision 3) > } > } > }, > > /** > - * Add/remove message listener and broadcast the status to frames while the > + * Broadcast the status to frames while the form autofill status changed. s/while/when/ s/changed/changes/ ::: browser/extensions/formautofill/FormAutofillPreferences.jsm:131 (Diff revision 3) > if (target == this.refs.enabledCheckbox) { > // Set preference directly instead of relying on <Preference> > Services.prefs.setBoolPref(PREF_AUTOFILL_ENABLED, target.checked); > } else if (target == this.refs.savedProfilesBtn) { > - // TODO: Open Saved Profiles dialog > + target.ownerGlobal.gSubDialog.open(MANAGE_PROFILES_URL, null, > + target.ownerGlobal); I think we should only use the first argument for now. ::: browser/extensions/formautofill/content/manageProfiles.css:24 (Diff revision 3) > + color: #808080; > + background-color: var(--in-content-box-background-hover); Is there no variable for the foreground color? Is this value from a spec or matching another piece of preference UI? ::: browser/extensions/formautofill/content/manageProfiles.js:48 (Diff revision 3) > + get _selectedOptions() { > + return Array.from(this.refs.profiles.selectedOptions); > + }, > + > + init() { > + this._profiles = []; This should be defined on the prototype. All member variables should be defined (not necessarily initialied) on the prototype so that it's easy to see what variables exist on an object. I wonder if it would have been less fragile to have each profile object as an expando property on the <option> instead… ::: browser/extensions/formautofill/content/manageProfiles.js:63 (Diff revision 3) > + > + uninit() { > + log.debug("uninit"); > + this.detachEventListeners(); > + this._profiles = null; > + this.refs = null; This should also be defined on the prototype and should be private. I'm also still not getting used to the name `refs`. `elements` is more clear to me. ::: browser/extensions/formautofill/content/manageProfiles.js:80 (Diff revision 3) > + this.renderProfileElements(profiles); > + }); > + }, > + > + /** > + * Get profiles from ProfileStorage.jsm The name of the JSM is an implementation detail… just say "from storage" ::: browser/extensions/formautofill/content/manageProfiles.js:104 (Diff revision 3) > + let option = document.createElement("option"); > + option.value = profile.guid; > + option.text = this.getProfileLabel(profile); > + if (selectedGuids.indexOf(profile.guid) > -1) { > + option.selected = true; > + } Btw. you could use the `Option` constructor to reduce this to one line: https://developer.mozilla.org/en-US/docs/Web/API/HTMLOptionElement/Option ::: browser/extensions/formautofill/content/manageProfiles.js:107 (Diff revision 3) > + this.clearProfileElements(); > + profiles.forEach(profile => { > + let option = document.createElement("option"); > + option.value = profile.guid; > + option.text = this.getProfileLabel(profile); > + if (selectedGuids.indexOf(profile.guid) > -1) { You should use `.includes` in new code as it's more readable. ::: browser/extensions/formautofill/content/manageProfiles.js:133 (Diff revision 3) > + Services.cpmm.sendAsyncMessage("FormAutofill:RemoveProfiles", > + {guids}); Nit: No need to wrap here ::: browser/extensions/formautofill/content/manageProfiles.js:144 (Diff revision 3) > + getProfileLabel(profile) { > + // TODO: Implement a smarter way for deciding what to display > + // as option text. Possibly improve the algorithm in > + // ProfileAutoCompleteResult.jsm and reuse it here. > + const fieldOrder = [ Yeah, it seems like this should be shared in the utils JSM but I guess it's fine to wait until we add names here. ::: browser/extensions/formautofill/content/manageProfiles.js:178 (Diff revision 3) > + * Open the edit profile dialog to create/edit a profile. > + * > + * @param {object} profile [optional] > + */ > + openEditDialog(profile) { > + this.prefWin.gSubDialog.open(EDIT_PROFILE_URL, null, profile); I still think we should use window.open[Dialog] in the short term. ::: browser/extensions/formautofill/content/manageProfiles.js:250 (Diff revision 3) > + this.loadProfiles().then(() => { > + this.updateButtonsStates(this._selectedOptions.length); It seems like `updateButtonsStates` should be called at the end of `loadProfiles` in case the default markup attributes are out of sync with the default selection. i.e. I think it would be less fragile that way.

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review122258 > Ok. I'll move it to the prototype object. I need the reference to the preferences window object in order to get to gSubDialog. I don't think we should implement calling gSubDialog.open from within the list until we have proper subdialog support as I think people will find it confusing and complain and thus I wouldn't want to promote the feature on Nightly. I think for now we don't need this and can use a regular window.open[Dialog] as it's a better UX for now. > Current subdialogs implementation closes the existing window before opening a new one. It works now except with the annoying flash from the overlay disappearing and reappearing. I'm going to change subdialogs.js to allow stacking (hopefully w/o too many surprises), and I expect this is how gSubDialog will be used. But closing the add/edit dialog doesn't go back to the list view, right? I think that's a problem for M1. > I wonder if it's reasonable to assume that when a select has `[size]` present, it is trying to be a flat select element rather than a dropdown one? With `[multiple]` it'd be clear, but I don't think people always want multi-select capability. Our user-agent stylesheets seem to special-case size=0 and size=1 so I think we should be consistent with how it works.

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review123174 > Is there no variable for the foreground color? Is this value from a spec or matching another piece of preference UI? Yeah I'm trying to match the treecol style but it's not referencing any variable: http://searchfox.org/mozilla-central/rev/c2a60adfc7b16761cbbfcefa2093fa402ba1aa69/toolkit/themes/shared/in-content/common.inc.css#776 > This should be defined on the prototype. All member variables should be defined (not necessarily initialied) on the prototype so that it's easy to see what variables exist on an object. > > I wonder if it would have been less fragile to have each profile object as an expando property on the <option> instead… My initial thought was to keep the profiles data in js so we could do diffing and make minimal changes to the DOM. But since we are not doing that now, I'd say yes it's a good idea to just put profile object as an expando property. > Btw. you could use the `Option` constructor to reduce this to one line: https://developer.mozilla.org/en-US/docs/Web/API/HTMLOptionElement/Option Thanks! Didn't know about the constructor usage. Looks cleaner now. > I still think we should use window.open[Dialog] in the short term. Ok I changed it to use window.openDialog and opening it as a modal to keep it on top. Wonder if this is what you have in mind? > It seems like `updateButtonsStates` should be called at the end of `loadProfiles` in case the default markup attributes are out of sync with the default selection. i.e. I think it would be less fragile that way. I don't fully understand what you mean by default markup attribues out of sync with the default selection. But I agree moving it is reasonable considering we almost always want to do updateButtonsStates after (re)loading profiles.

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review126610 ::: browser/extensions/formautofill/content/manageProfiles.css:43 (Diff revision 5) > + border-top: none; > + border-radius: 0 0 2px 2px; > +} > + > +#profiles > option { > + padding-left: 0.7em; Should this be padding-inline-start to handle RTL? You can test RTL with the ForceRTL extension on AMO. ::: browser/extensions/formautofill/content/manageProfiles.js:24 (Diff revision 5) > +} > + > +ManageProfileDialog.prototype = { > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, Ci.nsIObserver]), > + > + _elements: {}, I think I didn't say anything on the previous reviews but we generally try to avoid using objects as maps now that we have a `Map` type. The Map syntax is more verbose though… I think it's okay to be consistent with the other code. ::: browser/extensions/formautofill/content/manageProfiles.js:79 (Diff revision 5) > + Services.cpmm.addMessageListener("FormAutofill:Profiles", function getResult(result) { > + Services.cpmm.removeMessageListener("FormAutofill:Profiles", getResult); > + resolve(result.data); > + }); > + Services.cpmm.sendAsyncMessage("FormAutofill:GetProfiles", {}); I wonder if there will be issues with issues with races if multiple pieces of code send the same async message with different filters… Perhaps we need to assign GUIDs to messages and have the response send the same GUID back to that we can filter out messages of the same name for another consumer. Can you file a bug on this? I think it will turn into a problem through our code now that we're using cpmm and ppmm. ::: browser/extensions/formautofill/content/manageProfiles.js:96 (Diff revision 5) > + profiles.forEach(profile => { > + let option = new Option(this.getProfileLabel(profile), Nit: for…of it preferred over .forEach for cases where you don't need a new function scope since it's more natural for people used to `for` loops. ::: browser/extensions/formautofill/content/manageProfiles.js:124 (Diff revision 5) > + * ignore before loading profiles. > + * > + * @param {array<string>} guids > + */ > + removeProfiles(guids) { > + this._pendingChangeCount = guids.length - 1; What if `_pendingChangeCount` is already greater than 0? Should you use `+=` instead? I'm kinda worried that `_pendingChangeCount` we become out of sync and thought about time-batching instead with DeferredTask but that will make things seem less responsive. Now that I think about it more, ideally we wouldn't even wait for storage to update and instead would remove the <option>s synchronously. I think we should have the "formautofill-storage-changed" send the guid as the `subject` as an `nsISupportsString` and then in this code, if `data` indicates it's for deletion we don't use `renderProfileElements` and instead remove the <option>. This would get rid of the need for `_pendingChangeCount`. We should remove the option immediately in `removeProfiles` and when we get the async notification to delete we could gracefully handle them already being deleted. I'm fine with this done in a high priority follow-up if you'd like. ::: browser/extensions/formautofill/content/manageProfiles.js:169 (Diff revision 5) > + window.openDialog(EDIT_PROFILE_URL, null, "modal,width=600,height=355", > + profile); window.openDialog is non-standard and I thought we were removing it. Did you try window.open instead? You can specify "modal,dialog,…" I think to get the same behaviour.

Comment on attachment 8850462 [details] Bug 1019483 - (Part 2) Add mochitest-browser-chrome tests for manage profiles dialog. https://reviewboard.mozilla.org/r/123066/#review126614 ::: browser/extensions/formautofill/test/browser/browser_manageProfilesDialog.js:34 (Diff revision 1) > +function waitForStorageChange() { > + return new Promise(resolve => { You can use `TestUtils.topicObserved` instead. It should be imported already. ::: browser/extensions/formautofill/test/browser/browser_manageProfilesDialog.js:58 (Diff revision 1) > +function waitForProfiles() { > + return new Promise(resolve => { > + Services.cpmm.addMessageListener("FormAutofill:Profiles", function getResult(result) { > + Services.cpmm.removeMessageListener("FormAutofill:Profiles", getResult); > + // Wait for the next tick for elements to get rendered. > + setTimeout(resolve.bind(null, result.data), 0); Is `SimpleTest.executeSoon(resolve);` suffcient? It's preferred over `setTimeout(…, 0)` for tests. ::: browser/extensions/formautofill/test/browser/browser_manageProfilesDialog.js:64 (Diff revision 1) > + }); > + }); > +} > + > +add_task(function* test_manageProfilesInitialState() { > + let win = window.openDialog(MANAGE_PROFILES_DIALOG_URL); Why not use `yield BrowserTestUtils.withNewTab` and load it in a tab? ::: browser/extensions/formautofill/test/browser/browser_manageProfilesDialog.js:80 (Diff revision 1) > + is(btnEdit.disabled, true, "Edit button disabled"); > + win.close(); > +}); > + > +add_task(function* test_removingSingleAndMultipleProfiles() { > + yield saveProfile(TEST_PROFILE_1); You should use `registerCleanupFunction` in `test_manageProfilesInitialState` or a head.js file in the directory that cleans up any leftover profiles between tests.

This will need to remove the exception at http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/test/static/browser_all_files_referenced.js#70

Comment on attachment 8850462 [details] Bug 1019483 - (Part 2) Add mochitest-browser-chrome tests for manage profiles dialog. https://reviewboard.mozilla.org/r/123066/#review126614 > You can use `TestUtils.topicObserved` instead. It should be imported already. Didn't about this util, thanks! > Is `SimpleTest.executeSoon(resolve);` suffcient? It's preferred over `setTimeout(…, 0)` for tests. Thanks! > Why not use `yield BrowserTestUtils.withNewTab` and load it in a tab? Yes I can use `yield BrowserTestUtils.withNewTab` here. Though I haven't been able to make it work for `test_removingSingleAndMultipleProfiles` because I was trying to use EventUtils to simulate clicking on an item. Is there way to do that in ContentTask? > You should use `registerCleanupFunction` in `test_manageProfilesInitialState` or a head.js file in the directory that cleans up any leftover profiles between tests. Will do.

Comment on attachment 8847143 [details] Bug 1019483 - (Part 1) Create interface to manage autofill profiles. https://reviewboard.mozilla.org/r/120164/#review126610 > Should this be padding-inline-start to handle RTL? You can test RTL with the ForceRTL extension on AMO. Yes I should change it to inline-start. But it appears the xhtml page doesn't get RTL treatment automatically, but other subdialogs with xul content does.. Seems like -moz-locale-dir(rtl) does not work for HTML pages: https://developer.mozilla.org/zh-TW/docs/Web/CSS/:-moz-locale-dir(rtl). I'll file a bug for this. > I think I didn't say anything on the previous reviews but we generally try to avoid using objects as maps now that we have a `Map` type. The Map syntax is more verbose though… I think it's okay to be consistent with the other code. Ok I'll keep this in mind. > I wonder if there will be issues with issues with races if multiple pieces of code send the same async message with different filters… Perhaps we need to assign GUIDs to messages and have the response send the same GUID back to that we can filter out messages of the same name for another consumer. > > Can you file a bug on this? I think it will turn into a problem through our code now that we're using cpmm and ppmm. Ok! > What if `_pendingChangeCount` is already greater than 0? Should you use `+=` instead? I'm kinda worried that `_pendingChangeCount` we become out of sync and thought about time-batching instead with DeferredTask but that will make things seem less responsive. > > Now that I think about it more, ideally we wouldn't even wait for storage to update and instead would remove the <option>s synchronously. I think we should have the "formautofill-storage-changed" send the guid as the `subject` as an `nsISupportsString` and then in this code, if `data` indicates it's for deletion we don't use `renderProfileElements` and instead remove the <option>. This would get rid of the need for `_pendingChangeCount`. We should remove the option immediately in `removeProfiles` and when we get the async notification to delete we could gracefully handle them already being deleted. I'm fine with this done in a high priority follow-up if you'd like. Ok I'll change it to using += `_pendingChangeCount` first and fill a bug to add/remove profiles based on the subject from the `formautofill-storage-changed` notification. > window.openDialog is non-standard and I thought we were removing it. Did you try window.open instead? You can specify "modal,dialog,…" I think to get the same behaviour. `window.open` doesn't have the parameter for arguments though. I would need to JSON.stringify the profile and append it to URL. But since this dialog is only temporary until gSubDialog can open multiple dialogs, maybe we can stick with this for now?

Comment on attachment 8850462 [details] Bug 1019483 - (Part 2) Add mochitest-browser-chrome tests for manage profiles dialog. https://reviewboard.mozilla.org/r/123066/#review127056 ::: browser/extensions/formautofill/test/browser/browser_manageProfilesDialog.js:65 (Diff revision 2) > +}); > + > +add_task(function* test_manageProfilesInitialState() { > + yield BrowserTestUtils.withNewTab({gBrowser, url: MANAGE_PROFILES_DIALOG_URL}, function* (browser) { > + yield ContentTask.spawn(browser, TEST_SELECTORS, (args) => { > + let selProfiles = content.document.querySelector(args.selProfiles); I think `browser.contentDocument.querySelector(args.selProfiles);` should work without ContentTask but please check what eslint says.

Comment on attachment 8850462 [details] Bug 1019483 - (Part 2) Add mochitest-browser-chrome tests for manage profiles dialog. https://reviewboard.mozilla.org/r/123066/#review127056 > I think `browser.contentDocument.querySelector(args.selProfiles);` should work without ContentTask but please check what eslint says. It works, but I'm getting test failures on linux on try.. but I couldn't reproduce the error on an actual ubuntu machine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b5b53cb8cd3d1cb06a20f1cc3b3f19952b7d2d3 On top of that there's the CPOW error which you mentioned should be a false positive. I could add a rule in eslint to make it a warning to get around this, but failure on linux is the biggest problem now. Do you think we can just land the tests without using `BrowserTestUtils.withNewTab`? It doesn't seem to work on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e97f6bc80cc83d591e23adeb73a70acd20680c

Comment on attachment 8850462 [details] Bug 1019483 - (Part 2) Add mochitest-browser-chrome tests for manage profiles dialog. https://reviewboard.mozilla.org/r/123066/#review127056 > It works, but I'm getting test failures on linux on try.. but I couldn't reproduce the error on an actual ubuntu machine: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b5b53cb8cd3d1cb06a20f1cc3b3f19952b7d2d3 > > On top of that there's the CPOW error which you mentioned should be a false positive. I could add a rule in eslint to make it a warning to get around this, but failure on linux is the biggest problem now. > > Do you think we can just land the tests without using `BrowserTestUtils.withNewTab`? It doesn't seem to work on try: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e97f6bc80cc83d591e23adeb73a70acd20680c On try for linux the selProfiles is not getting selected for some reason. Could CPOW be the issue? https://hg.mozilla.org/try/rev/8fa10c2e406cea8992bd8d11367303c7d0526f10#l2.90

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ffbf6849c41f (Part 1) Create interface to manage autofill profiles. r=MattN https://hg.mozilla.org/integration/autoland/rev/f3ccca098f5e (Part 2) Add mochitest-browser-chrome tests for manage profiles dialog. r=MattN

Backed out for failing browser/base/content/test/static/browser_parsable_css.js: https://hg.mozilla.org/integration/autoland/rev/d8c6a1a1db8f7fad0c9ce1a5607bb93896c82d9a https://hg.mozilla.org/integration/autoland/rev/219606cc6a863a69e6ad90d90231f09c2be433d3 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f3ccca098f5ef8078d008b806190f670b5146552&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=87791746&repo=autoland 00:32:06 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for chrome://global/skin/in-content/common.css: Missing closing ‘)’ in negation pseudo-class ‘[’. Ruleset ignored due to bad selector. - 00:32:06 INFO - Stack trace: 00:32:06 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:162 00:32:06 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:320 00:32:06 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:735:9 00:32:06 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7 00:32:06 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

Sorry about the test failure. I forgot to run the CSS test. I've updated the patch and the errors should be gone now. I'm running another try myself first before pushing again. Thanks.

Looks fine now aside from a few intermittent failures from other modules: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf9607d2aec5

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41b52c8ad6ee (Part 1) Create interface to manage autofill profiles. r=MattN https://hg.mozilla.org/integration/autoland/rev/0d6ffde320ec (Part 2) Add mochitest-browser-chrome tests for manage profiles dialog. r=MattN

https://hg.mozilla.org/mozilla-central/rev/41b52c8ad6ee https://hg.mozilla.org/mozilla-central/rev/0d6ffde320ec