976679 - Move event-emitter to toolkit

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I've wanted to use event-emitter from toolkit code several times now, and I then become sad when I discover it's in browser instead.

Let's move it to toolkit.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Move event-emitter to toolkit

* Moved event-emitter and its test to toolkit
* Converted test to chrome from browser chrome

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

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I also added a general "devtools/toolkit" path to the loader as part of this.  Assuming that seems reasonable, I may cleanup all the other crazy toolkit paths we have in there to follow that format in another bug.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to J. Ryan Stinnett [:jryans] from comment #2)
> I also added a general "devtools/toolkit" path to the loader as part of
> this.  Assuming that seems reasonable, I may cleanup all the other crazy
> toolkit paths we have in there to follow that format in another bug.

Keep in mind that the patch in bug 859372 also does this, so you may have to rebase before landing.

Have you considered using the jetpack event-emitter API instead? AFAIK we want to eventually remove our custom libraries in favor of the SDK ones:

https://developer.mozilla.org/Add-ons/SDK/Low-Level_APIs/event_core

Just a thought.

Comment 4

[Paul Rouget [:paul]]

Comment on attachment 8381898 [details] [diff] [review]
Move event-emitter to toolkit

r+, what do you think of comment 3?

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Panos Astithas [:past] from comment #3)
> (In reply to J. Ryan Stinnett [:jryans] from comment #2)
> > I also added a general "devtools/toolkit" path to the loader as part of
> > this.  Assuming that seems reasonable, I may cleanup all the other crazy
> > toolkit paths we have in there to follow that format in another bug.
> 
> Keep in mind that the patch in bug 859372 also does this, so you may have to
> rebase before landing.

Good to know, maybe I'll be able to sneak in before that one. :)

> Have you considered using the jetpack event-emitter API instead? AFAIK we
> want to eventually remove our custom libraries in favor of the SDK ones:
> 
> https://developer.mozilla.org/Add-ons/SDK/Low-Level_APIs/event_core
> 
> Just a thought.

I do agree generally that we should switch down to one event system, and probably it should be the SDK one.

I filed bug 952653 a while ago, as it does bother me that we have (at least) 3 event systems in play at the moment.

For now, event-emitter has several more features (or differences at least...) compared to SDK event/core:

* e-e listeners get the event name, which can be used to have one "master" listener for many events (we use this in a few places)
* e-e's off() is able to remove one-time listeners
* e-e.decorate(), which is quite elegant, has no direct equivalent
* Bug 974056 will add debug logging to e-e

Anyway, I'm sure all these things could be addressed in SDK events, but for the moment, I just was hoping to use event-emitter. :) I'll copy this list into bug 952653.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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