Closed Bug 1356231 Opened 8 years ago Closed 8 years ago

Move event-emitter to /toolkit/

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 1356231 - Use toolkit EventEmitter.jsm instead of devtools module.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1356231 - Import DevTools event-emitter module to toolkit as a JSM.
59 bytes, text/x-review-board-request
mossop
: review+
Details
http://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.js This module is used by many modules from /browser/ and /toolkit. It isn't much specific to devtools and would be better hosted in /toolkit/. We are going to soon ship DevTools as an add-on, so that this module won't necessary exists and Firefox would break if we keep it as-is. Note that some add-ons are using this module, via different URLs as we already moved things around while extracting devtools out of /browser/: https://dxr.mozilla.org/addons/search?q=event-emitter&redirect=true So we may want to expose a shortcut URL if we land before m-c is FF57.
See Also: → 1156987, 1259522, 952653
Comment on attachment 8861057 [details] Bug 1356231 - Use toolkit EventEmitter.jsm instead of devtools module. Mossop, It looks like you more or less requested a patch like this one in bug 1156987. Are you still ok seeing event-emitter go to toolkit? Are you a good reviewer for this patch? Who should join the review for such patch? I kept event-emitter implementation as-is, I just fixed eslint issues. It would be better to keep implementation changes for bug 952653.
Attachment #8861057 - Flags: feedback?(dtownsend)
Comment on attachment 8861057 [details] Bug 1356231 - Use toolkit EventEmitter.jsm instead of devtools module. Yep I'm still happy for this to go to toolkit and I'm happy to do the review too! Main questions: Do we still need all the factory goop at the top or can this be a pure JSM? What do you think about naming the file EventEmitter.jsm to be consistent with other JSM naming?
Attachment #8861057 - Flags: feedback?(dtownsend) → feedback+
Comment on attachment 8861057 [details] Bug 1356231 - Use toolkit EventEmitter.jsm instead of devtools module. https://reviewboard.mozilla.org/r/133046/#review135806 ::: toolkit/modules/event-emitter.js:68 (Diff revision 1) > + let EventEmitter = this.EventEmitter = function () {}; > + module.exports = EventEmitter; > + > + // See comment in JSM module boilerplate when adding a new dependency. > + const Services = require("Services"); > + const defer = require("devtools/shared/defer"); Potentially a good time to switch to native Promises here? ::: toolkit/modules/event-emitter.js:69 (Diff revision 1) > + module.exports = EventEmitter; > + > + // See comment in JSM module boilerplate when adding a new dependency. > + const Services = require("Services"); > + const defer = require("devtools/shared/defer"); > +// const { describeNthCaller } = require("devtools/shared/platform/stack"); I assume a toolkit copy would be chrome only...? So, you should be able to copy the functionality of `resource://devtools/shared/platform/chrome/stack.js`.
Comment on attachment 8861057 [details] Bug 1356231 - Use toolkit EventEmitter.jsm instead of devtools module. https://reviewboard.mozilla.org/r/133046/#review135812 ::: toolkit/modules/moz.build:196 (Diff revision 1) > 'Console.jsm', > 'DateTimePickerHelper.jsm', > 'debug.js', > 'DeferredTask.jsm', > 'Deprecated.jsm', > + 'event-emitter.js', There is an event-emitter test in `devtools/shared/tests/mochitest/test_eventemitter_basic.html`, probably a good idea to copy this to toolkit as well.
(In reply to Dave Townsend [:mossop] from comment #5) > Comment on attachment 8861057 [details] > Bug 1356231 - Move event-emitter module to toolkit. ? > > Yep I'm still happy for this to go to toolkit and I'm happy to do the review > too! > > Main questions: > > Do we still need all the factory goop at the top or can this be a pure JSM? Oh yes, I didn't realized every single usage in m-c was via Cu.import. Yes, sure we can do that. I had in mind to reuse this module from devtools in a followup, but may be that's better to fork in order to do that... > What do you think about naming the file EventEmitter.jsm to be consistent > with other JSM naming? Sure, if we move to a jsm. (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > Potentially a good time to switch to native Promises here? Yes, should be easy enough to be inlined in this bug. > ::: toolkit/modules/event-emitter.js:69 > (Diff revision 1) > > + module.exports = EventEmitter; > > + > > + // See comment in JSM module boilerplate when adding a new dependency. > > + const Services = require("Services"); > > + const defer = require("devtools/shared/defer"); > > +// const { describeNthCaller } = require("devtools/shared/platform/stack"); > > I assume a toolkit copy would be chrome only...? So, you should be able to > copy the functionality of > `resource://devtools/shared/platform/chrome/stack.js`. As I just wrote to Mossop, I was thinking about a file move rather than a copy, but that may be another reason to make a copy.
Assignee: nobody → poirot.alex
Please consider reviewing all these changesets. I kept intermediate revision to help reviewing this, but I'm considering merging them all before landing. Or may be just keep "Use toolkit EventEmitter.jsm instead of devtools module." distinct. So I ended up doing the suggested cleanup. I introduced a new pref specific to this module. I also ported the test from devtools and converting it to xpcshell (it was a mochitest for no special reason).
Did you mean to flag me to review all of these? I'm fine to, just wanted to make sure they were ready to review before I did.
Flags: needinfo?(poirot.alex)
(In reply to Dave Townsend [:mossop] from comment #18) > Did you mean to flag me to review all of these? Yes.
Flags: needinfo?(poirot.alex)
Triaging dt-addon blockers as P3/enhancement since we're not using priorities for this project (filter on CLIMBING SHOES).
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8861058 [details] Bug 1356231 - Fix eslint https://reviewboard.mozilla.org/r/133048/#review136964
Attachment #8861058 - Flags: review?(dtownsend) → review+
Comment on attachment 8861584 [details] Bug 1356231 - Use native promises instead of devtools promises. https://reviewboard.mozilla.org/r/133552/#review136966
Attachment #8861584 - Flags: review?(dtownsend) → review+
Comment on attachment 8861586 [details] Bug 1356231 - Remove boilerplate https://reviewboard.mozilla.org/r/133556/#review136968
Attachment #8861586 - Flags: review?(dtownsend) → review+
Comment on attachment 8861057 [details] Bug 1356231 - Use toolkit EventEmitter.jsm instead of devtools module. https://reviewboard.mozilla.org/r/133046/#review136970
Attachment #8861057 - Flags: review?(dtownsend) → review+
Comment on attachment 8861583 [details] Bug 1356231 - Import DevTools event-emitter module to toolkit as a JSM. https://reviewboard.mozilla.org/r/133550/#review136494 ::: toolkit/modules/EventEmitter.jsm:19 (Diff revision 1) > + // on the main thread not use any chrome privileged APIs. Instead, > + // the body of the main function can only require() (not Cu.import) > + // modules that are available in the devtools content mode. This, > + // plus the lack of |console| in workers, results in some gyrations > + // in the definition of |console|. > + if (this.module && module.id.indexOf("event-emitter") >= 0) { Presumably this should now be s/event-emitter/EventEmitter/. I guess it doesn't matter a lot since this goes away in a following patch. ::: toolkit/modules/EventEmitter.jsm:213 (Diff revision 1) > + listener.apply(null, arguments); > + } catch (ex) { > + // Prevent a bad listener from interfering with the others. > + let msg = ex + ": " + ex.stack; > + console.error(msg); > + dump(msg + "\n"); This should be protected by loggingEnabled
Attachment #8861583 - Flags: review?(dtownsend) → review+
Comment on attachment 8861585 [details] Bug 1356231 - Import devtools test as xpcshell https://reviewboard.mozilla.org/r/133554/#review136986
Attachment #8861585 - Flags: review?(dtownsend) → review+
Comment on attachment 8861583 [details] Bug 1356231 - Import DevTools event-emitter module to toolkit as a JSM. https://reviewboard.mozilla.org/r/133550/#review136494 > This should be protected by loggingEnabled If we do that, we would silence any exception happening within callback passed to EventEmitter.on when the pref is off: obj.on("foo", function () { throw new Error("bar"); }); It would make sense to silence them if enabling the logging pref was a really common pref for development. But it is not, and for a good reason. It tends to be very verbose to print every emit call! Or did you meant to guard only the dump() call? Otherwise, do you think I should r? some other folks for these patches?
(In reply to Alexandre Poirot [:ochameau] from comment #32) > Comment on attachment 8861583 [details] > Bug 1356231 - Import event-emitter module to toolkit as a JSM. > > https://reviewboard.mozilla.org/r/133550/#review136494 > > > This should be protected by loggingEnabled > > If we do that, we would silence any exception happening within callback > passed to EventEmitter.on when the pref is off: > obj.on("foo", function () { > throw new Error("bar"); > }); > It would make sense to silence them if enabling the logging pref was a > really common pref for development. But it is not, and for a good reason. It > tends to be very verbose to print every emit call! > > Or did you meant to guard only the dump() call? I meant just the dump call, seems like the console logging should be enough for debugging purposes. > Otherwise, do you think I should r? some other folks for these patches? I think my review is enough here.
Attachment #8861058 - Attachment is obsolete: true
Attachment #8861584 - Attachment is obsolete: true
Attachment #8861585 - Attachment is obsolete: true
Attachment #8861586 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/479eb09af84e Import DevTools event-emitter module to toolkit as a JSM. r=mossop https://hg.mozilla.org/integration/autoland/rev/e919535c7872 Use toolkit EventEmitter.jsm instead of devtools module. r=mossop
https://hg.mozilla.org/mozilla-central/rev/479eb09af84e https://hg.mozilla.org/mozilla-central/rev/e919535c7872
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: