723904 - Implement a EventEmitter mechanism that can be used by any tool

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

We use DOM Events, callbacks or the ObserverService in our tools. These mechanisms are not always the most efficient ways to address our problems.

A simple EventEmiter mechanism would be very helpful.

The highlighter already implement something similar:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/highlighter.jsm#636

Comment 1

[Paul Rouget [:paul]]

Maybe we can implement that in toolkit/. No need to keep that only for the devtools.

Comment 2

[Paul Rouget [:paul]]

Also present in the debugger code:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/file/517cbf7e7cd4/toolkit/devtools/debugger/dbg-client.jsm#l67

I also think we get an eventemiter mechanism in the Addons Manager code.

Comment 3

[Mihai Sucan [:msucan]]

It's also present in Orion and Source Editor (in the next update, see bug 723069) will also use the EventTarget mixin coming from Orion. At least the Source Editor itself won't reimplement it. The Source Editor textarea fallback also has one implementation for itself.

If I am not mistaken the Panorama code has one as well...

Comment 4

[Paul Rouget [:paul]]

Attachment: v1

Comment 5

[Paul Rouget [:paul]]

Attachment: v1.1

Comment 6

[Paul Rouget [:paul]]

Attachment: v1.2

Comment 7

[Joe Walker [:jwalker]]

Comment on attachment 651791 [details] [diff] [review]
v1.2

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

