1261949 - Complete test coverage for browser.windows events

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

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

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1261949 - Complete test coverage for browser.windows events, r?kmag

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/

Comment 2

[Bob Silverberg [:bsilverberg]]

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 3

[Kris Maglione [:kmag]]

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.

Comment 4

[Bob Silverberg [:bsilverberg]]

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/

Comment 5

[Bob Silverberg [:bsilverberg]]

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`.

Comment 6

[Bob Silverberg [:bsilverberg]]

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?

Comment 7

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2519d1a05517

Comment 8

[Kris Maglione [:kmag]]

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 9

[Bob Silverberg [:bsilverberg]]

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/

Comment 10

[Bob Silverberg [:bsilverberg]]

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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/edf329166f4a

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edf329166f4a