Closed Bug 1261949 Opened 6 years ago Closed 6 years ago

Complete test coverage for browser.windows events

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

Attachments

(1 file)

browser.windows is missing coverage for:

* The |onCreated|, |onRemoved|, and |onFocusChanged| events.

Missing coverage can be seen at https://people.mozilla.org/~kmaglione/webextension-test-coverage/browser/components/extensions/ext-windows.js.html
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 18
Flags: blocking-webextensions+
Add coverage for:
* browser.windows.onCreated
* browser.windows.onRemoved
* browser.windows.onFocusChanged
* browser.windows.getLastFocused

Review commit: https://reviewboard.mozilla.org/r/44365/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44365/
Attachment #8738148 - Flags: review?(kmaglione+bmo)
I also added coverage for `getLastFocused` as I was already writing coverage for the `onFocusChanged` event, so I believe that this will finish the coverage for `browser.windows`, once bug 1261185 lands (which was backed out due to leaks).
Comment on attachment 8738148 [details]
MozReview Request: Bug 1261949 - Complete test coverage for browser.windows events, r?kmag

https://reviewboard.mozilla.org/r/44365/#review41205

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:8
(Diff revision 1)
> +"use strict";
> +
> +add_task(function* testWindowsEvents() {
> +  function background() {
> +    let isInteger = x => {
> +      return typeof x === "number" && (x % 1) === 0;

`Number.isInteger(x)`

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:16
(Diff revision 1)
> +    let focusChangedEventCount = 0;
> +
> +    browser.windows.onCreated.addListener(function listener(window) {
> +      browser.windows.onCreated.removeListener(listener);
> +      browser.test.assertTrue(isInteger(window.id),
> +        "Window object's id is an integer");

Please align after the opening paren on the previous line, or move all arguments to the next line. Same for the other calls.

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:31
(Diff revision 1)
> +        "windowId is an integer");
> +      browser.windows.getLastFocused().then(window => {
> +        browser.test.assertEq(windowId, window.id,
> +          "Last focused window has the correct id");
> +      });
> +      if (focusChangedEventCount === 1) {

Why is this necessary?

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:32
(Diff revision 1)
> +      browser.windows.getLastFocused().then(window => {
> +        browser.test.assertEq(windowId, window.id,
> +          "Last focused window has the correct id");
> +      });
> +      if (focusChangedEventCount === 1) {
> +        browser.test.sendMessage(`window-focus-changed-${windowId}`);

This should happen inside the `getLastFocused` callback.
Attachment #8738148 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8738148 [details]
MozReview Request: Bug 1261949 - Complete test coverage for browser.windows events, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44365/diff/1-2/
https://reviewboard.mozilla.org/r/44365/#review41205

> Why is this necessary?

Without it the `sendMessage` gets issues twice on e10s, and the test only expects one message. I have found that the behaviour is that on non-e10s the listener only gets fired once, but on e10s it is fired twice. I noticed this is documented, that sometimes the listener will fire twice, so I chose not to treat it as a bug, but I also wanted to make the test resilient to that fact, including not breaking if that behaviour changes and the listener always only fires once in the future. So at this point I only care that it fires once, and am happy to ignore the second firing.

I did move things around a bit in this next iteration, but I've kept the `if`.
Kris, I'm not sure if my response to your question is satisfactory. You r+'d this before my response. Is this good to land?
Flags: needinfo?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/44365/#review41205

> Without it the `sendMessage` gets issues twice on e10s, and the test only expects one message. I have found that the behaviour is that on non-e10s the listener only gets fired once, but on e10s it is fired twice. I noticed this is documented, that sometimes the listener will fire twice, so I chose not to treat it as a bug, but I also wanted to make the test resilient to that fact, including not breaking if that behaviour changes and the listener always only fires once in the future. So at this point I only care that it fires once, and am happy to ignore the second firing.
> 
> I did move things around a bit in this next iteration, but I've kept the `if`.

I think that bug 1248846 should fix this, at least as far as preventing the second event firing after you call `removeListener`. But I'd rather not add tests that try to work around this bug. Can you file a separate bug to fix this issue, and add tests for the even there?
Comment on attachment 8738148 [details]
MozReview Request: Bug 1261949 - Complete test coverage for browser.windows events, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44365/diff/2-3/
https://reviewboard.mozilla.org/r/44365/#review41205

> I think that bug 1248846 should fix this, at least as far as preventing the second event firing after you call `removeListener`. But I'd rather not add tests that try to work around this bug. Can you file a separate bug to fix this issue, and add tests for the even there?

Ok, I removed the tests for `onFocusChanged` and `getLastFocused` so that this should be okay to land on its own. I added those tests into the patch for bug 1262976.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/edf329166f4a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.