635748 - add EventEmitter interface to content scripts

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

This feature has been ask multiple times
Roundtable:
  https://wiki.mozilla.org/Labs/Jetpack/Weekly_Meeting/2011-01-04
Group:
  http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/190f912bc9be740c
Recent irc discussion about that between mcepl, brian and Zarutian, refering to such code:
  http://gitorious.org/addon-sdk/bugzilla-triage-scripts/blobs/pageMod/data/bzpage.js

Facts are that simple message passing pattern lead to old win32 programming with giant switch/case function in Event loop function!

Having said that, there is known solutions like :
* message handler:  
we would call functions coming from a dictionnary (or an object in JS words). That would replace the whole big switch by a big object, but it will enhance readability.
 postMessage( {name:"myEvent", data:123} );
 -->
 pageMod({
   messageHandler = { 
     myEvent: function (msg) {
       msg.data == 123;
     }
   }
 });


* message with names and listeners:
This is more like DOM events listener than DOM postMessage.
You send messages with a name and data instead of only data.
  postEvent("name", 123);
  ->
  let pm = pageMod();
  pm.addListener("myEvent", function (msg) {
    msg.name == "myEvent";
    msg.data == 123;
  });

We may even mix events with message handler, by passing a messageHandler attribute in pageMod constructor.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: One proposal: messageHandler 100% compatible with current postMessage behavior

There is tons of differents API to write on this subject, but here is one that try to be most compatible with current API and try to minimize new stuff added to the Worker api.

Here is an example:
Worker({
 contentScript: 'new ' + function WorkerScope() {
   postMessage({ name: 'myEvent', data: 'my event data' });
 },
 messageHandler: {
   myEvent: function (msg) {
     test.assert(gotOnMessageFirst, "got onMessage called before messageHandler");
     test.assertEqual('myEvent', msg.name);
     test.assertEqual('my event data', msg.data);
   }
 }
});

1/ This let postMessage "as is",
2/ onMessage is still called whatever there is a messageHandler or not,
3/ You have to send objects with a name attribute in order to use messageHandler.

Alternatives could be to:
* add another method than postMessage with a name and a data arguments,
* Do not call onMessage when there is a matching messageHandler,
* dispatch these names messages to EventEmitter in order to use "on" function to catch them,
* ...

Comment 2

[Daniel Buchner [:dbuc]]

In order to create a more prominent visual and conceptual link between the messagHandler function declarations and the postMessage invocation of those functions from the content side, we should consider changing the content-side 'name' attribute to 'handler' instead.

I am willing to bet that using 'handler' vs 'name' will be easier to understand when developers view the documentation on this feature. Thoughts?

Other than that, the code looked simple enough :)

Comment 3

[Daniel Buchner [:dbuc]]

Comment on attachment 514045 [details] [diff] [review]
One proposal: messageHandler 100% compatible with current postMessage behavior

I already posted this feedback is the bug's comments. If you've already seen it there, just ignore this ;)

---

In order to create a more prominent visual and conceptual link between the
messagHandler function declarations and the postMessage invocation of those
functions from the content side, we should consider changing the content-side
'name' attribute to 'handler' instead.

I am willing to bet that using 'handler' vs 'name' will be easier to understand
when developers view the documentation on this feature. Thoughts?

Other than that, the code looked simple enough :)

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to comment #3)
Daniel, I saw you discussion with Brian on irc, and I'm copying it there in order to expose all thoughts on this subject in bugzilla.

So you propose an API with explicit names on message:
http://paste.mootools.net/f2e0b0481
This is something close to what I suggest in first comment as :
  "* message with names and listeners"
but you added a way to retrieve handler returned value.
I think that if we add a new API, we may pass the callback for returned value directly in fireCallback method and moreover having a symetric API on both sides.


Having said that I agree with Irakli in this bug: 
https://bugzilla.mozilla.org/show_bug.cgi?id=636151
These many differents flavors to communicate hides a need to let user choose his best communication pattern on top of the low-level core IPC.

My initial thought was to have same API used by firefox for e10s that is simple, web style (like dom events) and still light (easily use for message passing across processes):
https://developer.mozilla.org/en/The_message_manager

But I first proposed a patch for something that was 100% compatible with current API.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Use EventEmitter interface: on/emit

Here is another really simple patch that simply add emit functions in both sides:
  let worker =  Worker({
      window: window,
      contentScript: 'new ' + function WorkerScope() {
        // #2 receive event from addon
        on('addon-to-content', function (data) {
          // #3 send an event back to addon
          emit('content-to-addon', data);
        });
      }
    });
  
  // #4 receive the event from content
  worker.on('content-to-addon', function (data) {
    // data = 'event data'
  });

  // #1: sent an event to content
  worker.emit('addon-to-content', 'event data');


