1093566 - Start properly sharing mochitest-browser test code between browser test directories

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Georg Fritzsche [:gfritzsche]]

browser/base/content/test was broken up into multiple sub-directories.
Now each of them has their own head.js, so we end up duplicating test code there.

We should look into properly sharing test code from something like browser/base/content/test/shared_head.js

Comment 1

[:Gijs (he/him)]

Actually, can we go for something even more high-level than that, that just gets auto-included in m-bc tests? I've lost count of how many implementations of promiseTabLoadEvent and waitForCondition and co we have throughout the tree... it's a little silly.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #1)
> Actually, can we go for something even more high-level than that, that just
> gets auto-included in m-bc tests? I've lost count of how many
> implementations of promiseTabLoadEvent and waitForCondition and co we have
> throughout the tree... it's a little silly.

Indeed - I'd rather see things moved into testing JSMs, so they can be used by any test anywhere.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I concur - I have also written promiseTabLoadEvent, promisePageShowEvent too many times.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Actually, can we go for something even more high-level than that, that just
> > gets auto-included in m-bc tests? I've lost count of how many
> > implementations of promiseTabLoadEvent and waitForCondition and co we have
> > throughout the tree... it's a little silly.
> 
> Indeed - I'd rather see things moved into testing JSMs, so they can be used
> by any test anywhere.

Do we have a good location for this? We could probably move things to a shared module incrementally, but someone would have to start it.

Comment 5

[:Gijs (he/him)]

This can just live under the testing/mochitest browser chrome bits, if we're auto-including it, I think?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Do we have a good location for this? We could probably move things to a
> shared module incrementally, but someone would have to start it.

The JSM should just go in the /test/ directory, under whatever directory is related to the utilities it provides. AddonsManagerTesting.jsm as an example may help:

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/moz.build#14
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/AddonManagerTesting.jsm
http://dxr.mozilla.org/mozilla-central/source/browser/experiments/test/xpcshell/test_disableExperiments.js#7

Comment 7

[Georg Fritzsche [:gfritzsche]]

So, how about:
* add a BrowserTest.jsm
* ... which exports "BrowserTest"
* ... from which we hang off the test functions
* import those functions into global scope in head.js where needed (so we don't need to rewrite all the tests)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

One thing that I'm a little worried about is that BrowserTest.jsm will sidestep the add-on shim stuff.

I could be wrong on that, but I'm pretty sure the only reason that browser mochitests for e10s allow us to attach load events on browser elements is because mochitest is an add-on, and runs inside an add-on compartment, which means it gets the e10s add-on shims. I _think_ any functions defined in this BrowserTest.jsm will not benefit from them since they run in a separate compartment.

Which kinda blows. billm, is my math right here? Can we assume that a separate JSM that a mochitest calls into will not have the benefit of the shims?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It depends on where the JSM is located. If it's packaged with the add-on, then it counts as add-on code and gets shims. I don't know too much about how mochitests are packaged, but it seems like we should be able to make this work.

Comment 10

[:Gijs (he/him)]

I really think we should include this from testing/mochitest instead of per-directory. Right now all the implementations of waitForCondition are strewn throughout browser/components/*/test/head.js, browser/base/content/test/*/head.js, etc. etc.

We also have umpteen incantations of the "load url x in tab y, and/or wait until it either starts loading / finishes loading" pattern.

All of this makes the individual incantations hard to trust in the case of intermittents (there have been broken ones in the past!).

Having these in the tested dir /test/Foo.jsm will not solve that problem. I really think we should include this from within testing/mochitest instead.

Comment 11

[Georg Fritzsche [:gfritzsche]]

As long as we can make clear that it's mochitest-browser specific, that sounds good.

I started on trying this out for browser/base/content/test/general before leaving for PTO, it's certainly hacky but i'll try to put it up for a first feedback this week.

Comment 12

[:Paolo Amadini]

The functions should probably be split in different test-only modules, based on how specific the functions are to browser-chrome rather than mochitest or test suites in general. See also the recent discussion in bug 1121939.

For the general TestUtils.jsm module, see bug 946708.

Comment 13

[:Paolo Amadini]

Ah, most things that are still callback-based should be made Promise-based as part of the migration.

I also suggest factoring all things that require a "window" in a constructable object rather than having "window" as an argument. For example, if you're working with other windows you could do:

yield BrowserTestUtils.for(myWindow).waitForSomething("argument");

But normally you would just do something like:

yield BrowserWindowTestUtils.waitForSomething("argument");

With BrowserWindowtestUtils being initialized by the framework.

Comment 14

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #12)
> The functions should probably be split in different test-only modules, based
> on how specific the functions are to browser-chrome rather than mochitest or
> test suites in general. See also the recent discussion in bug 1121939.

I'd really really really prefer not to stick everything into $yetanotherjsm , and have to add the boilerplate for it everywhere.

We should just make opening windows/panels/tabs/content and waiting for that to finish standard things in the test framework just like waitForFocus.

Comment 15

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #14)
> I'd really really really prefer not to stick everything into $yetanotherjsm
> , and have to add the boilerplate for it everywhere.

Totally agree on not having to add boilerplate everywhere.

> We should just make opening windows/panels/tabs/content and waiting for that
> to finish standard things in the test framework just like waitForFocus.

Yeah, and we should share as much of that as possible in a way that is independent from the testing framework - things should work the same everywhere.

Comment 16

[Steven MacLeod [:smacleod]]

I'd like to finish up Bug 1107609 and make it part of this (As per Gijs' suggestion in Bug 1107609 Comment 2). Gijs et al, where would you like me to put 'spawnContent'?

Comment 17

[:Gijs (he/him)]

(In reply to Steven MacLeod [:smacleod] from comment #16)
> I'd like to finish up Bug 1107609 and make it part of this (As per Gijs'
> suggestion in Bug 1107609 Comment 2). Gijs et al, where would you like me to
> put 'spawnContent'?

I guess we can put it in a JSM that goes under testing/mochitest/...somewhere ? And then automatically include it from the browser test framework file (IIRC testing/mochitest//browser-test.js ).

Comment 18

[Steven MacLeod [:smacleod]]

I'm fixing up all our privatebrowsing tests to get them enabled in e10s right now, so since I need most of this functionality - I think I'm just going to go ahead an implement it.

Right now I have a BrowserTestUtils.jsm loaded in the test scope with a few "promise<Thing>()" methods (I'm thinking of loading this as "BTU" in the scope to make it shorter to type).

An issue I'm hitting though is I have no way to open windows from the JSM (without a reference to another window) since all of the suitable methods aren't in my scope. I could construct BTU with a reference to a window, and take Paolo's suggestion and allow BTU.for(window) overrides, or I could just run the utils code as a subscript like the SimpleTest stuff. Anyone have thoughts on this or preferences?

Comment 19

[:Gijs (he/him)]

I expect a lot of things that depend on windows won't necessarily be using the window in which SimpleTest runs, so using BTU.for(window).whatever seems sensible to me.

Comment 20

[:Gijs (he/him)]

(OTOH, if I'm passing in a node already (promisePanelHidden(panel) or whatever) then it probably makes sense to derive the window from there?)

Comment 21

[Steven MacLeod [:smacleod]]

(In reply to :Gijs Kruitbosch from comment #20)
> (OTOH, if I'm passing in a node already (promisePanelHidden(panel) or
> whatever) then it probably makes sense to derive the window from there?)

In cases where it's possible to derive the window ya I think we should go ahead and do it in the method. So, how about both.

Comment 22

[Steven MacLeod [:smacleod]]

Attachment: Patch 1 - Add a BrowserTestUtils module for sharing mochitest-browser test code v1

This is pretty early and WIP (only including a few methods), but I thought I'd throw it up early to get some opinions before heading too far down the rabbit hole.

Right now I've just been implementing the helpers as I need them for Bug 1132566.

Other than the code itself and how it's being implemented, does anyone have preferences for how this gets landed? I'd be happiest if we could get the base module setup with just a few methods landed, and then I can iterate as I / other require helpers for new tests.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8563573 [details] [diff] [review]
Patch 1 - Add a BrowserTestUtils module for sharing mochitest-browser test code v1

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

\o/

awesome to see this taking off.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +44,5 @@
> +   *
> +   * @return A Promise which resolves when a load event is triggered
> +   *         for browser.
> +   */
> +  promiseBrowserLoaded(browser, ignoreSubFrames=true) {

Nit: I'd prefer to not have negations in the parameter names when the default is true, IOW:

includeSubFrames=false

would make more sense to me. This also matches the default sense of "can I leave this param out" that JS still has in that non-sent parameters are normally undefined and therefore falsy.

@@ +95,5 @@
> + *
> + * @param win The window to wait for delayed startup to finish on.
> + *
> + * @param callback A function to be called when delayed startup has finished.
> + */

Nit: indent comment

::: testing/mochitest/browser-test.js
@@ +611,5 @@
>      this.currentTest.scope.SimpleTest = this.SimpleTest;
>      this.currentTest.scope.gTestPath = this.currentTest.path;
>      this.currentTest.scope.Task = this.Task;
>      this.currentTest.scope.ContentTask = this.ContentTask;
> +    this.currentTest.scope.BTU = new this.BTU();

Shouldn't you be passing the window reference here if you're actually invoking the constructor?

Comment 24

[:Paolo Amadini]

Comment on attachment 8563573 [details] [diff] [review]
Patch 1 - Add a BrowserTestUtils module for sharing mochitest-browser test code v1

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

Thanks for doing this!

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict"
> +
> +this.EXPORTED_SYMBOLS = [
> +  "BrowserTestUtils"

What I find neat about JSMs is that their name almost always matches what you have to put before the function you are calling.

For example, if I see a function call for NetUtil.newURI I know I can find its implementation and documentation quickly in a file called "NetUtil.jsm".

If we end up writing BTU.promiseBrowserLoaded in tests, it would be great for this to be called "BTU.jsm".

@@ +7,5 @@
> +  "BrowserTestUtils"
> +];
> +
> +const FRAME_SCRIPTS = [
> +  "chrome://mochikit/content/tests/BrowserTestUtils/content-utils.js",

Will we ever need more than one frame script in this module?

Will the frame script be specific to this JSM, or can we consider having a single "mochitest-browser-chrome" content script?

nit: we use lowercase directory names in the "testing" folder

less nit: we may consider a flatter directory structure (how exactly may depend on the answers to other questions)

@@ +30,5 @@
> +      run: function() {
> +        func();
> +      }
> +    }, Ci.nsIThread.DISPATCH_NORMAL);
> +};

We have helper functions for doing this all around the place.

We have promiseTopicObserved functions all around the place as well.

I would just create a TestUtils (or TU) module now exposing those helper functions, so we don't need to move them later.

@@ +68,5 @@
> +   *                 opened and loaded.
> +   *
> +   * @return The opened window.
> +   */
> +  whenNewWindowLoaded(options, callback) {

Since doing ".then(callback)" is trivial, we should have only the Promise-based version of the APIs here.

@@ +83,5 @@
> +    this.whenDelayedStartupFinished(win, () => callback(win));
> +    return win;
> +  },
> +
> +  promiseNewWindowLoaded(options) {

While promiseBrowserLoaded waits for an existing browser to be loaded, this actually opens a new browser window, so it should be called "openNewBrowserWindow".

As async action methods would normally return promises, the "promise" prefix should be avoided (i.e. "promiseThingDone", but "doThing").

You may see the "promise" prefix being sometimes substituted with "waitFor" as well.

::: testing/mochitest/browser-test.js
@@ +611,5 @@
>      this.currentTest.scope.SimpleTest = this.SimpleTest;
>      this.currentTest.scope.gTestPath = this.currentTest.path;
>      this.currentTest.scope.Task = this.Task;
>      this.currentTest.scope.ContentTask = this.ContentTask;
> +    this.currentTest.scope.BTU = new this.BTU();

nit: since scope.BTU and this.BTU are different things, you may want to export "BTUFactory" from the JSM instead, and I would be fine with the JSM being called "BTU.jsm" or "BTUFactory.jsm" (should be equally findable). But also calling everything BTU is fine, shouldn't create too much confusion, unless you want to import the module in a browser-chrome test to then use it on a different window.

Comment 25

[Steven MacLeod [:smacleod]]

(In reply to :Paolo Amadini from comment #24) 
> nit: we use lowercase directory names in the "testing" folder

There appear to be other directories here with uppercase letters and this directory has already landed in Bug 1107609. That, along with the fact changing this with my silly case-insensitive filesystem is a pain, makes me want to just leave it capitalized

Comment 26

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: bz://1093566/smacleod

/r/4155 - Bug 1093566 - Add modules for sharing mochitest-browser test code.
/r/4157 - Bug 1093566 - Add Ci and Cc to scope of spawned ContentTasks

Pull down these commits:

hg pull review -r 51d900b141d6647e11a01f1c41664ac0485e8bd5

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I completely forgot about this bug... I recently landed a BrowserUITestUtils.jsm:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/BrowserUITestUtils.jsm
Doesn't have much use yet (just browser/components/readinglist/ tests), so should be easy to migrate the one function there (waitForEvent) to the JSM here. See also bug 1134139.

I've very concerned by the naming of "BTU"/"BTUtils"/"TUtil" - that's very opaque to anyone not familiar with it, and I think will be another barrier to contributors.

Comment 28

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #27)
> I've very concerned by the naming of "BTU"/"BTUtils"/"TUtil" - that's very
> opaque to anyone not familiar with it, and I think will be another barrier
> to contributors.

+1 - could we go with longer and more descriptive names and shorten it where needed?
BrowserTestUtils and TestUtils doesn't seem that long.

Comment 29

[Steven MacLeod [:smacleod]]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #27)
> I completely forgot about this bug... I recently landed a
> BrowserUITestUtils.jsm:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> BrowserUITestUtils.jsm
> Doesn't have much use yet (just browser/components/readinglist/ tests), so
> should be easy to migrate the one function there (waitForEvent) to the JSM
> here. See also bug 1134139.

Ya easy enough, I'll migrate that over.

> I've very concerned by the naming of "BTU"/"BTUtils"/"TUtil" - that's very
> opaque to anyone not familiar with it, and I think will be another barrier
> to contributors.

Sure, let's go with BrowserTestUtils and TestUtils as Georg suggested.

Comment 30

[:Paolo Amadini]

Comment on attachment 8567775 [details]
MozReview Request: bz://1093566/smacleod

https://reviewboard.mozilla.org/r/4153/#review3409

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +Cc["@mozilla.org/globalmessagemanager;1"]
> +  .getService(Ci.nsIMessageListenerManager)
> +  .loadFrameScript(
> +    "chrome://mochikit/content/tests/BrowserTestUtils/content-utils.js", true);

Drive-by comment: strange we don't have something like "Services.globalmm" yet. (No need to add it now.)

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please take a look here for the standard JSM preamble:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/formautofill/FormAutofill.jsm

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +"use strict"

Semicolon

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +  "BTUtil"

Comma on last line of multi-line initializers

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +/**
> + * This module implements a number of utilities useful for browser tests.
> + *
> + * All asynchronous helper methods should return promises, rather than being
> + * callback based.
> + */

File comment goes right after license header, and with only one asterisk on first line, so that it's picked up by MXR.

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +const {utils: Cu, interfaces: Ci, classes: Cc} = Components;

Standard line looks exactly like this:

const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

(All the four shortcuts should be included even if not used, in case we copy code from somewhere else)

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm");

No need to lazy load Services.jsm.

In general, I think we can avoid lazy getters in test-only modules, but having them won't hurt. In that case please use standard indentation.

::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
(Diff revision 1)
> +   * @param browser A xul:browser.
> +   *
> +   * @param includeSubFrames A boolean indicating if loads from subframes
> +   *                         should be included.

I'm going to do less indentiation nitpicking on the rest of the file, I'd just note that @param description on the next line is better:

   * @param browser
   *        A "xul:browser" element.
   * @param includeSubFrames
   *        A boolean...

::: testing/mochitest/browser-test.js
(Diff revision 1)
> +  this.BTUtil = Components.utils.import("resource://testing-common/BTUtil.jsm", null).BTUtil;
> +  this.TUtil = Components.utils.import("resource://testing-common/TUtil.jsm", null).TUtil;

Ok, having TestUtils and BrowserTestUtils with global methods seems like a good start.

In the future we may have BrowserTestUtils.for(window).method(...) with different browser-specific methods, if it turns out we often operate on new browser windows.

::: testing/mochitest/BrowserTestUtils/content/content-task.js
(Diff revision 1)
> -let Cu = Components.utils;
> +const {utils: Cu, interfaces: Ci, classes: Cc} = Components;

Use standard line here as well.

::: testing/mochitest/BrowserTestUtils/content/content-utils.js
(Diff revision 1)
> +let Cu = Components.utils;

And here.

::: testing/modules/TUtil.jsm
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Use standard JSM preamble.

::: testing/modules/TUtil.jsm
(Diff revision 1)
> +  executeSoon(func) {

I guess executeSoon is the best name, given we're all familiar with it. Technically it would be runOnNextTick or something.

::: testing/modules/TUtil.jsm
(Diff revision 1)
> +    Services.tm.mainThread.dispatch({
> +      run: function() {
> +        func();
> +      }
> +    }, Ci.nsIThread.DISPATCH_NORMAL);

nit: argument can be called "callbackFn".

nsIRunnable has the @function annotation. Just do:

Services.tm.mainThread.dispatch(callbackFn, Ci.nsIThread.DISAPTCH_NORMAL);

::: testing/modules/TUtil.jsm
(Diff revision 1)
> +  observedTopic(topic, subject=null) {

We have "browserLoaded" above and we have "observedTopic" here.

We should aim for consistency. I think these should be "browserLoaded" and "topicObserved", but maybe the other way around sounds better?

::: testing/modules/TUtil.jsm
(Diff revision 1)
> +        TUtil.executeSoon();

Leftover?

Comment 31

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #30)
> Comment on attachment 8567775 [details]
> MozReview Request: bz://1093566/smacleod
> ::: testing/mochitest/BrowserTestUtils/BTUtil.jsm
> (Diff revision 1)
> > +const {utils: Cu, interfaces: Ci, classes: Cc} = Components;
> 
> Standard line looks exactly like this:
> 
> const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> 
> (All the four shortcuts should be included even if not used, in case we copy
> code from somewhere else)

Why the extra spacing, and why does the ordering matter? This is the first I've heard that we have a "standard" for this - we use various versions of this incantation throughout the tree, and I don't see why one is really better than the other (besides including all 4 things). We should really just get bug 1135220 fixed instead, but that depends on some xpconnect intricacies.

Comment 32

[:Gijs (he/him)]

Comment on attachment 8567775 [details]
MozReview Request: bz://1093566/smacleod

Clearing per IRC discussion yesterday.

FWIW... personally I would even be happy to see the utility functions available in the global scope as they usually are now, and so use a destructuring assignment or some other magic to just ensure that the test scope has all the functions/methods the BrowserTestUtils thing has.

Comment 33

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #31)
> Why the extra spacing, and why does the ordering matter?

I don't mind about the spacing of braces, different styles are equally represented. With regard to the order, I slightly prefer to copy-and-paste an existing string rather than come up with an arbitrary one.

> (besides including all 4 things)

This is the main point.

> We should really just get bug 1135220 fixed instead

Yeah, I was aware there was some work going on.

Comment 34

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #32)
> FWIW... personally I would even be happy to see the utility functions
> available in the global scope as they usually are now, and so use a
> destructuring assignment or some other magic to just ensure that the test
> scope has all the functions/methods the BrowserTestUtils thing has.

I strongly prefer a style like "NetUtil.newURI(var)" than "uri(var)". With the latter, I don't know where to look for the function. Is it defined at the beginning of this test, in the head.js file, in the JS engine, or is it a pre-loaded global only available in this framework? Is that a function that I can just reuse if I move the code to another framework like xpcshell, or not?

When we had just a few support functions (like ok and is) and very locally contained tests, we could know this by heart. executeSoon is very familiar to us (probably less to new contributors). getTestFileURL and getTestFilePath are already less familiar. Do you think people know which one is in which framework? I spend time looking that up when I need one of them, all the time.

Today the number of support functions we would need is just so high that it cannot reasonably be expected to be kept in mind even after studying all the documentation. And also, it's unlikely that documentation will keep up with the number of functions added. I'd rather not need to do an MXR/DXR search for a function name (with potentially ambiguous results) every time, but I'd rather promptly know which module or framework I should look up (for example NetUtil.jsm or BrowserTestUtils.jsm).

Comment 35

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4153/#review3553

> Please take a look here for the standard JSM preamble:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/formautofill/FormAutofill.jsm

Those preambles are identical?

Comment 36

[Steven MacLeod [:smacleod]]

Comment on attachment 8567775 [details]
MozReview Request: bz://1093566/smacleod

/r/4155 - Bug 1093566 - Add modules for sharing mochitest-browser test code.
/r/4157 - Bug 1093566 - Add Ci and Cc to scope of spawned ContentTasks
/r/4361 - Bug 1093566 - Migrate BrowserUITestUtils into BrowserTestUtil.

Pull down these commits:

hg pull review -r 37b51f4440aee2da356a7632c8680ddb8a6c9104

Comment 37

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/4155/#review3561

Ship It!

::: testing/mochitest/BrowserTestUtils/BrowserTestUtil.jsm
(Diff revision 2)
> +      let win = Services.ww.openWindow(
> +      null, Services.prefs.getCharPref("browser.chromeURL"), "_blank",

This indenting is wonky. Tabs or something?

Comment 38

[:Gijs (he/him)]

Comment on attachment 8567775 [details]
MozReview Request: bz://1093566/smacleod

https://reviewboard.mozilla.org/r/4153/#review3563

Ship It!

Comment 39

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/4157/#review3559

::: testing/mochitest/BrowserTestUtils/content/content-task.js
(Diff revision 2)
> -let Cu = Components.utils;
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

Uber-nit: fix the commit message to include 'results'. :-)

Comment 40

[:Paolo Amadini]

Comment on attachment 8567775 [details]
MozReview Request: bz://1093566/smacleod

https://reviewboard.mozilla.org/r/4153/#review3565

r+ with one change.

::: testing/mochitest/browser-test.js
(Diff revision 2)
> +  this.BrowserTestUtil = Components.utils.import("resource://testing-common/BrowserTestUtil.jsm", null).BrowserTestUtil;
> +  this.TestUtil = Components.utils.import("resource://testing-common/TestUtil.jsm", null).TestUtil;

I'd have expected these to be BrowserTestUtils and TestUtils. The "Util" variant is way less popular:

http://mxr.mozilla.org/mozilla-central/find?string=Utils.jsm
http://mxr.mozilla.org/mozilla-central/find?string=Util.jsm

Comment 41

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8567775 [details]
MozReview Request: bz://1093566/smacleod

https://reviewboard.mozilla.org/r/4153/#review3583

What Paolo said.

Comment 42

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/integration/fx-team/rev/fdbe5abe8675
https://hg.mozilla.org/integration/fx-team/rev/ec936c510299
https://hg.mozilla.org/integration/fx-team/rev/be473722fb84

Comment 43

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

https://hg.mozilla.org/mozilla-central/rev/fdbe5abe8675
https://hg.mozilla.org/mozilla-central/rev/ec936c510299
https://hg.mozilla.org/mozilla-central/rev/be473722fb84

Comment 44

[Ryan VanderMeulen [:RyanVM]]

I backported this to Aurora as well to make test fixes easier to backport throughout the duration of the esr38 lifecycle.
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e5d503cd9c9
https://hg.mozilla.org/releases/mozilla-aurora/rev/1888d98f1d6f
https://hg.mozilla.org/releases/mozilla-aurora/rev/8771d218e0e1

Comment 46

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1093566 - Migrate BrowserUITestUtils into BrowserTestUtil.

Comment 47

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1093566 - Add modules for sharing mochitest-browser test code.

Comment 48

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1093566 - Add Ci and Cc to scope of spawned ContentTasks