Remove CPOWs from more tests

RESOLVED FIXED in Firefox -esr52

Status

()

Firefox
General
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
Firefox 55
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(18 attachments)

59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
needinfo'ing myself for reviews
Flags: needinfo?(felipc)

Comment 20

a year ago
mozreview-review
Comment on attachment 8845976 [details]
Bug 1346286 - Remove CPOWs from browser_739805.js.

https://reviewboard.mozilla.org/r/119062/#review121068
Attachment #8845976 - Flags: review+

Comment 21

a year ago
mozreview-review
Comment on attachment 8845977 [details]
Bug 1346286 - Remove CPOWs from browser_687710_2.js.

https://reviewboard.mozilla.org/r/119064/#review121092
Attachment #8845977 - Flags: review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8845979 [details]
Bug 1346286 - Remove CPOWs from browser_findbar.js.

https://reviewboard.mozilla.org/r/119068/#review121550
Attachment #8845979 - Flags: review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8845980 [details]
Bug 1346286 - Remove CPOWs from browser_usercontext.js.

https://reviewboard.mozilla.org/r/119070/#review121552
Attachment #8845980 - Flags: review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8845981 [details]
Bug 1346286 - Remove CPOWs from browser_bug578534.js.

https://reviewboard.mozilla.org/r/119072/#review121558
Attachment #8845981 - Flags: review+

Comment 25

a year ago
mozreview-review
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.
Attachment #8845988 - Flags: review-

Comment 26

a year ago
mozreview-review
Comment on attachment 8845987 [details]
Bug 1346286 - Remove CPOWs from browser_aboutCertError.js.

https://reviewboard.mozilla.org/r/119084/#review121566
Attachment #8845987 - Flags: review+

Comment 27

a year ago
mozreview-review
Comment on attachment 8845982 [details]
Bug 1346286 - Remove CPOWs from browser_bug749738.js.

https://reviewboard.mozilla.org/r/119074/#review121568
Attachment #8845982 - Flags: review+

Comment 28

a year ago
mozreview-review
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
Attachment #8845989 - Flags: review+

Comment 29

a year ago
mozreview-review
Comment on attachment 8845990 [details]
Bug 1346286 - Remove CPOWs from browser_bug1045809.js.

https://reviewboard.mozilla.org/r/119090/#review121574
Attachment #8845990 - Flags: review+

Comment 30

a year ago
mozreview-review
Comment on attachment 8845991 [details]
Bug 1346286 - Remove CPOWs from browser_child_resource.js.

https://reviewboard.mozilla.org/r/119092/#review121576
Attachment #8845991 - Flags: review+

Comment 31

a year ago
mozreview-review
Comment on attachment 8845984 [details]
Bug 1346286 - Remove CPOWs from browser_bug882977.js.

https://reviewboard.mozilla.org/r/119078/#review121578
Attachment #8845984 - Flags: review+

Comment 32

a year ago
mozreview-review
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.
Attachment #8845992 - Flags: review+

Comment 33

a year ago
mozreview-review
Comment on attachment 8845993 [details]
Bug 1346286 - Remove CPOWs from browser_popupNotification_3.js.

https://reviewboard.mozilla.org/r/119096/#review121584
Attachment #8845993 - Flags: review+
Blocks: 1334455

Comment 34

a year ago
mozreview-review
Comment on attachment 8845978 [details]
Bug 1346286 - Remove CPOWs from browser_500328.js.

https://reviewboard.mozilla.org/r/119066/#review122058
Attachment #8845978 - Flags: review+

Comment 35

a year ago
mozreview-review
Comment on attachment 8845986 [details]
Bug 1346286 - Remove CPOWs from browser_history_persist.js

https://reviewboard.mozilla.org/r/119082/#review122060
Attachment #8845986 - Flags: review+

Comment 36

a year ago
mozreview-review
Comment on attachment 8845985 [details]
Bug 1346286 - Remove CPOWs from browser_911547.js.

https://reviewboard.mozilla.org/r/119080/#review122062
Attachment #8845985 - Flags: review+

Comment 37

a year ago
mozreview-review
Comment on attachment 8845983 [details]
Bug 1346286 - Remove CPOWs from browser_bug822367.js

https://reviewboard.mozilla.org/r/119076/#review122068
Attachment #8845983 - Flags: review+
Flags: needinfo?(felipc)
(Assignee)

Comment 38

a year ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 39

a year ago
(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? :-)
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 40

a year ago
(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.
(Assignee)

Updated

a year ago
Attachment #8845987 - Flags: review?(felipc)
Attachment #8845988 - Flags: review?(felipc)
Attachment #8845989 - Flags: review?(felipc)
Attachment #8845990 - Flags: review?(felipc)
(Assignee)

Comment 41

a year ago
I hate reviewboard.
(Assignee)

Updated

a year ago
Attachment #8845987 - Flags: review?(felipc)
(Assignee)

Updated

a year ago
Attachment #8845988 - Flags: review?(felipc)
(Assignee)

Updated

a year ago
Attachment #8845989 - Flags: review?(felipc)
(Assignee)

Updated

a year ago
Attachment #8845990 - Flags: review?(felipc)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 60

a year ago
mozreview-review
Comment on attachment 8845988 [details]
Bug 1346286 - Remove CPOWs from browser_aboutNetError.js.

https://reviewboard.mozilla.org/r/119086/#review122718
Attachment #8845988 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 61

a year ago
Pushed by mrbkap@mozilla.com:
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 63

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e62e7262959
status-firefox54: --- → fixed
Flags: in-testsuite+

Comment 64

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/7269ab549229
status-firefox53: --- → fixed

Comment 65

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/3e1c39fa0525
status-firefox-esr52: --- → fixed
You need to log in before you can comment on or make changes to this bug.