This solution is more disruptive as it adds a new function (emit) that would quickly deprecate postMessage, as it's simply more handy.

I didn't propose a "message handler" pattern here, as it is not really necessary. We may prefer do multiple worker.on/self.on in order to avoid "Win32 big switch case" effect encouraged by message handler pattern.

Again my willing here is not to propose some high level API but only upgrade from postMessage pattern that is really too dump and annoying in real world usages:
http://gitorious.org/addon-sdk/bugzilla-triage-scripts/blobs/93bd458f3f60b47ccb4f1d1d0f2688b267eab55f/data/lib/bzpage.js#line23

Comment 6

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

Just wanted to mention that I really like this proposal as it's very simple & flexible, also solves problems people raised. In addition it's much more consistent with other APIs.

Also note that (I don't know why, but) context module already uses this / similar API:
https://jetpack.mozillalabs.com/sdk/1.0b3/docs/packages/addon-kit/docs/context-menu.html

That being said, I'd prefer having self.emit / self.on instead of global self & on.

I'm +1 on this one.

Comment 7

[Daniel Buchner [:dbuc]]

I have no issues with the mechanics here, looks pretty good. One thing I do have a different opinion about is the injection of one-off items into a page mod's content scope.

Whatever verbs are chosen to stand for these actions should be accessed on a scoped object that stands for the module instance, like pagemod.on or addon.emit.

I would caution the route of injecting YARV (Yet Another Random Variable) into the scope that isn't namespaced. As with any single-level-depth proposal, it always looks good if you never go beyond a few, then it looks like a scope mess.

Another advantage for developers is that they know "Hey all the interaction points these folks will ever offer me can be found here [on the pagemod/addon object]". This means that even if the developer overlooks something in the docs or the docs for an item forget to include something, it is a simple matter of for in looping the known touchpoint and checking out all the properties.

Let me know what you think.

Comment 8

[Daniel Buchner [:dbuc]]

(In reply to comment #6)

> That being said, I'd prefer having self.emit / self.on instead of global self &
> on.

I didn't see that part of Irakli's comment, I 100% agree!

Comment 9

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

Comment on attachment 519922 [details] [diff] [review]
Use EventEmitter interface: on/emit

Irakli and I talked through this proposal last week, and I think it's the right next step in the refinement of the mechanisms by which addons communicate with their content scripts.  As Irakli and Daniel suggest, however, the `on` and `emit` functions should be methods of the `self` global rather than globals in their own right.

Comment 10

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

Comment on attachment 514045 [details] [diff] [review]
One proposal: messageHandler 100% compatible with current postMessage behavior

The more recent EventEmitter-based proposal seems preferable to this one, as it is more explicit, simpler to call, and segregates the message name from the data passed in the message.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Add self.emit and self.on in content scripts, and worker.emit.

Myk: As you already approved this API change, I asked for review to Irakli.
If that's wrong, do not hesitate to jump in the discussion!

I'll provide a documentation patch in a second time.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Refactor Annotator example according to this new API

Here is a patch to use EventEmitter instead of postMessage in annotator example.
(Only on page-mod uses)

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Update worker, page-worker and panel docs

And a first modification on documentation. A broader change is still needed in dev-guides. From my perspective, I think we should try to hide postMessage in favor of EventEmitter API. First by providing examples only with emit/on and only mention postMessage in API methods lists for backward compatibility.
Your thoughts ?

Comment 14

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

https://bugzilla.mozilla.org/attachment.cgi?id=525669&action=diff#a/examples/annotator/data/selector.js_sec1

According to our coding style matcher.js#45 should look like:

self.emit('show', [
  document.location.toString(),
  $(ancestor).attr("id"),
  $(matchedElement).text()
]);

Comment 15

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

It was like this before but since you are at it can you please fix matcher.js#65-66 `false` should be on the same line.

  }, false);

Comment 16

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

Comment on attachment 525668 [details] [diff] [review]
Add self.emit and self.on in content scripts, and worker.emit.

Alex I have to r- this as it breaks context-menu. Please find that and all my other comments in pull request containing your patch: 
https://github.com/mozilla/addon-sdk/pull/146

Also I think there are two things that requires myk's feedback.

Comment 17

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

One more suggestion would be to print deprecation warning in the console when postMessage / onMessage is used suggesting to use on / emit instead.

Comment 18

[Alexandre Poirot [:ochameau]]

