1346286 - Remove CPOWs from more tests

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I've been working on my list of CPOW-using tests. Here's a try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d879ad7519fb4a0142374be19ee50788f3977de

I'm going to flag Felipe for the reviews here, but others are always welcome to help out.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_739805.js.

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

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_687710_2.js.

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

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

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

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_findbar.js.

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

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_usercontext.js.

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

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_bug578534.js.

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

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_bug749738.js.

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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_bug822367.js

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

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_bug882977.js.

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

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_911547.js.

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

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_history_persist.js

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

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_aboutCertError.js.

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

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_aboutNetError.js.

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

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_bug554155.js.

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

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_bug1045809.js.

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

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_child_resource.js.

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

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOW usage via browser_bug511456.js.

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

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1346286 - Remove CPOWs from browser_popupNotification_3.js.

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

Comment 19

[:Felipe Gomes (needinfo for replies!)]

needinfo'ing myself for reviews

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845976 [details]
Bug 1346286 - Remove CPOWs from browser_739805.js.

https://reviewboard.mozilla.org/r/119062/#review121068

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845977 [details]
Bug 1346286 - Remove CPOWs from browser_687710_2.js.

https://reviewboard.mozilla.org/r/119064/#review121092

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845979 [details]
Bug 1346286 - Remove CPOWs from browser_findbar.js.

https://reviewboard.mozilla.org/r/119068/#review121550

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845980 [details]
Bug 1346286 - Remove CPOWs from browser_usercontext.js.

https://reviewboard.mozilla.org/r/119070/#review121552

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845981 [details]
Bug 1346286 - Remove CPOWs from browser_bug578534.js.

https://reviewboard.mozilla.org/r/119072/#review121558

Comment 25

[:Gijs (he/him)]

Comment on attachment 8845988 [details]
Bug 1346286 - Remove CPOWs from browser_aboutNetError.js.

https://reviewboard.mozilla.org/r/119086/#review121562

::: browser/base/content/test/general/browser_aboutNetError.js:38
(Diff revision 1)
> -    Assert.equal(prefResetButton.getAttribute("autofocus"), "true", "prefResetButton has autofocus");
> +    is(prefResetButton.getAttribute("autofocus"), "true", "prefResetButton has autofocus");
>      prefResetButton.click();
> -  });
> -  yield pageshowPromise;
>  
> -  Assert.equal(content.document.documentURI, LOW_TLS_VERSION, "Should not be showing page");
> +    yield ContentTaskUtils.waitForEvent(this, "pageshow", true);

Does this actually work? In general, I think this should probably be using https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#705 . There might even be a separate bug for those changes - :johannh would know.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8845987 [details]
Bug 1346286 - Remove CPOWs from browser_aboutCertError.js.

https://reviewboard.mozilla.org/r/119084/#review121566

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845982 [details]
Bug 1346286 - Remove CPOWs from browser_bug749738.js.

https://reviewboard.mozilla.org/r/119074/#review121568

Comment 28

[:Gijs (he/him)]

Comment on attachment 8845989 [details]
Bug 1346286 - Remove CPOWs from browser_bug554155.js.

https://reviewboard.mozilla.org/r/119088/#review121570

::: docshell/test/browser/browser_bug554155.js:20
(Diff revision 1)
> -    // pushState to a new URL (http://example.com/foo").  This should trigger
> +      // pushState to a new URL (http://example.com/foo").  This should trigger
> -    // exactly one LocationChange event.
> +      // exactly one LocationChange event.
> -    tab.linkedBrowser.contentWindow.history.pushState(null, null, "foo");
> +      content.history.pushState(null, null, "foo");
> +    });
> +
> +    yield new Promise(resolve => setTimeout(resolve(), 0));

Typo? Surely this should be:

```js
yield new Promise(resolve => setTimeout(resolve, 0));
```

(note no () after 'resolve').

If this passes reliably, I except you can just drop the setTimeout and compact to:

```js
yield Promise.resolve();
```

with either of those (but I prefer the second!), r=me

Comment 29

[:Gijs (he/him)]

Comment on attachment 8845990 [details]
Bug 1346286 - Remove CPOWs from browser_bug1045809.js.

