Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Description • 9 years ago

This bug covers some work I've been doing to eliminate CPOWs from some browser-chrome tests. I started working on this for bug 967873, but it's generally useful stuff since we always want to reduce our use of CPOWs. So I'm putting it in a separate bug.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 1 • 9 years ago

This patch extends BrowserTestUtils.browserLoaded so that you can decide whether a particular load is of interest to you.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 2 • 9 years ago

This simply allows you to use |this| inside a ContentTask. Basically it means you can add event listeners to the frame script global, which is really useful.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 3 • 9 years ago

This patch is a little gross but very useful. It allows you to use ok(), is(), and info() inside a ContentTask. It makes it a lot easier to convert existing tests to use ContentTask.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 4 • 9 years ago

For the remaining patches I don't have much to say...

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 35 • 9 years ago

This test just barely exceeds the 90 second limit with my changes. Looking at runs over the past couple days, it's just under 90 seconds.

	Mike Conley (:mconley) (:⚙️)
	Comment 36 • 9 years ago

Comment on attachment 8680350 [details] [diff] [review]
01 BrowserTestUtils 1

Review of attachment 8680350 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with doc update.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +146,3 @@
>     *
>     * @return {Promise}
>     * @resolves When a load event is triggered for the browser.

You'll need to also update this to mention that it resolves to the url of the page that loaded.

	:Gijs (he/him)
	Comment 37 • 9 years ago

Comment on attachment 8680382 [details] [diff] [review]
28 Fix browser_force_refresh.js

Review of attachment 8680382 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/browser_force_refresh.js
@@ +42,3 @@
>      var cachedLoad = false;
>  
> +    function eventHandler(msg) {

Nit: this is still called eventHandler, but it is really a messageHandler, so it should probably be called that.

You could avoid the s/event/msg.data/ changes in the function body by defining the function as:

function messageHandler({data: event})

because none of the other fields are used. Up to you whether you think that's worth it / clearer.

	:Gijs (he/him)
	Comment 38 • 9 years ago

Comment on attachment 8680366 [details] [diff] [review]
15 Fix browser_URLBarSetURI.js

Review of attachment 8680366 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_URLBarSetURI.js
@@ -74,1 @@
>      info("Tab loaded");

This means this info() will now only happen for the load we're interested in, instead of for every load. I don't think that's bad, because it didn't log the "other" URL that got loaded either, so it probably wasn't super useful to begin with.

	:Gijs (he/him)
	Comment 39 • 9 years ago

Comment on attachment 8680367 [details] [diff] [review]
16 Fix browser_bug329212.js

Review of attachment 8680367 [details] [diff] [review]:
-----------------------------------------------------------------

s/Fox/Fix/ in the commit message, though maybe you'll squash/fold the commits anyway? :-)

	:Gijs (he/him)
	Comment 40 • 9 years ago

Comment on attachment 8680360 [details] [diff] [review]
10 Fix browser_popupUI.js

Review of attachment 8680360 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_popupUI.js
@@ +2,5 @@
>    waitForExplicitFinish();
>    gPrefService.setBoolPref("dom.disable_open_during_load", false);
>  
>    var browser = gBrowser.selectedBrowser;
> +  BrowserTestUtils.browserLoaded(browser).then(() => {

Nit: might as well nix the browser = gBrowser.selectedBrowser assignment and use it directly, here and below.

	:Gijs (he/him)
	Comment 41 • 9 years ago

Comment on attachment 8680359 [details] [diff] [review]
09 Fix browser_findbarClose.js

Review of attachment 8680359 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_findbarClose.js
@@ +9,1 @@
>    waitForExplicitFinish();

You don't need waitForExplicitFinish() inside add_task.

@@ +21,5 @@
> +  yield new ContentTask.spawn(newTab.linkedBrowser, null, function* () {
> +    let iframe = content.document.getElementById("iframe");
> +    let promise = ContentTaskUtils.waitForEvent(iframe, "load", false);
> +    iframe.src = "http://example.org/";
> +    yield promise;

You could just return the promise here, right?

@@ +29,5 @@
>       "subdocument changes");
>  
>    gFindBar.close();
>    gBrowser.removeTab(newTab);
>    finish();

Likewise, you don't need finish() here.

	:Gijs (he/him)
	Comment 42 • 9 years ago

Comment on attachment 8680350 [details] [diff] [review]
01 BrowserTestUtils 1

Review of attachment 8680350 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +150,5 @@
> +  browserLoaded(browser, includeSubFrames=false, wantLoad=null) {
> +    function isWanted(url) {
> +      if (!wantLoad) {
> +        return true;
> +      } else if (typeof(wantLoad) == "function") {

Driveby nit because I had to look at this for the other reviews: please don't use else after a return.

	Matthew N. [:MattN]
	Comment 43 • 9 years ago

Comment on attachment 8680387 [details] [diff] [review]
33 Fix passwordmgr

Review of attachment 8680387 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this! I wish we would modernize tests more often.

mozreview makes it much easier to review code movement and indentation changes btw.

	Dave Townsend [:mossop]
	Comment 44 • 9 years ago

Comment on attachment 8680383 [details] [diff] [review]
30 Fix xpinstall tests

Review of attachment 8680383 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks.

	Mike Conley (:mconley) (:⚙️)
	Comment 45 • 9 years ago

Comment on attachment 8680353 [details] [diff] [review]
03 content-task 2

Review of attachment 8680353 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding these!

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 46 • 9 years ago

Comment on attachment 8680357 [details] [diff] [review]
07 Fix browser_bug676619.js

Mike, I think Felipe is a TRIBE. Would you mind looking at this one?

	Mike Conley (:mconley) (:⚙️)
	Comment 47 • 9 years ago

Comment on attachment 8680357 [details] [diff] [review]
07 Fix browser_bug676619.js

Review of attachment 8680357 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for getting this test off CPOWs. Might be nice to put a bullet in this thing and write a new add_task'y test someday, but today is not that day.

	Pulsebot
	Comment 48 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/77f868daa629
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b0dcd99711

	Carsten Book [:Tomcat]
	Comment 49 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/77f868daa629
https://hg.mozilla.org/mozilla-central/rev/90b0dcd99711