(In reply to comment #16)
> Comment on attachment 525668 [details] [diff] [review]
> Add self.emit and self.on in content scripts, and worker.emit.
> 
> Alex I have to r- this as it breaks context-menu. Please find that and all my
> other comments in pull request containing your patch: 
> https://github.com/mozilla/addon-sdk/pull/146
> 
> Also I think there are two things that requires myk's feedback.

FYI: I filled a seperate bug 649629 about events mix. Issues I identified in this bug may not require to block this patch whereas context-menu require to do so!

Comment 19

[Will Bamberg [:wbamberg]]

Comment on attachment 525669 [details] [diff] [review]
Refactor Annotator example according to this new API

The changes to the annotator look good (and the code looks nicer, thanks!). 

However, all the quoted code in the annotator tutorial would have to be updated as well. This is quite tedious but not difficult. However, we also need to update the guide on working with content scripts: https://jetpack.mozillalabs.com/sdk/1.0b4/docs/dev-guide/addon-development/web-content.html, which is a bit more involved.

If you like, I can take on updating the docs for this (annotator+guide) in a separate bug and we can cancel this review. I'll use the patch here for the annotator updates.

I still think the API docs (the second review you asked for) should be handled in this bug though.

Comment 20

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

As noted in the pull request:

Let's update the context-menu API to use self.on. Because this is a breaking change, we have to announce it far and wide.

I'll address the issue about what to do when an addon author tries to emit a core event in bug 649629.

Comment 21

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

Note the API change in bug 649629, comment 2.

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Proposal 1: add self.on/self.emit, worker.emit and throw exception

Patch on top of attachment 525668 [details] [diff] [review]/pull request:
I've addressed all review comments. 
This first proposal throw exception when we try to use internal event type.
We can easily modify it to only log errors.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Proposal 2: add self.{on,emit} and worker.port.{on,emit}

Again, this patch is an incremental one on top of attachment 525668 [details] [diff] [review]/pull request.
This time no exception is throwed, instead, bug 649629, comment 2, we add a new attribute `port` on Worker objects that implement the EventEmitter interface.

Note for the review: I had to rename existing _port/port to addonWorker/contentWorker to avoid confusing between this new port attribute and these one. And as Worker and WorkerGlobalScope are not more symetric, it's harder to keep this port concept. (WorkerGlobalScope receive all event, but Worker receive internal events and Worker.port receive developers events)

I'm not convinced by this attribute name `port`, even less by `sink`. It doesn't suggest me a clear meaning. What about: pipe, events, messages, ... ?

Finally I didn't add warning messages in postMessage, as it will breaks all tests and this is something we can do in a seperate bug.

Comment 24

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

Comment on attachment 527233 [details] [diff] [review]
Proposal 2: add self.{on,emit} and worker.port.{on,emit}

Just one more think.

Comment 25

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

(In reply to comment #24)
> Comment on attachment 527233 [details] [diff] [review]
> Proposal 2: add self.{on,emit} and worker.port.{on,emit}
> 
> Just one more think.

thing

Comment 26

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

Also after looking at examples of usage I'm really not unconvinced that sub-emitter is a good approach. More details here: https://bugzilla.mozilla.org/show_bug.cgi?id=649629#c3

Comment 27

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

Comment on attachment 527231 [details] [diff] [review]
Proposal 1: add self.on/self.emit, worker.emit and throw exception

Cancelling this one as Myk proposed to go with an alternative API.

Comment 28

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

(In reply to comment #23)
> I'm not convinced by this attribute name `port`, even less by `sink`. It
> doesn't suggest me a clear meaning. What about: pipe, events, messages, ... ?

`sink` is an abbreviation of `eventSink`, which is suitable because the communications channel we are proposing is quite like the concept of an event sink <http://en.wikipedia.org/wiki/Sink_%28computing%29>.

Normally I would bias towards the longer, unabbreviated name, but in this case the interface will be used frequently enough by addon developers to warrant its abbreviation.

`port` is also suitable, but it is used by Google Chrome Extensions to refer to a postMessage-based communications channel <http://code.google.com/chrome/extensions/extension.html#type-Port>, which could confuse developers who use both platforms.

`pipe` is suitable, although `sink` is more descriptive.

`events` and `messages` could confuse developers who also use Worker's own events and postMessage-based communications channel.

Comment 29

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

Alex can you please also add support for passing listeners through constructor as described here: https://bugzilla.mozilla.org/show_bug.cgi?id=649629#c4

In terms of name my preference would be: `port`, `pipe`, `sink`. I think similarity with chrome extensions is a + even though APIs are not exactly the same.

Also I do like `sink` as it reminds me of `sink` in Java which from my experience is a writable channel of the pipe: http://download.oracle.com/javase/1.5.0/docs/api/java/nio/channels/Pipe.html Which is different from our case as it's bidirectional pipe. 

Donno how is it in C++ though.

Comment 30

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

Also I don't mind having support for listeners via constructor as separate bug / patch on top of this one.

Comment 31

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

Just making myself clear options are listed from left to right in order of my preference: `port`, `pipe`, `sink`.

Comment 32

[Alexandre Poirot [:ochameau]]

Attachment: New pull request

First, here is a new pull request, in order for me to be able to commit in the
pull request!
I've merged with the current master, to avoid late merge conflict during land.
And I've addressed your comment on Traits hacks.

Then, I've post a new topic on the group and I hope to retrieve some feedback
about the attribute name choice:
http://groups.google.com/group/mozilla-labs-jetpack/t/d684b0f6ac396abd

Comment 33

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

Comment on attachment 527753 [details] [diff] [review]
New pull request

Please make sure:

1. That change does not includes timer related changes (see comment in pull req). 
2. That highlighted indentation error is fixed.
3. That Myk is happy with chosen naming.

Also no review is required for 1 and 2.

Thanks good work!

Comment 34

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

Comment on attachment 527753 [details] [diff] [review]
New pull request

Setting aside the naming issue for a moment, this interface is not quite right, because it adds an EventEmitter property on one end but not on the other.  Both ends, including the content script end, should have dedicated EventEmitter properties.

Comment 35

[Alexandre Poirot [:ochameau]]

(In reply to comment #34)
> Comment on attachment 527753 [details] [diff] [review]
> New pull request
> 
> Setting aside the naming issue for a moment, this interface is not quite right,
> because it adds an EventEmitter property on one end but not on the other.  Both
> ends, including the content script end, should have dedicated EventEmitter
> properties.

I've :
- reverted the timer commit that had nothing to do there!
- allowed to pass n-arguments to emit/on. (Use case in context menu module)
- added a port attribute in worker in order to match worker.emit:

So here is the revised API:
self.on("message", function (data) {}); // self.on is only for message
self.postMessage(data);
self.port.on("my-event-name", function (data) {});
self.port.emit("my-event-name", data);

worker.postMessage(data);
worker.on("message", function (data) {}); // only for message, error and internal events
worker.port.on("my-event-name", function (data) {});
worker.port.emit("my-event-name", data);

About indentation I don't see exactly what's wrong? Did I added too much indentation? Do I need to remove one level and shift code one step on the left?

Now we need to take decision on naming. Unfortunately, no feedback was provided by the Group. I'm letting port, but Myk, do not hesitate to tell me to change to something else. 

I really want to land such change in 1.0b5, so my plan is to provide all necessary modifications needed as soon as both of you respond me, and land this before the tomorrow's freeze.
Then I thought that we may still land documentation and may be examples modifications after the freeze ?

Comment 36

[Alexandre Poirot [:ochameau]]

Attachment: Need a review about self.port addition

I add some new code to implement self.port that need to be reviewed.

Comment 37

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

(In reply to comment #35)
> Now we need to take decision on naming. Unfortunately, no feedback was provided
> by the Group. I'm letting port, but Myk, do not hesitate to tell me to change
> to something else.

I thought about it some more and agree with using "port".  It's quite descriptive, doesn't conflict too badly with the usage of the term in Chrome Extensions, and isn't event-specific, which allows us to expand the interface in the future to encompass additional methods of communication.

Comment 38

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

Comment on attachment 528333 [details]
Need a review about self.port addition

I made few comments in the pull request.

1. Please fix comment before push.
2. Optionally use `port` getter instead.

Non of this needs additional review as long as test pass.

Comment 39

[Alexandre Poirot [:ochameau]]

Landed! Thanks for all reviews.

https://github.com/mozilla/addon-sdk/commit/1727bc5eed9b2acf29d1ce169835f7da387dba37

Comment 40

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

Resolving bug fixed, as this has landed.

Also, note commit https://github.com/mozilla/addon-sdk/commit/6e2dc3f46ddfadfd52a785bb5c62995039f47dc8, which fixes test bustage caused by the fix for this bug in conjunction with the fix for bug 652978.

Comment 41

[Alexandre Poirot [:ochameau]]

(In reply to comment #19)
> Comment on attachment 525669 [details] [diff] [review]
> Refactor Annotator example according to this new API
> 
Just landed annotator refactor:
https://github.com/mozilla/addon-sdk/commit/59f2ba601a79bf1262d61177d90c022f6210c604

Comment 42

[Alexandre Poirot [:ochameau]]

I've submitted a new bug 653109 dedicated on documentation.

Comment 43

[Will Bamberg [:wbamberg]]

Comment on attachment 525671 [details] [diff] [review]
Update worker, page-worker and panel docs

...I think this ended up being handled in a different bug...