Closed Bug 976679 Opened 6 years ago Closed 6 years ago

Move event-emitter to toolkit

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
* Moved event-emitter and its test to toolkit
* Converted test to chrome from browser chrome

Try: https://tbpl.mozilla.org/?tree=Try&rev=e11c7f1b490e
Attachment #8381898 - Flags: review?(paul)
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.
(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 on attachment 8381898 [details] [diff] [review]
Move event-emitter to toolkit

r+, what do you think of comment 3?
Attachment #8381898 - Flags: review?(paul) → review+
(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.
Keywords: checkin-needed
Blocks: 925692
https://hg.mozilla.org/integration/fx-team/rev/b2baefa192ff
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b2baefa192ff
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Depends on: 982707
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.