1137935 - Remove usage of sdk APIs in devtools/shared/protocol.js

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Instead of Addon SDK event emitters.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Remove the reference to:

  sdk/event/core
  sdk/event/target

Used in:

  devtools/shared/protocol.js
  devtools/docs/backend/protocol.js.md

Comment 3

[Julian Descottes [:jdescottes]]

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.

Comment 4

[Julian Descottes [:jdescottes]]

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

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1137935 - add support for wildcard event type in devtools event emitter;

Review commit: https://reviewboard.mozilla.org/r/169186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169186/

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1137935 - remove all usage of sdk/event/core/ in devtools;

Review commit: https://reviewboard.mozilla.org/r/169188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169188/

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1137935 - remove usage of sdk event-target in devtools;

Review commit: https://reviewboard.mozilla.org/r/169190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169190/

Comment 8

[Julian Descottes [:jdescottes]]

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

Comment 9

[Alexandre Poirot [:ochameau]]

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 10

[Alexandre Poirot [:ochameau]]

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 11

[Alexandre Poirot [:ochameau]]

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?

Comment 12

[Julian Descottes [:jdescottes]]

(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

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8897908 [details]
Bug 1137935 - remove usage of sdk event-target in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169190/diff/1-2/

Comment 14

[Julian Descottes [:jdescottes]]

(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 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8897906 [details]
Bug 1137935 - add support for wildcard event type in devtools event emitter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169186/diff/1-2/

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8897907 [details]
Bug 1137935 - remove all usage of sdk/event/core/ in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169188/diff/1-2/

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8897908 [details]
Bug 1137935 - remove usage of sdk event-target in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169190/diff/2-3/

Comment 18

[Julian Descottes [:jdescottes]]

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.

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8897906 [details]
Bug 1137935 - add support for wildcard event type in devtools event emitter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169186/diff/2-3/

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8897907 [details]
Bug 1137935 - remove all usage of sdk/event/core/ in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169188/diff/2-3/

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8897908 [details]
Bug 1137935 - remove usage of sdk event-target in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169190/diff/3-4/

Comment 22

[Julian Descottes [:jdescottes]]

(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 23

[Alexandre Poirot [:ochameau]]

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 24

[Alexandre Poirot [:ochameau]]

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 25

[Julian Descottes [:jdescottes]]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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