The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jwalker, Assigned: yzen)

Tracking

unspecified
Firefox 20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.

Updated

4 years ago
Whiteboard: [good first bug][mentor=paul][lang=js]
(Assignee)

Comment 1

4 years ago
I'd like to take this bug.
Thanks Yura! Paul Rouget will be your host :)
Assignee: nobody → yura.zenevich

Comment 3

4 years ago
(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!
(Assignee)

Comment 4

4 years ago
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

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
Created attachment 691214 [details] [diff] [review]
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

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

Comment 8

4 years ago
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);
>+};
Attachment #691214 - Flags: feedback+
(Assignee)

Comment 9

4 years ago
Created attachment 691321 [details] [diff] [review]
Revised patch for the issue.

Updated the patch. ResponsiveUIManager is now an EventEmitter decorated component itself.
Attachment #691214 - Attachment is obsolete: true
Attachment #691321 - Flags: review?(paul)
Flags: needinfo?

Comment 10

4 years ago
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".
Attachment #691321 - Flags: review?(paul) → review+
Flags: needinfo?
(Assignee)

Comment 11

4 years ago
Created attachment 691340 [details] [diff] [review]
Latest patch to address the issue (updated the argument name).

Renamed anObjectToDecorate to aObjectToDecorate. Updated the comment for EventEmitter.decorate .
Attachment #691321 - Attachment is obsolete: true
Attachment #691340 - Flags: review?(paul)

Comment 12

4 years ago
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).
Attachment #691340 - Flags: review?(paul)
(Assignee)

Comment 13

4 years ago
Created attachment 692312 [details] [diff] [review]
Updated patch to the latest version of fx-team.
Attachment #691340 - Attachment is obsolete: true
Attachment #692312 - Flags: review?(paul)

Comment 14

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

Updated

4 years ago
Attachment #692312 - Flags: review?(paul) → review+

Updated

4 years ago
Whiteboard: [good first bug][mentor=paul][lang=js] → [land-in-fx-team]

Comment 15

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/c7c3914d6127
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c7c3914d6127
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.