Instead of Addon SDK event emitters.

Remove the reference to: sdk/event/core sdk/event/target Used in: devtools/shared/protocol.js devtools/docs/backend/protocol.js.md

Fair warning, I think most of the remaining bugs for the no-sdk project are severely depending on each other. Let's say we have 3 classes/objects/files using events: - class A can emit the event "my-event" (ie. it's an event target in the SDK vocabulary) - class B listens to "my-event" on an instance of A (ie. uses on, once, off) - class C triggers the event on instances of A (ie. uses emit) All 3 actually need to use the same event util in order for this to work. If class A relies on the new devtools event helper then class B and C need to also use the devtools event helper. What this means is that we can't really migrate file by file. We could migrate "event by event", but I feel like this will be unnecessarily complicated. I'm hoping a mass migration can be easy enough here. Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb851f4b5c91b94a2bd449949405d845f913d5c4 If this is green then all the "Stop using sdk/event/core ..." bugs will have to be closed as duplicates of this one. I will try to block them all on this bug.

After a few additional fixes, try seems green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d40330191c9a0ded07aed2ffcec9c25e6cae206f

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa84e9a9481c0fdfd74e851e8ddc8a72a4ee19e0

Comment on attachment 8897906 [details] Bug 1137935 - add support for wildcard event type in devtools event emitter; https://reviewboard.mozilla.org/r/169186/#review174822 I'm concerned about the performance impact of such feature. 'emit' and 'on' are hot codepath, used in many places. We should avoid slowing it down for special/rarely-used features. Is it only about these couple of usages? http://searchfox.org/mozilla-central/search?q=on(%22*%22&case=false&regexp=false&path=devtools May be we could make it so that it uses explicit listener names... http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#2685-2698 http://searchfox.org/mozilla-central/source/devtools/client/performance/performance-controller.js#413-419 Do not hesitate to keep these special callsites depending on wildard for a followup and proceed with most of you current refactoring! Otherwise you should also introduce tests for such new feature in: http://searchfox.org/mozilla-central/source/devtools/shared/tests/unit/test_eventemitter_basic.js

Comment on attachment 8897907 [details] Bug 1137935 - remove all usage of sdk/event/core/ in devtools; https://reviewboard.mozilla.org/r/169188/#review174828 Looks good.

Comment on attachment 8897908 [details] Bug 1137935 - remove usage of sdk event-target in devtools; https://reviewboard.mozilla.org/r/169190/#review174834 ::: devtools/client/webaudioeditor/models.js:22 (Diff revision 1) > // Will be added via AudioNodes `add` > collection: null, > > initialize: function (actor) { > + events.decorate(this); > + Can't we keep using `extends`? ``` const EventEmitter = require("devtools/shared/event-emitter"); class AudioNodeModel = Class({ extends: EventEmitter, ... ``` According to: https://github.com/devtools-html/snippets-for-removing-the-sdk#replacing-sdkeventtarget that's what we should be aiming for (in addition to try to switch to ES6 modules). Is there an issue in using heritage + extends + EventEmitter?

