A generic loadURL API will probably be more scalable that a specific (privacy) preferences one as we already could have used it for "showFirefoxAccounts".

If we use a generic API we should have a whitelist. For about:preferences we regularly use switchToTab to prevent multiple.

Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs

https://reviewboard.mozilla.org/r/12675/#review11163 ::: browser/components/uitour/test/browser.ini:23 (Diff revision 1) > +[browser_UITour_defaultBrowser.js] > +skip-if = e10s # Bug 1073247 - UITour.jsm not e10s friendly > +[browser_UITour_detach_tab.js] > +skip-if = e10s # Bug 1073247 - UITour.jsm not e10s friendly I'm just fixing some tests that weren't in alphabetical order. ::: browser/components/uitour/content-UITour.js:88 (Diff revision 1) > sendPageEvent: function (type, detail) { > + if (!this.ensureTrustedOrigin()) { > + return; This is an unrelated change that I noticed while looking at the code.

Comment on attachment 8630075 [details] MozReview Request: Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs https://reviewboard.mozilla.org/r/12675/#review11275 ::: browser/components/uitour/test/browser_openPreferences.js:18 (Diff revision 1) > + let promiseTabOpen = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen"); > + > + gContentAPI.openPreferences(); > + yield promiseTabOpen; > + > + yield BrowserTestUtils.waitForEvent(gBrowser.selectedBrowser, "load", true); This races. ::: browser/components/uitour/test/browser_openPreferences.js:33 (Diff revision 1) > + let promiseTabOpen = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen"); > + > + gContentAPI.openPreferences("privacy"); > + yield promiseTabOpen; > + > + yield BrowserTestUtils.waitForEvent(gBrowser.selectedBrowser, "load", true); And this. ::: browser/components/uitour/UITour.jsm:623 (Diff revision 1) > + if (typeof data.pane != "string" && typeof data.pane != "undefined") { > + log.warn("openPreferences: Invalid pane specified"); > + return false; > + } There seems to be no test for the failure case?

Comment on attachment 8630075 [details] MozReview Request: Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs

Comment on attachment 8630075 [details] MozReview Request: Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs https://reviewboard.mozilla.org/r/12675/#review11367 Ship It! ::: browser/components/uitour/test/browser_openPreferences.js:26 (Diff revisions 1 - 2) > - yield BrowserTestUtils.waitForEvent(gBrowser.selectedBrowser, "load", true); > + yield ContentTaskUtils.waitForCondition(() => { ContentTaskUtils is intended for, well, content tasks... it's kind of weird that we're using it in the parent process here. Additionally, waitForCondition needs to die in a fire. It would be better if we would wait for onLocationChange or something, but unfortunately it seems like that is non-trivial. Up to you whether you want to land like this or followup. ::: browser/components/uitour/test/browser_openPreferences.js:37 (Diff revisions 1 - 2) > + yield ContentTaskUtils.waitForCondition(() => { > + return gBrowser.selectedBrowser.currentURI.spec.startsWith("about:preferences"); > + }, "Check if about:preferences opened"); This will wait 5 whole seconds. Please tone it down a little (1s should be enough).

https://reviewboard.mozilla.org/r/12675/#review11367 > ContentTaskUtils is intended for, well, content tasks... it's kind of weird that we're using it in the parent process here. > > Additionally, waitForCondition needs to die in a fire. It would be better if we would wait for onLocationChange or something, but unfortunately it seems like that is non-trivial. Up to you whether you want to land like this or followup. > ContentTaskUtils is intended for, well, content tasks... it's kind of weird that we're using it in the parent process here. Yeah, I agree but until we have bug 940882 it's the only shared implementation that I know of and I'd rather not make another fork. I now see that UITour's head.js already has a fork so I'll use it. > Additionally, waitForCondition needs to die in a fire. It's better than a single setTimeout for intermittents and in some cases it's much easier than a bullet-proof check without timers. > It would be better if we would wait for onLocationChange or something, but unfortunately it seems like that is non-trivial. I would have checked the readyState + a load listener but e10s… I've added a new helper now to BrowserTestUtils and I'm using it. > This will wait 5 whole seconds. Please tone it down a little (1s should be enough). Fixed by switching to the head.js version.

Comment on attachment 8630075 [details] MozReview Request: Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs

Comment on attachment 8630075 [details] MozReview Request: Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs https://reviewboard.mozilla.org/r/12675/#review11475 ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:171 (Diff revision 3) > + let openEvent = yield this.waitForEvent(tabbrowser.tabContainer, "TabOpen"); Doesn't this still race? Because onLocationChange might happen straight after the TabOpen, and yield here is async. The hacky way to fix this is to add the progressListener from the "checkEvent" function that waitForEvent takes which is executed synchronously when the event is dispatched. Either that or just actually dealing with the event listeners yourself (or adding another API to BrowserTestUtils...).

https://reviewboard.mozilla.org/r/12675/#review11475 > Doesn't this still race? Because onLocationChange might happen straight after the TabOpen, and yield here is async. > > The hacky way to fix this is to add the progressListener from the "checkEvent" function that waitForEvent takes which is executed synchronously when the event is dispatched. Either that or just actually dealing with the event listeners yourself (or adding another API to BrowserTestUtils...). Yeah, sorry about that. I'm going to handle the events myself.

Comment on attachment 8630075 [details] MozReview Request: Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs

Comment on attachment 8630075 [details] MozReview Request: Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs https://reviewboard.mozilla.org/r/12675/#review11509 Ship It!

(on the assumption that you don't change which tests are enabled/disabled while you reorder them, the last interdiff shows one test being re-enabled... not sure what's up there)