Last Comment Bug 803067 - EventEmitter should have a decorator that isn't called 'new'
: EventEmitter should have a decorator that isn't called 'new'
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 20
Assigned To: Yura Zenevich [:yzen]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-18 06:36 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-12-19 00:10 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This is the patch that addresses the issue of using new with EventEmitter (objectToDecorate). (14.60 KB, patch)
2012-12-11 21:33 PST, Yura Zenevich [:yzen]
paul: feedback+
Details | Diff | Splinter Review
Revised patch for the issue. (18.35 KB, patch)
2012-12-12 06:16 PST, Yura Zenevich [:yzen]
paul: review+
Details | Diff | Splinter Review
Latest patch to address the issue (updated the argument name). (18.36 KB, patch)
2012-12-12 07:19 PST, Yura Zenevich [:yzen]
no flags Details | Diff | Splinter Review
Updated patch to the latest version of fx-team. (18.10 KB, patch)
2012-12-14 08:05 PST, Yura Zenevich [:yzen]
paul: review+
Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-18 06:36:06 PDT
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] 2012-12-11 06:19:31 PST
I'd like to take this bug.
Comment 2 David Bolter [:davidb] ***PTO until 29th*** 2012-12-11 06:20:48 PST
Thanks Yura! Paul Rouget will be your host :)
Comment 3 Paul Rouget [:paul] 2012-12-11 06:35:24 PST
(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] 2012-12-11 06:58:31 PST
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] 2012-12-11 07:01:46 PST
(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] 2012-12-11 21:33:17 PST
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 Paul Rouget [:paul] 2012-12-12 00:53:36 PST
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] 2012-12-12 00:59:55 PST
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] 2012-12-12 06:16:25 PST
Created attachment 691321 [details] [diff] [review]
Revised patch for the issue.

Updated the patch. ResponsiveUIManager is now an EventEmitter decorated component itself.
Comment 10 Paul Rouget [:paul] 2012-12-12 06:34:48 PST
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] 2012-12-12 07:19:03 PST
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 .
Comment 12 Paul Rouget [:paul] 2012-12-14 04:43:37 PST
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] 2012-12-14 08:05:20 PST
Created attachment 692312 [details] [diff] [review]
Updated patch to the latest version of fx-team.
Comment 14 Paul Rouget [:paul] 2012-12-14 08:45:27 PST
try run: https://tbpl.mozilla.org/?tree=Try&rev=b949b7e30fe4
Comment 16 Panos Astithas [:past] 2012-12-19 00:10:42 PST
https://hg.mozilla.org/mozilla-central/rev/c7c3914d6127

Note You need to log in before you can comment on or make changes to this bug.