1375710 - Remove more CPOWs from tests

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I removed more CPOWs from some tests.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_tabfocus.js.

This test was already trying to prevent itself from touching CPOWs. The trick
is to stop the event from ever propagating all the way to the shim.

Review commit: https://reviewboard.mozilla.org/r/151982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151982/

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_500328.js.

This also reworks an async function to read more linearly.

Review commit: https://reviewboard.mozilla.org/r/151984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151984/

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_iframe_gone_mid_download.js.

Review commit: https://reviewboard.mozilla.org/r/151986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151986/

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_bug575561.js.

In the case that aLinkIndexOrFunction is a function, it's a pain to use the
message manager. Fortunately, the test only uses that functionality for a page
that's loaded in the parent process anyway.

Review commit: https://reviewboard.mozilla.org/r/151988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151988/

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove a CPOW from browser_bug734076.js.

This was an informative message, removing it does not reduce test coverage.

Review commit: https://reviewboard.mozilla.org/r/151990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151990/

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_bug592338.js.

Review commit: https://reviewboard.mozilla.org/r/151992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151992/

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_domFullscreen_fullscreenMode.js.

In these changes, note that the cleanup function was duplicating a check done
at the end of the loop so didn't need to be duplicated.

Review commit: https://reviewboard.mozilla.org/r/151994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151994/

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_e10s_chrome_process.

Review commit: https://reviewboard.mozilla.org/r/151996/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151996/

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_visibleFindSelection.js.

Unfortunately, adding the scroll event listener to the message manager global
didn't work in non-e10s mode so I just do the original thing in the non-e10s
case.

Review commit: https://reviewboard.mozilla.org/r/151998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151998/

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375710 - Remove CPOWs from browser_plainTextLinks.js.

Review commit: https://reviewboard.mozilla.org/r/152000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152000/

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8880646 [details]
Bug 1375710 - Remove CPOWs from browser_tabfocus.js.

https://reviewboard.mozilla.org/r/151982/#review157096

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8880647 [details]
Bug 1375710 - Remove CPOWs from browser_500328.js.

https://reviewboard.mozilla.org/r/151984/#review157100

I feel bad because this is a really nice refactoring, but I think using the browser.goBack()/goForward() APIs is actually part of the 'point' of this test, so switching those to content-process-side DOM API calls is not OK.

