649629 - Add EventEmitter API to widgets

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

As bug 635748 is going to land, we will have EventEmitter API in almost all content scripts except Widgets. Because widgets are not a composition of a Symbiont/Worker, but deleguate to a Symbiont.
In order to ease documentation and global use of content scripts, we may provide exactly same API to widgets.

This undercover one side effect of bug 635748: workers events are mixed with show/hide events in Panel. And if we do exactly the same in Widgets, they are going to be mixed with click, mouseover, mouseout.

To avoid this, we may provide a new attribute, `worker` for ex., that has an EventEmitter API.

Your thoughts ?

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

As I expressed in Bug 635748 I think it's fine to allow emitting events we just need to make sure that they can't emit our reserved events, by throwing exceptions when they try to do so.

Alternative option can be requiring users to use predefined prefix so that they can only emit "custom:show" this will guarantee that there will be no conflicts with an events we may decide to add in the future.

Comment 2

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

(In reply to comment #1)
> As I expressed in Bug 635748 I think it's fine to allow emitting events we just
> need to make sure that they can't emit our reserved events, by throwing
> exceptions when they try to do so.

The problem with this approach, as you note, is that adding new events to the Panel and Widget objects will break addons that emit those events, violating the principle that API additions be backward-compatible with addons developed using an older version of the SDK.


> Alternative option can be requiring users to use predefined prefix so that they
> can only emit "custom:show" this will guarantee that there will be no conflicts
> with an events we may decide to add in the future.

Blech.


I think we should avoid this problem for Panel, Widget, and for that matter Worker by implementing the EventEmitter-based communications channel we're implementing over in bug 635748 on a new `sink` (alternately, `port`) property of workers, including both explicit Worker objects and objects like Panel and Widget that are workers through composition.

That way we avoid all potential conflicts with Panel and Widget events as well as any potential conflicts with Worker-specific events (like the new "detach" event we're adding over in bug 607601).

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Yeah after looking at the code using it I really dislike it :(.

One thing that we loose is ease of registering listeners through properties of the constructor:

new PageMod({
  onAttach: ...
  onMyEvent: ...
  onSomethingElse: ...
  ....
});


With a proposed API one will have to do this instead:

var mod = new PageMod({
  onAttach: ...
  ...
});
mod.port.on("myEvent", ....
mod.port.on("somethingElse", ...
mod.on("detach", ...

That is inconvenience.

After considering this once again I'm not sure if it's actually worth disallowing to emit attach / detach or other standard events. Thing is that code emitting and consuming events will be from the same add-on and if they decide to emit conflicting event it's there choice. Also we can recommend to using a prefix to avoid conflicts with events that will be added in the future.

Comment 4

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

(In reply to comment #3)
> One thing that we loose is ease of registering listeners through properties of
> the constructor:
> 
> new PageMod({
>   onAttach: ...
>   onMyEvent: ...
>   onSomethingElse: ...
>   ....
> });
> 
> 
> With a proposed API one will have to do this instead:
> 
> var mod = new PageMod({
>   onAttach: ...
>   ...
> });

We should address this issue by adding a `sink` property to worker constructors for specifying content script event listeners at construct-time:

  let mod = new PageMod({
    onAttach: ...,
    sink: {
      onMyEvent: ...
      onSomethingElse: ...
    },
    ....
  });
  ...
  mod.sink.on("aThirdThing", ...);


> After considering this once again I'm not sure if it's actually worth
> disallowing to emit attach / detach or other standard events. Thing is that
> code emitting and consuming events will be from the same add-on and if they
> decide to emit conflicting event it's there choice. Also we can recommend to
> using a prefix to avoid conflicts with events that will be added in the future.

A prefix is similar to a property: it namespaces the content script events, segregating them from events generated by the worker itself.  But a prefix is arbitrary, optional, and incompletely segregated, which makes its use more complex and uncertain.

We are a defining a communications channel for addons to use to communicate between different parts of their codebase, and even though we use the EventEmitter interface, these are not the same kind of events as those we define on the objects we expose through our high-level APIs, and it's worth requiring their complete segregation.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Work in progress

I'm not able to make more progress on this patch.
The only issue left is an error on widget.destroy() that say: "this._window is undefined", in Worker.constructor.
If someone want to take this bug in order to land it for 1.0b5, it is welcomed!

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Implement widget.port

I was able to fix previous exception.
It was due to early destroy of worker object, during its constructor!

From my list of bugs, this is the last bug that have to be in 1.0b5 from my point of view.

It is really important to have same features over all content scripts APIs, in order to provide a clear and simple documentation without exception/corner case.

Comment 7

[Alexandre Poirot [:ochameau]]

I forgot to mention that this patch require patch from bug 653594 to be applied!

Comment 8

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

Comment on attachment 529070 [details] [diff] [review]
Implement widget.port

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

I see test failures when running widget tests after applying this patch (as well as the patch from bug 653594):

(addon-sdk)myk@myk:~/Projects/addon-sdk/packages/addon-kit$ cfx test -f test-widget --verbose
...
info: executing 'test-widget.testConstructor'
info: pass: panel has correct number of child elements after widget construction
info: pass: widget has correct default height
error: TEST FAILED: test-widget.testConstructor (exception)
error: An exception occurred.
Traceback (most recent call last):
TypeError: setting a property that has only a getter
...
info: executing 'test-widget.testWidgetViewsCustomEvents'
info: pass: event argument is valid on WidgetView
info: pass: event argument is valid on Widget
error: An exception occurred.
Traceback (most recent call last):
TypeError: setting a property that has only a getter
...
error: TEST FAILED: test-widget.testWidgetViewsCustomEvents (timed out)
info: executing 'test-widget.testWidgetViewsTooltip'
error: TEST FAILED: test-widget.testWidgetViewsTooltip (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://addon-kit-addon-kit-lib/widget.js", line 360, in Widget
    w._initWidget(options);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 253, in _initWidget
    browserManager.validate(this);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 522, in 
    throw new Error("This widget ID is already used: " + item.id);
Error: This widget ID is already used: foo
...
info: executing 'test-widget.testWidgetMove'
error: TEST FAILED: test-widget.testWidgetMove (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://addon-kit-addon-kit-lib/widget.js", line 360, in Widget
    w._initWidget(options);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 253, in _initWidget
    browserManager.validate(this);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 522, in 
    throw new Error("This widget ID is already used: " + item.id);
Error: This widget ID is already used: foo
...
14 of 21 tests passed.

The following tests failed:
  test-widget.testConstructor: exception, timed out
  test-widget.testWidgetViewsCustomEvents: timed out
  test-widget.testWidgetViewsTooltip: exception, timed out
  test-widget.testWidgetMove: exception, timed out

FAIL
Total time: 62.929587 seconds
Program terminated unsuccessfully.

::: packages/addon-kit/lib/widget.js
@@ +329,5 @@
   },
   
+  get port() this._port._public,
+  set port(v) {}, // Work around Cortex failure with getter without setter
+                  // See bug 653464

I wonder if it would be better to throw a "setting a property that only has a getter" exception (which is what normally happens when you try to set a property with only a getter in strict mode) instead of just silently failing (which is what happens in non-strict mode), given that we encourage developers to "use strict;".

I guess it doesn't really matter, since we'll fix the Cortex bug, and then the right behavior will happen automagically.

@@ +442,5 @@
+    basePort._emit.apply(basePort, args);
+  },
+  
+  get port() this._port._public,
+  set port(v) {},

Nit: also put a note here about this being a workaround for the Cortex bug, to make sure we don't forget to fix it when we fix that bug.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Same patch but with exception fixed.

These exception was due to the port getter hack.
I'm replacing port during the first time we try to access this property.
I was only replacing port attribute on _public, the public object built by Traits,
but I have to replace it on the private API too.
So here is the only difference from the previous patch:
worker.js, get port()
    delete this._public.port;
    this._public.port = Cortex(this._port);
+    // Replicate public port to the private object
+    delete this.port;
+    this.port = this._public.port;

Comment 10

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

Comment on attachment 529167 [details] [diff] [review]
Same patch but with exception fixed.

Looks good, thanks for the quick response! r,a=myk

Comment 11

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

https://github.com/mozilla/addon-sdk/commit/6b336f64406940877070b7fa506f858a5a0280b7