::: browser/devtools/shared/EventEmitter.jsm
@@ +11,5 @@
> +   *        The event name to which we're connecting.
> +   * @param function aListener
> +   *        Called when the event is fired.
> +   */
> +  on: function EventEmitter_on(aEvent, aListener) {

So I've seen 2 basic approaches, Browser Style and Event Function.

EventEmitter is basically the same Browser Style, with one basic difference. Event Function works like this:

    this.onFoo = createEvent();
    
    this.onFoo.add(function(ev) {
      console.log('foo was passed ' + ev.name);
    });
    
    this.onFoo({ name: 'bar' });

Event Function has a number of benefits:
- it is kind of type safe. If you typo the event name, you get an error not missed events, and it is easier to refactor for the same reason
- you don't need to have copies of the add/remove functions in the class owning the event, because the event comes with them.

But on the other hand it's not the Browser Way, so it requires more understanding.


I think that we're far enough into the browser that doing things the browser way probably makes sense, but I'd be happy either way.

If we do stick with the Browser Way, I wonder if we should be calling 'on' 'addListener' for 2 reasons:
- If we're doing things the browser way, then we should do that and not shortcut here
- It's not symmetrical with removeListener

Also, perhaps we should make a minor tweak to the parameter list:

    addListener(aEvent, aListener, aScope)

This would allow people to add a listener without binding, and worrying about where to store the bound version so they can remove it easily.

@@ +82,5 @@
> +   * Destroy the references to the listeners.
> +   */
> +  removeAllListeners: function EventEmitter_destroy() {
> +    delete this._eventEmitterListeners;
> +  }

I've got 2 thoughts on this. If you do:

    this.f = new EventListener();
    this.f.on('bar', thinger);
    delete this.f;

Then I would think that it's not possible for the event listener to hold onto a reference to thinger. So we don't need to call removeAllListeners first.

Also this violates the principle of 'clear up after yourself' somewhat.

So I wonder if we shouldn't just remove this function.

Comment 8

[Dave Camp (:dcamp)]

(In reply to Joe Walker [:jwalker] from comment #7)

> If we do stick with the Browser Way, I wonder if we should be calling 'on'
> 'addListener' for 2 reasons:
> - If we're doing things the browser way, then we should do that and not
> shortcut here
> - It's not symmetrical with removeListener

Some extra data here:

* This was modeled after jetpack's API, which used on() and removeListener(), but has since changed to on() and off().  Jetpack's API seems like a reasonable possibility to model ourselves, particularly because:
* This isn't quite a browser analogue - there's no bubbling, event objects, etc.  So a slightly distinct naming/api might be in order?

> Also, perhaps we should make a minor tweak to the parameter list:
> 
>     addListener(aEvent, aListener, aScope)
> 
> This would allow people to add a listener without binding, and worrying
> about where to store the bound version so they can remove it easily.

I like that.

Comment 9

[Joe Walker [:jwalker]]

(In reply to Dave Camp (:dcamp) from comment #8)
> (In reply to Joe Walker [:jwalker] from comment #7)
> 
> > If we do stick with the Browser Way, I wonder if we should be calling 'on'
> > 'addListener' for 2 reasons:
> > - If we're doing things the browser way, then we should do that and not
> > shortcut here
> > - It's not symmetrical with removeListener
> 
> Some extra data here:
> 
> * This was modeled after jetpack's API, which used on() and
> removeListener(), but has since changed to on() and off().  Jetpack's API
> seems like a reasonable possibility to model ourselves, particularly because:
> * This isn't quite a browser analogue - there's no bubbling, event objects,
> etc.  So a slightly distinct naming/api might be in order?

Ahh, that's it. I knew I'd seen it somewhere before.

So I vote we s/removeEventListener/off/g

> > Also, perhaps we should make a minor tweak to the parameter list:
> > 
> >     addListener(aEvent, aListener, aScope)
> > 
> > This would allow people to add a listener without binding, and worrying
> > about where to store the bound version so they can remove it easily.
> 
> I like that.

But the jetpack API doesn't do it. And we can add it much more easily that we can take it away, so I vote we leave it as it is.

Also I revote my suggestion to remove removeAllListeners, because the jetpack api doesn't have that.

In summary:
- s/removeEventListener/off/g
- remove removeAllListeners

And I think we nailed it.

Comment 10

[Paul Rouget [:paul]]

Attachment: v1.1

- s/removeEventListener/off/g
- remove removeAllListeners

Comment 11

[Dave Camp (:dcamp)]

Comment on attachment 652115 [details] [diff] [review]
v1.1

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

::: browser/devtools/highlighter/inspector.jsm
@@ +255,2 @@
>    {
> +    this._eventEmitter.emit(aEvent);

Won't this lose the extra arguments (that were previously accessed via the argument array)?

Is there some way we can avoid every event emitter having to have this boilerplate?  Maybe something like:

function emitter(obj)
{
  let e = new EventEmitter();
  obj.on = e.on.bind(e);
  obj.off = e.off.bind(e);
  ..
}

Comment 12

[Joe Walker [:jwalker]]

NewClass.prototype = Object.create(EventEmitter.prototype);

?

Comment 13

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #12)
> NewClass.prototype = Object.create(EventEmitter.prototype);

No, then events would be per class. Sorry.

Comment 14

[Paul Rouget [:paul]]

Should we call the callback asynchronously?

This current implementation is very much synchronous. We might want to get asynchronous events in the future.

Comment 15

[Dave Camp (:dcamp)]

(In reply to Paul Rouget [:paul] from comment #14)
> Should we call the callback asynchronously?

I don't think so... does jetpack's event emitter?

Comment 16

[Joe Walker [:jwalker]]

(In reply to Dave Camp (:dcamp) from comment #15)
> (In reply to Paul Rouget [:paul] from comment #14)
> > Should we call the callback asynchronously?
> 
> I don't think so... does jetpack's event emitter?

I think that doing it async is safer. You can make an async api act sync without breaking anything, but you can't do it the other way around.

Also the corners that you might cut as a result of having a sync API are probably corners that you shouldn't be cutting anyway (although I suspect that these 2 arguments are actually the same thing)

Comment 17

[Joe Walker [:jwalker]]

TL;DR: Ignore comment 16. Let's do it sync as it is now.


22:22 dcamp: async event emitting seems like a Really Bad Idea to me.
22:23 joe_walker: ok?
22:23 dcamp: will bring comments to the bug but
22:23 dcamp: in the event emitter case the "you can make async appear sync" is opposite, kinda?
22:24 dcamp: easy to wrap event emitting/handling in a 0-length timeout function
22:24 dcamp: hard to get notification that you can prove is timely
22:24 dcamp: with an async api
22:26 joe_walker: yes, i agree, but i'm wondering if that's a case where we shouldn't be using events
22:27 dcamp: I guess I've seen a dozen or so sync event apis
22:27 dcamp: and very few (none, maybe?) async ones
22:29 joe_walker: maybe sync is the default because it's be most obvious
22:29 dcamp: most obvious isn't bad ;)
22:29 joe_walker: do you mean sync because it needs to be sync
22:29 dcamp: I mean sync because it's easier to debug, less likely to get into weird states
22:30 dcamp: (I signal "I'm in state foo!" but by the time it's delivered I've moved on to state bar)
22:30 joe_walker: that i agree with
22:30 joe_walker: also we should see what jetpack is doing
22:30 joe_walker: if we're serious about having similar api
22:30 dcamp: And I don't really think it's Good Programming Discipline to be ready for an object in state 'bar' when you respond to a 'foo' event, it's just Masochistic :)
22:31 dcamp: agreed, though we could debate whether we're serious about that :)
22:31 dcamp: I'd say that in this case it's easier to wrap async around sync
22:32 dcamp: (just turn your emits into setTimeout(function emit() {}, 0) and your attaches to on("event", function() { setTimeout(realCallback, 0) })
22:32 dcamp: than vice-versa.  But I could be wrong.
22:35 dcamp: I'm sympathetic to the argument that syncronous emission requires you to defend against reentrancy in a way that most people don't do well.
22:36 joe_walker: think jetpack api is sync
22:38 dcamp: my Software Patterns foo is weak, but I think there's one that describes publish/subscribe notifications
22:39 dcamp: and one that describes uhm... like decoupled bulk method calls
22:39 dcamp: and I tend to think of what we need to build the debugger/inspector/etc as the second
22:39 joe_walker: the point i was making about wrapping is that if you have an event that was sync that for some reason you need to make async, then you've broken your users
22:39 joe_walker: the other way around isn't breakage
22:39 dcamp: ah.  Yeah, that's true.
22:39 joe_walker: but we probably don't need to debate this at length, i think you've convinced me
22:40 dcamp: heh, just as I unconvinced myself :P
22:40 joe_walker: actual cost now >> theoretical future cost

Comment 18

[Paul Rouget [:paul]]

Attachment: v1.2

Comment 19

[Paul Rouget [:paul]]

(In reply to Dave Camp (:dcamp) from comment #11)
> Is there some way we can avoid every event emitter having to have this
> boilerplate?  Maybe something like:
> 
> function emitter(obj)
> {
>   let e = new EventEmitter();
>   obj.on = e.on.bind(e);
>   obj.off = e.off.bind(e);
>   ..
> }

I didn't address that.
Maybe we could, in the constructor, extend the object:

 function EventEmitter(aObjToExtend) {
    // if on, off or emit are already in the obj, throw exception
    aObjToExtend.on  = this.on.bind(this);
    aObjToExtend.off = this.off.bind(this);
    aObjToExtend.emit = this.emit.bind(this);
 }

Comment 20

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=3c99d4fcedd2

Comment 21

[Paul Rouget [:paul]]

Comment on attachment 660031 [details] [diff] [review]
v1.2

Green. I want to land that.

(I only changed the emit code to address Dave's comment. Also, I think we can do comment 19 later)

Comment 22

[Paul Rouget [:paul]]

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

Comment 23

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/f1ef5bccac0d