https://reviewboard.mozilla.org/r/119090/#review121574

Comment 30

[:Gijs (he/him)]

Comment on attachment 8845991 [details]
Bug 1346286 - Remove CPOWs from browser_child_resource.js.

https://reviewboard.mozilla.org/r/119092/#review121576

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845984 [details]
Bug 1346286 - Remove CPOWs from browser_bug882977.js.

https://reviewboard.mozilla.org/r/119078/#review121578

Comment 32

[:Gijs (he/him)]

Comment on attachment 8845992 [details]
Bug 1346286 - Remove CPOW usage via browser_bug511456.js.

https://reviewboard.mozilla.org/r/119094/#review121580

::: toolkit/components/startup/tests/browser/head.js:9
(Diff revision 1)
>  "use strict";
>  
>  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>  
>  function whenBrowserLoaded(browser, callback) {
> -  browser.addEventListener("load", function onLoad(event) {
> +  return BrowserTestUtils.browserLoaded(browser).then(callback);

Are we not able to just update all the consumers?

Either way, r=me.

Comment 33

[:Gijs (he/him)]

Comment on attachment 8845993 [details]
Bug 1346286 - Remove CPOWs from browser_popupNotification_3.js.

https://reviewboard.mozilla.org/r/119096/#review121584

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845978 [details]
Bug 1346286 - Remove CPOWs from browser_500328.js.

https://reviewboard.mozilla.org/r/119066/#review122058

Comment 35

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845986 [details]
Bug 1346286 - Remove CPOWs from browser_history_persist.js

https://reviewboard.mozilla.org/r/119082/#review122060

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845985 [details]
Bug 1346286 - Remove CPOWs from browser_911547.js.

https://reviewboard.mozilla.org/r/119080/#review122062

Comment 37

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8845983 [details]
Bug 1346286 - Remove CPOWs from browser_bug822367.js

https://reviewboard.mozilla.org/r/119076/#review122068

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gijs from comment #25)
> Does this actually work? In general, I think this should probably be using ...

I'm not sure I understand this question. This test is already using BTU.waitForErrorPage. The quoted code is waiting for the "pageshow" event that we get after clicking on the "Restore default settings" button on the loaded error page and causing us to be able to load the original URI.

Comment 39

[:Gijs (he/him)]

(In reply to Blake Kaplan (:mrbkap) from comment #38)
> (In reply to :Gijs from comment #25)
> > Does this actually work? In general, I think this should probably be using ...
> 
> I'm not sure I understand this question. This test is already using
> BTU.waitForErrorPage.

D'oh.

> The quoted code is waiting for the "pageshow" event
> that we get after clicking on the "Restore default settings" button on the
> loaded error page and causing us to be able to load the original URI.

Yeah, the potential issue I was worried about is that it used to be the case that, depending on the situation, loading an error page would cause a remoteness switch (so, potentially, unloading the error page in favour of something else would, too) in which case I've had issues with using waitForContentEvent or ContentTasks (because they'd be waiting for an event in the wrong docshell/process). However, based on the context here I would expect all of this to be in the content process (when e10s is enabled) so hopefully it's just fine and I'm just being paranoid? :-)

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gijs from comment #39)
> hopefully it's just fine and I'm just being paranoid? :-)

Yeah, we now load error pages in the child so things here work. I'll add a comment to the test to warn future people to look out for this gotcha if the test starts failing.

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

I hate reviewboard.

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845976 [details]
Bug 1346286 - Remove CPOWs from browser_739805.js.

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

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845977 [details]
Bug 1346286 - Remove CPOWs from browser_687710_2.js.

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

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845978 [details]
Bug 1346286 - Remove CPOWs from browser_500328.js.

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

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845979 [details]
Bug 1346286 - Remove CPOWs from browser_findbar.js.

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

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845980 [details]
Bug 1346286 - Remove CPOWs from browser_usercontext.js.

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

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845981 [details]
Bug 1346286 - Remove CPOWs from browser_bug578534.js.

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

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845982 [details]
Bug 1346286 - Remove CPOWs from browser_bug749738.js.

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

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845983 [details]
Bug 1346286 - Remove CPOWs from browser_bug822367.js

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

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845984 [details]
Bug 1346286 - Remove CPOWs from browser_bug882977.js.

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

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845985 [details]
Bug 1346286 - Remove CPOWs from browser_911547.js.

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

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845986 [details]
Bug 1346286 - Remove CPOWs from browser_history_persist.js

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

Comment 53

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845987 [details]
Bug 1346286 - Remove CPOWs from browser_aboutCertError.js.

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

Comment 54

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845988 [details]
Bug 1346286 - Remove CPOWs from browser_aboutNetError.js.

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

Comment 55

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845989 [details]
Bug 1346286 - Remove CPOWs from browser_bug554155.js.

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

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845990 [details]
Bug 1346286 - Remove CPOWs from browser_bug1045809.js.

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

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845991 [details]
Bug 1346286 - Remove CPOWs from browser_child_resource.js.

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

Comment 58

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845992 [details]
Bug 1346286 - Remove CPOW usage via browser_bug511456.js.

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

Comment 59

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8845993 [details]
Bug 1346286 - Remove CPOWs from browser_popupNotification_3.js.

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

Comment 60

[:Gijs (he/him)]

Comment on attachment 8845988 [details]
Bug 1346286 - Remove CPOWs from browser_aboutNetError.js.

https://reviewboard.mozilla.org/r/119086/#review122718

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7b163f179c4
Remove CPOWs from browser_739805.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/6ec3dfe59edc
Remove CPOWs from browser_687710_2.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/0ecff791a04f
Remove CPOWs from browser_500328.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/5868ff24a21d
Remove CPOWs from browser_findbar.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/d2d0684d2606
Remove CPOWs from browser_usercontext.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/ae9012a9f59a
Remove CPOWs from browser_bug578534.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/6687f5dfcca8
Remove CPOWs from browser_bug749738.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/a835fb5156aa
Remove CPOWs from browser_bug822367.js r=Felipe
https://hg.mozilla.org/integration/autoland/rev/5bb902a79442
Remove CPOWs from browser_bug882977.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/ac5932c6b054
Remove CPOWs from browser_911547.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/e457c43d07bf
Remove CPOWs from browser_history_persist.js r=Felipe
https://hg.mozilla.org/integration/autoland/rev/de71b499fd2f
Remove CPOWs from browser_aboutCertError.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/fe7ad7712763
Remove CPOWs from browser_aboutNetError.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a3955f774aec
Remove CPOWs from browser_bug554155.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/72f436784abc
Remove CPOWs from browser_bug1045809.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/8e5371678d52
Remove CPOWs from browser_child_resource.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/aacc3de27376
Remove CPOW usage via browser_bug511456.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ecea6739ebf1
Remove CPOWs from browser_popupNotification_3.js. r=Gijs

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7b163f179c4
https://hg.mozilla.org/mozilla-central/rev/6ec3dfe59edc
https://hg.mozilla.org/mozilla-central/rev/0ecff791a04f
https://hg.mozilla.org/mozilla-central/rev/5868ff24a21d
https://hg.mozilla.org/mozilla-central/rev/d2d0684d2606
https://hg.mozilla.org/mozilla-central/rev/ae9012a9f59a
https://hg.mozilla.org/mozilla-central/rev/6687f5dfcca8
https://hg.mozilla.org/mozilla-central/rev/a835fb5156aa
https://hg.mozilla.org/mozilla-central/rev/5bb902a79442
https://hg.mozilla.org/mozilla-central/rev/ac5932c6b054
https://hg.mozilla.org/mozilla-central/rev/e457c43d07bf
https://hg.mozilla.org/mozilla-central/rev/de71b499fd2f
https://hg.mozilla.org/mozilla-central/rev/fe7ad7712763
https://hg.mozilla.org/mozilla-central/rev/a3955f774aec
https://hg.mozilla.org/mozilla-central/rev/72f436784abc
https://hg.mozilla.org/mozilla-central/rev/8e5371678d52
https://hg.mozilla.org/mozilla-central/rev/aacc3de27376
https://hg.mozilla.org/mozilla-central/rev/ecea6739ebf1

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e62e7262959

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7269ab549229

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/3e1c39fa0525