Alexandre Poirot [:ochameau] Assignee
	Description • 11 years ago

Often, when writting a mochitest-plain, you need some limited and shared piece of chrome code. So far, the practice has been to add some specifics to SpecialPowers, like setAllAppsLaunchable, autoConfirmAppInstall, addAutoCompletePopupEventListener, attachFormFillControllerTo, getMaxLineBoxWidth ... and so on.

Each of these methods are specific to one particular kind of tests. That doesn't really scale.

One alternative, is to use Cc and Ci exposed by SpecialPowers, but then you get some proxy wrappers that introduce many hard to debug and unexpected issues.

So for the sake of testing properly webapps actor and after many iterations I ended up implementing an easy way to evaluate chrome scripts and communicate with them from the mochitest.

### mochitest.html

// Load a javascript file with chrome priviledges
// and instanciate a communication channel with it
var script = SpecialPowers.loadChromeScript(window, "myChromeScript.js");

// Listen for messages sent by the chrome script
script.addMessageListener("bar", function (message) {
  // message == { bar: "foo" }
  script.destroy();
  SimpleTest.finish();
});

// Send a message to the chrome script
script.sendAsyncMessage("from-mochitest-to-chrome", {foo: "bar"});

### myChromeScript.js
// Listen for message sent by the mochitest
addMessageListener("from-mochitest-to-chrome", function (message) {
  // Send a message to the mochitest
  sendAsyncMessage("bar", {bar: "foo"});
});


And the very cool thing is that as it uses messageManager and asynchronous messages, it does even work on b2g!!

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 11 years ago

That ends up being easier passing an absolute URL to SpecialPowers.loadChromeScript.
(Instead of letting it call SimpleTest.getTestFile) as specialpowersAPI doesn't have
access to SimpleTest.

	Alexandre Poirot [:ochameau] Assignee
	Comment 3 • 11 years ago

So instead of:
var script = SpecialPowers.loadChromeScript(window, "myChromeScript.js");

we should now do:
var url = SimpleTest.getTestFileURL("myChromeScript.js");
var script = SpecialPowers.loadChromeScript(url);

	(not currently active) Ted Mielczarek
	Comment 4 • 11 years ago

This is an interesting idea. My only worry is that by creating a new type of chrome scope specifically for this it people will be a bit confused about what they can or can't do in that scope.

	Bobby Holley (:bholley)
	Comment 5 • 11 years ago

I've sometimes used SpecialPowers.Cu to create a chrome-privileged sandbox. This looks similar, but with a better API.

I don't think it'll be too confusing though. It looks nicely sandboxed, so it's basically just an async channel to a chrome actor that can't otherwise interact with the content scope.

	Alexandre Poirot [:ochameau] Assignee
	Comment 6 • 11 years ago

Comment on attachment 802360 [details] [diff] [review]
Switched from SpecialPowers.getTestFile to SimpleTest.getTestFile.

