Closed Bug 1177169 Opened 9 years ago Closed 9 years ago

Add the ability to open (privacy) preferences via UITour

Categories

(Firefox :: Tours, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 42
Iteration:
42.1 - Jul 13
Tracking Status
firefox42 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(1 file)

A generic loadURL API will probably be more scalable that a specific (privacy) preferences one as we already could have used it for "showFirefoxAccounts".
Flags: firefox-backlog+
Points: --- → 3
If we use a generic API we should have a whitelist. For about:preferences we regularly use switchToTab to prevent multiple.
Blocks: 1175017
Rank: 17
Priority: -- → P1
Flags: qe-verify-
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 42.1 - Jul 13
Bug 1177169 - Add the ability to open preferences via UITour. r=Gijs
Attachment #8630075 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8630075 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8630075 - Flags: review?(gijskruitbosch+bugs)
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).
Attachment #8630075 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8630075 - Flags: review+ → review?(gijskruitbosch+bugs)
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
Attachment #8630075 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8630075 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8630075 - Flags: review?(gijskruitbosch+bugs) → review+
(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)
https://hg.mozilla.org/mozilla-central/rev/6a26d26f6695
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: