Closed
Bug 976679
Opened 11 years ago
Closed 11 years ago
Move event-emitter to toolkit
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: jryans, Assigned: jryans)
References
Details
Attachments
(1 file)
(deleted),
patch
|
paul
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
* 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)
Assignee | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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+
Assignee | ||
Comment 5•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Updated•11 years ago
|
QA Whiteboard: [qa-]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•