653594 - Problem with Panel's 'port' property

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Will Bamberg [:wbamberg]]

Taking the following trivial add-on:

***
var panel = require("panel").Panel({
  contentScript: "self.postMessage('showing');"
});

panel.on("message", function() {
  console.log("panel is showing");
});

panel.show();
***

If I try to change it to use 'port' as follows:

***
var panel = require("panel").Panel({
  contentScript: "self.port.emit('showing');"
});

panel.port.on("showing", function() {
  console.log("panel is showing");
});

panel.show();
***

I get this:

***
error: An exception occurred.
Traceback (most recent call last):
  File "resource://jid0-z6c2yzbah2l9exwgyvktsf9ubhg-api-utils-lib/content/worker.js", line 363, in
    get port() this._port._public,
TypeError: this._port is null
FAIL
***

A similar transformation but using page-mod works as expected.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Fix Panel.port and add unit tests.

This was due to the fact that Panel doesn't call Worker constructor immediatly,
so `port` is still not initialized when we try to access it.

Here is one way to solve this problem.
We have almost the same problem with postMessage. postMessage doesn't work until Worker constructor is called. Actually, it throw an exception saying that it has been destroyed.

With this patch I solved both: port is now defined AND we can emit an event as soon as we want! (Things that we can't do with postMessage)

Comment 2

[Will Bamberg [:wbamberg]]

Thanks Alex! On a related note, over in bug 649629 comment 4 Myk talks about setting up event listeners in the constructor: is this possible, using something like the syntax he suggests there? (Or rather: will it be possible, when this bug is fixed?)

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to comment #2)
We can implement this syntax quite easily. This is unrelated to main bug 649629 issue that is specific to Widget AP. So we can work on syntax in parallel of Widget port attribute addition.
It could be a good idea to fill a specific bug for this enhancement.

Comment 4

[Will Bamberg [:wbamberg]]

(In reply to comment #3)
> (In reply to comment #2)
> We can implement this syntax quite easily. This is unrelated to main bug 649629
> issue that is specific to Widget AP. So we can work on syntax in parallel of
> Widget port attribute addition.

Sure. It just came up as I was looking at explaining how to migrate from the old style.

> It could be a good idea to fill a specific bug for this enhancement.

-> bug 653629

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 529000 [details] [diff] [review]
Fix Panel.port and add unit tests.

Review of attachment 529000 [details] [diff] [review]:

Looks good, works well, r=myk!

::: packages/addon-kit/tests/test-panel.js
@@ +28,5 @@
+  test.waitUntilDone();
+  let panel = Panel({
+    contentURL: "about:buildconfig",
+    contentScript: "self.port.emit('loaded');" +
+                   "self.port.on('addon-to-content', "+

Nit: space before plus sign.

@@ +46,5 @@
+tests.testPanelEmitEarly = function(test) {
+  test.waitUntilDone();
+  let panel = Panel({
+    contentURL: "about:buildconfig",
+    contentScript: "self.port.on('addon-to-content', "+

Nit: space before plus sign.

::: packages/api-utils/lib/content/worker.js
@@ +470,5 @@
+    this._inited = true;
+    
+    // Flush all events that has been fired before the worker is initialized.
+    let self = this;
+    this._earlyEvents.forEach(function (args) self._emitEventToContent(args));

Nit: it might be better to start using .bind in these situations to avoid the extra variable definition, i.e.:

    this._earlyEvents.forEach((function (args) this._emitEventToContent(args)).
                              bind(this));

Comment 6

[Myk Melez [:myk] [@mykmelez]]

a=myk for commission during freeze.

Comment 7

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/2af17628677164d679b860366f24e776b5b7f4b0