Closed Bug 1137935 Opened 7 years ago Closed 4 years ago

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

Categories

(DevTools :: General, defect, P1)

x86
macOS
defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: fitzgen, Assigned: jdescottes)

References

Details

(Whiteboard: [nosdk])

Attachments

(3 files)

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
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [nosdk]
No longer blocks: 952653
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Duplicate of this bug: 1378862
Summary: Migrate protocol.js's event emitters to toolkit/devtools/event-emitter.js → Remove usage of sdk APIs in devtools/shared/protocol.js
Priority: P2 → P1
Target Milestone: --- → Firefox 57
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.
Blocks: 1378904
Blocks: 1378926
Blocks: 1378925
Blocks: 1378922
Blocks: 1378919
Blocks: 1378902
Blocks: 1378906
Blocks: 1378917
Blocks: 1378911
Blocks: 1378901
Blocks: 1378905
Blocks: 1378916
Blocks: 1378918
Blocks: 1378907
Blocks: 1378921
Blocks: 1378903
Blocks: 1378909
Blocks: 1378897
Blocks: 1378910
Blocks: 1378920
Blocks: 1378927
Blocks: 1378898
Blocks: 1378913
Blocks: 1378923
Blocks: 1378915
Blocks: 1378914
Blocks: 1378900
Blocks: 1378908
Blocks: 1378924
Blocks: 1378894
Blocks: 1378893
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
Attachment #8897906 - Flags: review?(poirot.alex)
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.
Attachment #8897907 - Flags: review?(poirot.alex) → review+
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.
Attachment #8897906 - Flags: review?(poirot.alex)
See Also: → 1391261
(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.
Blocks: 1378849
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!
Attachment #8897906 - Flags: review?(poirot.alex) → review+
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)
Attachment #8897908 - Flags: review?(poirot.alex) → review+
Depends on: 1391562
Depends on: 1391563
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
Duplicate of this bug: 1378910
Duplicate of this bug: 1378927
Duplicate of this bug: 1378911
Duplicate of this bug: 1378894
Duplicate of this bug: 1378916
Duplicate of this bug: 1378897
Duplicate of this bug: 1378917
Duplicate of this bug: 1378898
Duplicate of this bug: 1378918
Duplicate of this bug: 1378900
Duplicate of this bug: 1378919
Duplicate of this bug: 1378901
Duplicate of this bug: 1378920
Duplicate of this bug: 1378902
Duplicate of this bug: 1378921
Duplicate of this bug: 1378922
Duplicate of this bug: 1378923
Duplicate of this bug: 1378924
Duplicate of this bug: 1378925
Duplicate of this bug: 1378926
Duplicate of this bug: 1378893
Duplicate of this bug: 1378903
Duplicate of this bug: 1378904
Duplicate of this bug: 1378905
Duplicate of this bug: 1378906
Duplicate of this bug: 1378907
Duplicate of this bug: 1378908
Duplicate of this bug: 1378909
Duplicate of this bug: 1378913
Duplicate of this bug: 1378914
Duplicate of this bug: 1378915
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.