Closed Bug 1346286 Opened 7 years ago Closed 7 years ago

Remove CPOWs from more tests

Categories

(Firefox :: General, enhancement)

Product:
Firefox
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(18 files)

Bug 1346286 - Remove CPOWs from browser_739805.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_687710_2.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_500328.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_findbar.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_usercontext.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_bug578534.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_bug749738.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_bug822367.js
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_bug882977.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_911547.js.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_history_persist.js
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1346286 - Remove CPOWs from browser_aboutCertError.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1346286 - Remove CPOWs from browser_aboutNetError.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1346286 - Remove CPOWs from browser_bug554155.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1346286 - Remove CPOWs from browser_bug1045809.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1346286 - Remove CPOWs from browser_child_resource.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1346286 - Remove CPOW usage via browser_bug511456.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1346286 - Remove CPOWs from browser_popupNotification_3.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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.
needinfo'ing myself for reviews
Flags: needinfo?(felipc)
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 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 on attachment 8845979 [details]
Bug 1346286 - Remove CPOWs from browser_findbar.js.

https://reviewboard.mozilla.org/r/119068/#review121550
Attachment #8845979 - Flags: 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 on attachment 8845981 [details]
Bug 1346286 - Remove CPOWs from browser_bug578534.js.

https://reviewboard.mozilla.org/r/119072/#review121558
Attachment #8845981 - Flags: 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 on attachment 8845987 [details]
Bug 1346286 - Remove CPOWs from browser_aboutCertError.js.

https://reviewboard.mozilla.org/r/119084/#review121566
Attachment #8845987 - Flags: 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 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 on attachment 8845990 [details]
Bug 1346286 - Remove CPOWs from browser_bug1045809.js.

https://reviewboard.mozilla.org/r/119090/#review121574
Attachment #8845990 - Flags: 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 on attachment 8845984 [details]
Bug 1346286 - Remove CPOWs from browser_bug882977.js.

https://reviewboard.mozilla.org/r/119078/#review121578
Attachment #8845984 - Flags: 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 on attachment 8845993 [details]
Bug 1346286 - Remove CPOWs from browser_popupNotification_3.js.

https://reviewboard.mozilla.org/r/119096/#review121584
Attachment #8845993 - Flags: 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 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 on attachment 8845985 [details]
Bug 1346286 - Remove CPOWs from browser_911547.js.

https://reviewboard.mozilla.org/r/119080/#review122062
Attachment #8845985 - Flags: 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)
(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)
(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)
(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.
Attachment #8845987 - Flags: review?(felipc)
Attachment #8845988 - Flags: review?(felipc)
Attachment #8845989 - Flags: review?(felipc)
Attachment #8845990 - Flags: review?(felipc)
I hate reviewboard.
Attachment #8845987 - Flags: review?(felipc)
Attachment #8845988 - Flags: review?(felipc)
Attachment #8845989 - Flags: review?(felipc)
Attachment #8845990 - Flags: review?(felipc)
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+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: