1391562 - Convert usage of EventEmitter.emit(this [...]) to this.emit([...]) in devtools

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1137935. 

When migrating out of the sdk event-emitter, we spotted a few places in which we could directly use this.on/off/emit rather than the static version of the API: EventEmitter.on/off/emit(this, ...)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1137935#c24

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

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

Comment 2

[Julian Descottes [:jdescottes]]

Here's a first attempt to migrate all call sites relying on static methods to use instance methods when possible.
Also trying to standardize imports of devtools/shared/event-emitter as const EventEmitter for easier grepping in the future.

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

Comment 3

[Alexandre Poirot [:ochameau]]

Nice refactoring, this may really improve understanding of events!

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

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

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Nice refactoring, this may really improve understanding of events!

Hopefully! Found a couple of places where arguments passed to actors where not actually extending EventEmitter, so went back to static methods there. For the record, WebConsoleActor and ThreadActor are both taking a parentActor argument which doesn't necessarily extend EventEmitter.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

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

Comment 7

[Julian Descottes [:jdescottes]]

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf7e1d2edfcd086916eb6cc5097ceb18621186ad

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

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

Comment 9

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

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

https://reviewboard.mozilla.org/r/170188/#review178678

::: devtools/server/actors/tab.js:194
(Diff revision 4)
>   *
>   * @param connection DebuggerServerConnection
>   *        The conection to the client.
>   */
>  function TabActor(connection) {
> +  EventEmitter.decorate(this);

We're trying to reduce / remove the decoration of objects for EventEmitter, so I would avoid to add a new one unless is really necessary.

So if `TabActor` can't be a ES6 class for some reason it's fine to use `EventEmitter.decorate` instead of `class TabActor extends EventEmitter`, but we should write a comment here about why, if that's the case.

::: devtools/server/tests/unit/test_layout-reflows-observer.js:30
(Diff revision 4)
>    this.window = new MockWindow();
>    this.windows = [this.window];
>    this.attached = true;
> +
> +  // TabActor should support EventEmitter.
> +  EventEmitter.decorate(this);

Same as before. We should avoid the usage of `decorate` unless is really necessary.

::: devtools/shared/client/main.js:1313
(Diff revision 4)
>    activeAddon: null
>  };
>  
>  eventSource(DebuggerClient.prototype);
>  
>  function Request(request) {

I think this object can be safely converted in a ES6 class, we also reduce the code below. We basically simplify the whole block in:

```js
class Request extends EventEmitter {
  constructor(request) {
     this.request = request;
  }
  
  get actor() {
    return this.request.to || this.request.actor;
  }
}
```

And that should be enough, all the methods should be inherited.

Comment 10

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

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

https://reviewboard.mozilla.org/r/170188/#review178716

Looks good! Thanks for this huge refactoring! Just few comments, but nothing serious.
Maybe we should have some follow up bugs (one of them unrelated, like removing the usage of `var`, it's also inconsistent), like ensure we always extends from EventEmitter, so that we don't have to use the static methods – it's safe use `emit` on generic objects, but `on` and `once` it would be better if we could avoid it (see https://github.com/devtools-html/snippets-for-removing-the-sdk#good-practices)

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

https://reviewboard.mozilla.org/r/170188/#review178678

> We're trying to reduce / remove the decoration of objects for EventEmitter, so I would avoid to add a new one unless is really necessary.
> 
> So if `TabActor` can't be a ES6 class for some reason it's fine to use `EventEmitter.decorate` instead of `class TabActor extends EventEmitter`, but we should write a comment here about why, if that's the case.

The TabActor migration to an ES6 class deserves its own bug IMO. If you look at it's prototype, it's complex enough to prevent blindly migrating it to ES6. Can we drop and move to follow-up?

> Same as before. We should avoid the usage of `decorate` unless is really necessary.

This is just a simple test class, so ok for using ES6 here.

> I think this object can be safely converted in a ES6 class, we also reduce the code below. We basically simplify the whole block in:
> 
> ```js
> class Request extends EventEmitter {
>   constructor(request) {
>      this.request = request;
>   }
>   
>   get actor() {
>     return this.request.to || this.request.actor;
>   }
> }
> ```
> 
> And that should be enough, all the methods should be inherited.

Sure, let's try this.

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

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

Comment 13

[Julian Descottes [:jdescottes]]

Try with comments addressed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7d35002772eefca9f536a131ad604915eccee6c

Matteo: Thanks for the review! ni? for my question in comment 11

Comment 14

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

(In reply to Julian Descottes [:jdescottes] from comment #11)

> The TabActor migration to an ES6 class deserves its own bug IMO. If you look
> at it's prototype, it's complex enough to prevent blindly migrating it to
> ES6. Can we drop and move to follow-up?

Sure! As said, if there is any reason for not doing so here and using `decorate` it's fine. A follow-up bug sounds good, just add a comment where you're using the `decorate` for `TabActor` referring the follow-up bug.

Thanks!

Comment 15

[Julian Descottes [:jdescottes]]

Green try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7f384ba7c518274e9b4bd98b93638404abb8aa9

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170188/diff/5-6/

Comment 17

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0955b37b854d -d 0e5471fdbb8c: rebasing 416561:0955b37b854d "Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;r=zer0" (tip)
merging devtools/server/performance/framerate.js
merging devtools/server/performance/memory.js
merging devtools/server/performance/profiler.js
merging devtools/server/performance/recorder.js
merging devtools/server/performance/timeline.js
warning: conflicts while merging devtools/server/performance/framerate.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/performance/timeline.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8898819 [details]
Bug 1391562 - use obj.on/off/emit rather than static methods from devtools event-emitter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170188/diff/6-7/

Comment 19

[Julian Descottes [:jdescottes]]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e54ed7547a3e
use obj.on/off/emit rather than static methods from devtools event-emitter;r=zer0

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e54ed7547a3e