803067 - EventEmitter should have a decorator that isn't called 'new'

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

This code is getting common:

    function SomeClass() {
      new EventEmitter(this);
    }

This sucks planets out of orbit.
Better:

    function SomeOtherClass() {
      EventEmitter.decorate(this);
    }

We should fix this before it gets to intrenched.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

I'd like to take this bug.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Yura! Paul Rouget will be your host :)

Comment 3

[Paul Rouget [:paul]]

(In reply to yura.zenevich from comment #1)
> I'd like to take this bug.

Yura, do you understand the scope of this bug? Do you need any more details?

The idea is to add a method to EventEmitter (to the EventEmitter object directly, not to the prototype) that will do what the current constructor does).

Code is here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/EventEmitter.jsm

Once this is one, you need to find all the "new EventEmitter(this)" occurrences  and replace them with "EventEmitter.decorate(this)".

Thank you!

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Thanks Paul, that's what I was thinking.
Do you also want me to take a look at places where the new is used as well?
Thanks.

Comment 5

[Paul Rouget [:paul]]

(In reply to yura.zenevich from comment #4)
> Thanks Paul, that's what I was thinking.
> Do you also want me to take a look at places where the new is used as well?
> Thanks.

I think we use "new" all the time. So yeah. Kill them all, and use decorate instead.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: This is the patch that addresses the issue of using new with EventEmitter (objectToDecorate).

This is to get some feedback regarding the approach taken in devtools/responsivedesign/responsivedesign.jsm

I was not sure what exactly the purpose of events getter was (that resulted in creation of, I'm assuming, sudo-private _eventEmitter property). So I used Object.defineProperty to take advantage of memoization withing the getter to get the same result without the need for public _eventEmitter.

Tests are updated as well (with new decorate approach).

Comment 7

[Paul Rouget [:paul]]

In the future, when you want some feedback, in "Details", add the flag "f?" with ":paul" in the text field.

Comment 8

[Paul Rouget [:paul]]

Comment on attachment 691214 [details] [diff] [review]
This is the patch that addresses the issue of using new with EventEmitter (objectToDecorate).

Thanks! This looks great!


>diff --git a/browser/devtools/responsivedesign/responsivedesign.jsm b/browser/devtools/responsivedesign/responsivedesign.jsm
>--- a/browser/devtools/responsivedesign/responsivedesign.jsm
>+++ b/browser/devtools/responsivedesign/responsivedesign.jsm
>@@ -64,25 +64,32 @@ this.ResponsiveUIManager = {
>         if (aTab.__responsiveUI) {
>           aTab.__responsiveUI.close();
>         }
>         break;
>       case "resize toggle":
>           this.toggle(aWindow, aTab);
>       default:
>     }
>-  },
>+  }
>+}
> 
>-  get events() {
>-    if (!this._eventEmitter) {
>-      this._eventEmitter = new EventEmitter();
>-    }
>-    return this._eventEmitter;
>-  },
>-}
>+Object.defineProperty(ResponsiveUIManager, "events", {
>+  get: (function () {
>+    var events;
>+    return function () {
>+      if (!events) {
>+        events = {};
>+        EventEmitter.decorate(events);
>+      }
>+      return events;
>+    };
>+  })(),
>+  enumerable: true
>+});

Don't use decorate here, just keep the original code.
Or maybe you can kill ResponsiveUIManager.events and just decorate ResponsiveUIManager.
Up to you.

>+EventEmitter.decorate = function EventEmitter_decorate (anObjectToDecorate) {
>+  var emitter;

Use `let`, not `var`.

>+  if (!anObjectToDecorate) {
>+    return;
>   }

This test is not interesting anymore. Remove it.

>-}
>+  emitter = new EventEmitter();
>+  anObjectToDecorate.on = emitter.on.bind(emitter);
>+  anObjectToDecorate.off = emitter.off.bind(emitter);
>+  anObjectToDecorate.once = emitter.once.bind(emitter);
>+  anObjectToDecorate.emit = emitter.emit.bind(emitter);
>+};

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Revised patch for the issue.

Updated the patch. ResponsiveUIManager is now an EventEmitter decorated component itself.

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 691321 [details] [diff] [review]
Revised patch for the issue.

Looks right. Thanks a lot!
We need a try run before landing. I'll take care of that soon.

> diff --git a/browser/devtools/shared/EventEmitter.jsm b/browser/devtools/shared/EventEmitter.jsm
> +EventEmitter.decorate = function EventEmitter_decorate (anObjectToDecorate) {
> +  let emitter = new EventEmitter();
> +  anObjectToDecorate.on = emitter.on.bind(emitter);
> +  anObjectToDecorate.off = emitter.off.bind(emitter);
> +  anObjectToDecorate.once = emitter.once.bind(emitter);
> +  anObjectToDecorate.emit = emitter.emit.bind(emitter);
> +};

Nit: s/anObjectToDecorate/aObjectToDecorate/
"a" mean "argument".

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Latest patch to address the issue (updated the argument name).

Renamed anObjectToDecorate to aObjectToDecorate. Updated the comment for EventEmitter.decorate .

Comment 12

[Paul Rouget [:paul]]

Comment on attachment 691340 [details] [diff] [review]
Latest patch to address the issue (updated the argument name).

That looks right.

Can I ask you to rebase your patch on the latest version of fx-team? We made some changes and it broke your patch (sorry for that).

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Updated patch to the latest version of fx-team.

Comment 14

[Paul Rouget [:paul]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=b949b7e30fe4

Comment 15

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/c7c3914d6127

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

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