Bobby, would you be able to review this patch? (Joel told me he isn't confident reviewing this one)

(I'm trying to get this patch and the related ones in F26, so I'm looking forward a review sometimes soon :o)

	Bobby Holley (:bholley)
	Comment 7 • 11 years ago

Comment on attachment 802360 [details] [diff] [review]
Switched from SpecialPowers.getTestFile to SimpleTest.getTestFile.

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

The Sandbox stuff looks good. For the rest of the API design though I think we need an automation peer. Flagging ted.

I think it would also be nice to give the chrome scripts a convenience mechanism to call ok() and is() (perhaps asynchronously over the message channel).

	Alexandre Poirot [:ochameau] Assignee
	Comment 8 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #7)
> Comment on attachment 802360 [details] [diff] [review]
> Switched from SpecialPowers.getTestFile to SimpleTest.getTestFile.
> 
> Review of attachment 802360 [details] [diff] [review]:
> -----------------------------------------------------------------
> I think it would also be nice to give the chrome scripts a convenience
> mechanism to call ok() and is() (perhaps asynchronously over the message
> channel).

I was tempted to do that in that patch, but I didn't. I first wanted to address the issue of SpecialPowers being polluted by more and more test specific helpers. I'd be happy to add such additional helper, for me it is common to have to test both content and chrome sides at the same time.

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 11 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> This is an interesting idea. My only worry is that by creating a new type of
> chrome scope specifically for this it people will be a bit confused about
> what they can or can't do in that scope.

In its current shape, it is mostly to stop adding helpers directly into SpecialPowers.
But as Bobby highlighted, it can also be a handy way to execute and also do some sanity checks on chrome side, while being compatible with b2g and OOP platforms.

Otherwise, the context of these sandboxes is quite simple. It is an empty chrome sandbox, living in parent process, that has regular access to Components.* and only two specifics: addMessageListener and sendAsyncMessage.

	(not currently active) Ted Mielczarek
	Comment 10 • 11 years ago

Comment on attachment 802360 [details] [diff] [review]
Switched from SpecialPowers.getTestFile to SimpleTest.getTestFile.

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

This mostly looks good, I just have a few nits and a qustion.

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +295,5 @@
> +                                 .getService(Ci.nsIScriptableInputStream);
> +        var channel = Services.io.newChannel(url, null, null);
> +        var input = channel.open();
> +        scriptableStream.init(input);
> +        var jsScript = scriptableStream.read(input.available());

Yuck, sync I/O. :-(

@@ +330,5 @@
> +        var name = aMessage.json.name;
> +        var message = aMessage.json.message;
> +        this._chromeScriptListeners
> +            .filter(function (o) o.name == name && o.id == id)
> +            .forEach(function (o) o.listener(message));

=> ?

::: testing/specialpowers/content/specialpowersAPI.js
@@ +513,5 @@
> +      },
> +
> +      removeMessageListener: (name, listener) => {
> +        listeners = listeners.filter(
> +          function (o) (o.name != name || o.listener != listener)

Any reason you didn't use a => here as well?

@@ +518,5 @@
> +        );
> +      },
> +
> +      sendAsyncMessage: (name, message) => {
> +        this._sendSyncMessage("SPChromeScriptMessage",

How is "this" the right thing here? Don't you need to be saving it in a different name from above?

@@ +524,5 @@
> +      },
> +
> +      destroy: () => {
> +        listeners = [];
> +        this._removeMessageListener("SPChromeScriptMessage", chromeScript);

Also the "this" here.

@@ +536,5 @@
> +        if (messageId != id)
> +          return;
> +
> +        listeners.filter(function (o) o.name == name)
> +                 .forEach(function (o) o.listener(this.wrap(message)),

=> here?

	Alexandre Poirot [:ochameau] Assignee
	Comment 13 • 11 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> Comment on attachment 802360 [details] [diff] [review]
> Switched from SpecialPowers.getTestFile to SimpleTest.getTestFile.
> 
> Review of attachment 802360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This mostly looks good, I just have a few nits and a qustion.
> 
> ::: testing/specialpowers/content/SpecialPowersObserverAPI.js
> @@ +295,5 @@
> > +                                 .getService(Ci.nsIScriptableInputStream);
> > +        var channel = Services.io.newChannel(url, null, null);
> > +        var input = channel.open();
> > +        scriptableStream.init(input);
> > +        var jsScript = scriptableStream.read(input.available());
> 
> Yuck, sync I/O. :-(

I kept it, otherwise I would have to complexify the final API.
I assume that when I call SpecialPowers.loadChromeScript, I can immediatly send messages to it.
If we load the script asynchronously, we would either had to save incoming messages received during script load, or complexify the API by dispatching a "ready" or "load" event, that would have to be listen for before sending any message.
If that's something you feel very wrong, would you be ok going for the "save incoming messages during load" in a followup?

> 
> ::: testing/specialpowers/content/specialpowersAPI.js
> @@ +513,5 @@
> > +      },
> > +
> > +      removeMessageListener: (name, listener) => {
> > +        listeners = listeners.filter(
> > +          function (o) (o.name != name || o.listener != listener)
> 
> Any reason you didn't use a => here as well?

No particular reason, I tend to use arrow function mainly when we need to bind its `this`.
(see next comment)

> 
> @@ +518,5 @@
> > +        );
> > +      },
> > +
> > +      sendAsyncMessage: (name, message) => {
> > +        this._sendSyncMessage("SPChromeScriptMessage",
> 
> How is "this" the right thing here? Don't you need to be saving it in a
> different name from above?

Arrow functions aren't just about an even shorter way to write closures,
it also ease this typical usecase. For each arrow function, its `this` is automagically
binded to the current lexical scope `this`.
So `() => {}` is similar to `(function () {}).bind(this)`
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/arrow_functions#Lexical_this
So that, here, `this` from `this._sendSyncMessage` will be equal to the SpecialPowerAPI instance.

	(not currently active) Ted Mielczarek
	Comment 14 • 11 years ago

Comment on attachment 804367 [details] [diff] [review]
Convert all closures to arrow functions.

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

I didn't know that about arrow functions. Nice!

http://stream1.gifsoup.com/view/204031/the-more-you-know-o.gif

You don't have to fix the sync I/O problem, it's just a visceral reaction. This is test-only code, it's not the end of the world.

	Alexandre Poirot [:ochameau] Assignee
	Comment 15 • 11 years ago

Thanks for the fast review cycles!

A try run, just to be sure before landing:
https://tbpl.mozilla.org/?tree=Try&rev=24c677b706da

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/986c3e80530c

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/986c3e80530c

	Martijn Wargers (dead)
	Comment 18 • 11 years ago

I would love to use loadChromeScript for bug 871323, but I need a reference to the window in the chrome script. That doesn't seem possible, is there a way to have that supported?

	Martijn Wargers (dead)
	Comment 19 • 11 years ago

Never mind, found a solution.