::: browser/components/sessionstore/test/browser_500328.js:19
(Diff revision 1)
> -
> +  content.testState = "foo";
> -  let popStateCount = 0;
>  
> -  browser.addEventListener("popstate", function(aEvent) {
> -    if (popStateCount == 0) {
> -      popStateCount++;
> +  // Now go back.  This should trigger the popstate event handler.
> +  let popstatePromise = ContentTaskUtils.waitForEvent(this, "popstate", true);
> +  content.history.back();

Both here and in the forward() case, I think we should ensure that we continue to call browser.goForward / browser.goBack() . Bug 500328 landed a separate mochitest to check the DOM history APIs, and we should ensure that browser UI copes correctly with the push/popstate stuff as well. It enters docshell via a very different route, so it seems worth keeping the separate coverage.

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 8880648 [details]
Bug 1375710 - Remove CPOWs from browser_iframe_gone_mid_download.js.

https://reviewboard.mozilla.org/r/151986/#review157102

yay, net code removal!

Comment 14

[:Gijs (out for now; he/him)]

Comment on attachment 8880650 [details]
Bug 1375710 - Remove a CPOW from browser_bug734076.js.

https://reviewboard.mozilla.org/r/151990/#review157104

Comment 15

[:Gijs (out for now; he/him)]

Comment on attachment 8880649 [details]
Bug 1375710 - Remove CPOWs from browser_bug575561.js.

https://reviewboard.mozilla.org/r/151988/#review157106

The advantage of the progress listener approach was that we'd catch the load no matter which tab it happened in, and now we won't and the test will just time out if we expected the wrong thing. I checked bugzilla and all the current intermittent issues with the test come in the form of timeouts *anyway*, so still r+, though if it's not strictly necessary to abandon the progress listener approach (can't tell really quickly - are the nsIChannel/nsIRequests CPOWs, or something?) then perhaps you want to keep them? Up to you.

::: browser/base/content/test/general/browser_bug575561.js:61
(Diff revision 1)
> -  if (testSubFrame)
> +  await BrowserTestUtils.browserLoaded(browser);
> -    browser = browser.contentDocument.querySelector("iframe");
>  
> -  let link;
> -  if (typeof aLinkIndexOrFunction == "function") {
> -    link = aLinkIndexOrFunction(browser.contentDocument);
> +  let promise;
> +  if (expectNewTab) {
> +    promise = BrowserTestUtils.waitForNewTab(gBrowser).then((tab) => {

Nit: don't need () around a single-argument of an arrow function.

::: browser/base/content/test/general/browser_bug575561.js:87
(Diff revision 1)
> +      link.click();
> +      return link.href;
> +    });
> +  }
>  
> -  gBrowser.removeTab(appTab);
> +  let loaded = await promise;

Please add an info() before this line saying something like 'waiting on load of <href>' so that if/when tests do fail with a timeout, it's more obvious what failed.

Comment 16

[:Gijs (out for now; he/him)]

Comment on attachment 8880651 [details]
Bug 1375710 - Remove CPOWs from browser_bug592338.js.

https://reviewboard.mozilla.org/r/151992/#review157110

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8880652 [details]
Bug 1375710 - Remove CPOWs from browser_domFullscreen_fullscreenMode.js.

https://reviewboard.mozilla.org/r/151994/#review157112

::: commit-message-09fc5:3
(Diff revision 1)
> +In these changes, note that the cleanup function was duplicating a check done
> +at the end of the loop so didn't need to be duplicated.

Well, the cleanup function is guaranteed to run, even if the test breaks halfway through...

::: browser/base/content/test/general/browser_domFullscreen_fullscreenMode.js
(Diff revision 1)
> -  registerCleanupFunction(() => {
> -    if (browser.contentWindow.fullScreen) {
> -      BrowserFullScreen();
> -    }
> -    gBrowser.removeTab(tab);
> -  });

How about just putting the cleanup from the end of the test in here:

```js
registerCleanupFunction(async function() {
  if (window.fullScreen) {
    executeSoon(() => BrowserFullScreen());
    await waitForFullscreenChanges(FS_CHANGE_SIZE);
  }
});
```

That is, assuming that doesn't race with the tab being closed triggering a full screen change as well...

Comment 18

[:Gijs (out for now; he/him)]

Comment on attachment 8880653 [details]
Bug 1375710 - Remove CPOWs from browser_e10s_chrome_process.

https://reviewboard.mozilla.org/r/151996/#review157118

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8880654 [details]
Bug 1375710 - Remove CPOWs from browser_visibleFindSelection.js.

https://reviewboard.mozilla.org/r/151998/#review157120

Comment 20

[:Gijs (out for now; he/him)]

Comment on attachment 8880655 [details]
Bug 1375710 - Remove CPOWs from browser_plainTextLinks.js.

https://reviewboard.mozilla.org/r/152000/#review157122

Comment 21

[:Gijs (out for now; he/him)]

For future reference, for test-only changes like this, if you use --artifact in the try syntax (on the assumption your changes are otherwise against a usual m-i or m-c changeset, not on top of a bunch of other patches) try can use artifact builds and the try push will complete a lot faster. :-)

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880649 [details]
Bug 1375710 - Remove CPOWs from browser_bug575561.js.

https://reviewboard.mozilla.org/r/151988/#review157106

> Please add an info() before this line saying something like 'waiting on load of <href>' so that if/when tests do fail with a timeout, it's more obvious what failed.

If this turns out to be a problem, we can switch this to `BrowserTestUtils.firstBrowserLoaded` which waits for any foreground load. In practice, I'm not too worried, though.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880646 [details]
Bug 1375710 - Remove CPOWs from browser_tabfocus.js.

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

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880647 [details]
Bug 1375710 - Remove CPOWs from browser_500328.js.

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

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880648 [details]
Bug 1375710 - Remove CPOWs from browser_iframe_gone_mid_download.js.

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

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880649 [details]
Bug 1375710 - Remove CPOWs from browser_bug575561.js.

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

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880650 [details]
Bug 1375710 - Remove a CPOW from browser_bug734076.js.

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

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880651 [details]
Bug 1375710 - Remove CPOWs from browser_bug592338.js.

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

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880652 [details]
Bug 1375710 - Remove CPOWs from browser_domFullscreen_fullscreenMode.js.

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

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880653 [details]
Bug 1375710 - Remove CPOWs from browser_e10s_chrome_process.

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

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880654 [details]
Bug 1375710 - Remove CPOWs from browser_visibleFindSelection.js.

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

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880655 [details]
Bug 1375710 - Remove CPOWs from browser_plainTextLinks.js.

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

Comment 33

[:Gijs (out for now; he/him)]

Comment on attachment 8880647 [details]
Bug 1375710 - Remove CPOWs from browser_500328.js.

https://reviewboard.mozilla.org/r/151984/#review158098

Thanks!

Comment 34

[:Gijs (out for now; he/him)]

Comment on attachment 8880652 [details]
Bug 1375710 - Remove CPOWs from browser_domFullscreen_fullscreenMode.js.

https://reviewboard.mozilla.org/r/151994/#review158102

::: commit-message-09fc5:3
(Diff revision 2)
> +In these changes, note that the cleanup function was duplicating a check done
> +at the end of the loop so didn't need to be duplicated.

Nit: please remove this comment from the commit message, as it's outdated now. :-)

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880646 [details]
Bug 1375710 - Remove CPOWs from browser_tabfocus.js.

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

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880647 [details]
Bug 1375710 - Remove CPOWs from browser_500328.js.

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

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880648 [details]
Bug 1375710 - Remove CPOWs from browser_iframe_gone_mid_download.js.

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

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880649 [details]
Bug 1375710 - Remove CPOWs from browser_bug575561.js.

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

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880650 [details]
Bug 1375710 - Remove a CPOW from browser_bug734076.js.

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

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880651 [details]
Bug 1375710 - Remove CPOWs from browser_bug592338.js.

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

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880652 [details]
Bug 1375710 - Remove CPOWs from browser_domFullscreen_fullscreenMode.js.

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

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880653 [details]
Bug 1375710 - Remove CPOWs from browser_e10s_chrome_process.

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

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880654 [details]
Bug 1375710 - Remove CPOWs from browser_visibleFindSelection.js.

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

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880655 [details]
Bug 1375710 - Remove CPOWs from browser_plainTextLinks.js.

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

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Gijs, I added a hack to make things race less in non-e10s, I hope that's OK.

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

In browser_500328.js.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34bd0360c470
Remove CPOWs from browser_tabfocus.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/b97f735867fe
Remove CPOWs from browser_500328.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/8860147eb439
Remove CPOWs from browser_iframe_gone_mid_download.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a15327f2942f
Remove CPOWs from browser_bug575561.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/27bd2b13d517
Remove a CPOW from browser_bug734076.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/99e98edd0f98
Remove CPOWs from browser_bug592338.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a55f83bca4e4
Remove CPOWs from browser_domFullscreen_fullscreenMode.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/30444454a15d
Remove CPOWs from browser_e10s_chrome_process. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/988f116dc33b
Remove CPOWs from browser_visibleFindSelection.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/5e2d242ba522
Remove CPOWs from browser_plainTextLinks.js. r=Gijs

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34bd0360c470
https://hg.mozilla.org/mozilla-central/rev/b97f735867fe
https://hg.mozilla.org/mozilla-central/rev/8860147eb439
https://hg.mozilla.org/mozilla-central/rev/a15327f2942f
https://hg.mozilla.org/mozilla-central/rev/27bd2b13d517
https://hg.mozilla.org/mozilla-central/rev/99e98edd0f98
https://hg.mozilla.org/mozilla-central/rev/a55f83bca4e4
https://hg.mozilla.org/mozilla-central/rev/30444454a15d
https://hg.mozilla.org/mozilla-central/rev/988f116dc33b
https://hg.mozilla.org/mozilla-central/rev/5e2d242ba522