(In reply to Alexandre Poirot [:ochameau] from comment #9) > Comment on attachment 8897906 [details] > Bug 1137935 - add support for wildcard event type in devtools event emitter; > > https://reviewboard.mozilla.org/r/169186/#review174822 > > I'm concerned about the performance impact of such feature. > 'emit' and 'on' are hot codepath, used in many places. We should avoid > slowing it down for special/rarely-used features. > > Is it only about these couple of usages? > http://searchfox.org/mozilla-central/ > search?q=on(%22*%22&case=false&regexp=false&path=devtools There are also: http://searchfox.org/mozilla-central/search?q=on%5C(%5B%5E%2C%5D%2B%2C%5Cs*%22%5C*%22&case=false&regexp=true&path=devtools (call sites using the alternate syntax EventEmitter.on(target, eventType, listener)) > May be we could make it so that it uses explicit listener names... > > http://searchfox.org/mozilla-central/source/devtools/client/framework/ > toolbox.js#2685-2698 > > http://searchfox.org/mozilla-central/source/devtools/client/performance/ > performance-controller.js#413-419 Some call sites are only targeting an explicit list of events. Others are just piping events: - devtools/server/actors/performance.js pipes events from devtools/server/performance/recorder (although using a list of allowed events) - devtools/server/actors/profiler.js pipes events from devtools/server/performance/profiler - devtools/server/actors/timeline.js pipes events from devtools/server/performance/timeline - devtools/shared/fronts/profiler.js pipes its own events and translates them (though I doubt it is still used) I'll see if I can implement piping in a different way. > > Do not hesitate to keep these special callsites depending on wildard for a > followup and proceed with most of you current refactoring! If you mean keep using the sdk event emitter for the wildcard, that's a bit problematic because of comment 3. > > Otherwise you should also introduce tests for such new feature in: > http://searchfox.org/mozilla-central/source/devtools/shared/tests/unit/ > test_eventemitter_basic.js

(In reply to Alexandre Poirot [:ochameau] from comment #11) > Comment on attachment 8897908 [details] > Bug 1137935 - remove usage of sdk event-target in devtools; > > https://reviewboard.mozilla.org/r/169190/#review174834 > > ::: devtools/client/webaudioeditor/models.js:22 > (Diff revision 1) > > // Will be added via AudioNodes `add` > > collection: null, > > > > initialize: function (actor) { > > + events.decorate(this); > > + > > Can't we keep using `extends`? > ``` > const EventEmitter = require("devtools/shared/event-emitter"); > class AudioNodeModel = Class({ > extends: EventEmitter, > ... > ``` > According to: > https://github.com/devtools-html/snippets-for-removing-the-sdk#replacing- > sdkeventtarget > that's what we should be aiming for (in addition to try to switch to ES6 > modules). > Is there an issue in using heritage + extends + EventEmitter? No issue, I assumed extends had to be used with heritage-based classes only!

Comment on attachment 8897906 [details] Bug 1137935 - add support for wildcard event type in devtools event emitter; I just added a test in the meantime. I'll try to get rid of the wildcard call sites.

(In reply to Julian Descottes [:jdescottes] from comment #18) > Comment on attachment 8897906 [details] > Bug 1137935 - add support for wildcard event type in devtools event emitter; > > I just added a test in the meantime. I'll try to get rid of the wildcard > call sites. As discussed on IRC, I'd like to support the wildcard API for now. I logged Bug 1391261 as a follow-up to remove it after we are done with the refactor and I commented the code accordingly.

Comment on attachment 8897906 [details] Bug 1137935 - add support for wildcard event type in devtools event emitter; https://reviewboard.mozilla.org/r/169186/#review174962 Thanks for the test and the followup already opened!

Comment on attachment 8897908 [details] Bug 1137935 - remove usage of sdk event-target in devtools; https://reviewboard.mozilla.org/r/169190/#review174964 Thanks, it looks reasonable as a first iteration to switch into the new event-emitter codepath. But I see many followups which may already be filled? ::: devtools/client/webaudioeditor/models.js:189 (Diff revision 4) > node.collection = this; > > this.models.add(node); > > node.on("*", this._onModelEvent); > - coreEmit(this, "add", node); > + EventEmitter.emit(this, "add", node); I'm wondering if we could switch to this thanks to the new implementation: this.emit("add", node); This comment applies here and in other places where we use `emit(this` or `on(this`. The important point is when we use "this" as first argument. But feel free to cover that in a dedicated bug. ::: devtools/server/performance/timeline.js:57 (Diff revision 4) > this._framerate = null; > > // Make sure to get markers from new windows as they become available > this._onWindowReady = this._onWindowReady.bind(this); > this._onGarbageCollection = this._onGarbageCollection.bind(this); > - events.on(this.tabActor, "window-ready", this._onWindowReady); > + EventEmitter.on(this.tabActor, "window-ready", this._onWindowReady); Here we aren't using `this` as first argument, but I imagine we would want to do: this.tabActor.on("window-ready", this._onWindowReady); in near future? Do we? Is there a bug for that? ::: devtools/shared/protocol.js:14 (Diff revision 4) > -var {EventTarget} = require("sdk/event/target"); > +var EventEmitter = require("devtools/shared/event-emitter"); > -var events = require("devtools/shared/event-emitter"); > var {getStack, callFunctionWithAsyncStack} = require("devtools/shared/platform/stack"); > var {settleAll} = require("devtools/shared/DevToolsUtils"); > > -exports.emit = events.emit; > +exports.emit = EventEmitter.emit; Hum. Is this used? Should we track that and eventually get rid of this? (may be it is already tracked by some bug?) It looks like an hack necessary because of SDK events API. ::: devtools/shared/protocol.js:1296 (Diff revision 4) > > // Check to see if any of the preEvents returned a promise -- if so, > // wait for their resolution before emitting. Otherwise, emit synchronously. > if (results.some(result => result && typeof result.then === "function")) { > promise.all(results).then(() => { > - return events.emit.apply(null, [this, event.name].concat(args)); > + return EventEmitter.emit.apply(null, [this, event.name].concat(args)); Could this become? this.emit(event.name, ...args); Is this really important to nullify the `this` value?? I think we used to pass null before as it wasn't used, not really because we wanted it to be null. (same comment applies to the same code a few lines after)

Comment on attachment 8897908 [details] Bug 1137935 - remove usage of sdk event-target in devtools; https://reviewboard.mozilla.org/r/169190/#review174964 Thanks for the review! Logged to follow up bugs to address the issues raised here. > I'm wondering if we could switch to this thanks to the new implementation: > this.emit("add", node); > > This comment applies here and in other places where we use `emit(this` or `on(this`. > The important point is when we use "this" as first argument. > But feel free to cover that in a dedicated bug. Good point, let's track this in Bug 1391562 > Here we aren't using `this` as first argument, > but I imagine we would want to do: > this.tabActor.on("window-ready", this._onWindowReady); > in near future? Do we? Is there a bug for that? Sure, I think we can also handle this in Bug 1391562 > Hum. > Is this used? > Should we track that and eventually get rid of this? > (may be it is already tracked by some bug?) > It looks like an hack necessary because of SDK events API. Logged Bug 1391563 about this. > Could this become? > this.emit(event.name, ...args); > Is this really important to nullify the `this` value?? I think we used to pass null before as it wasn't used, not really because we wanted it to be null. > > (same comment applies to the same code a few lines after) Let's also take care of this in Bug 1391562

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/878db7bb8045 add support for wildcard event type in devtools event emitter;r=ochameau https://hg.mozilla.org/integration/autoland/rev/d9be0f1c5dcd remove all usage of sdk/event/core/ in devtools;r=ochameau https://hg.mozilla.org/integration/autoland/rev/006b6882688f remove usage of sdk event-target in devtools;r=ochameau

https://hg.mozilla.org/mozilla-central/rev/878db7bb8045 https://hg.mozilla.org/mozilla-central/rev/d9be0f1c5dcd https://hg.mozilla.org/mozilla-central/rev/006b6882688f