Closed Bug 1356231 Opened 2 years ago Closed 2 years ago

Move event-emitter to /toolkit/

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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
Duplicate of this bug: 1156987
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: 2 years ago
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.