Issue 3862 - Add EventEmitter class and implement in the FilterNotifier

lib/events.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 /**
- * Registers and emits names events.

[Wladimir Palant, 2016/03/23 16:01:07]

names => named

[Sebastian Noack, 2016/03/23 16:23:43]

Done.

+ * Registers and emits named events.
  *
  * @constructor
  */
 exports.EventEmitter = function()
 {
   this._listeners = Object.create(null);
 };
 
 exports.EventEmitter.prototype = {
   /**
@@ skipping 28 matching lines @@
   },
 
   /**
    * Adds a one time listener and returns a promise that
    * is resolved the next time the specified event is emitted.
    *
    * @return {Promise}
    */
   once: function(name)
   {
-    return new Promise(function(resolve)
+    return new Promise(resolve =>
     {
-      let listener = function()

[Wladimir Palant, 2016/03/23 16:01:07]

What about arguments? For this to be usable these need to be passed on to
resolve().

[Sebastian Noack, 2016/03/23 16:23:43]

I thought about passing arguments, but passing in an array would be kinda ugly,
and after all as far as I'm aware, this method is only useful for the
initialization code in adblockpluschrome where we need a promise that resolves
on the initial "load" action, where we don't bother about arguments anyway.

So I decided to omit arguments for now, that we can still decide whether to pass
in an array or just the first argument, without the risk of breaking anything,
once we need it. Do you disagree?

+      let listener = () =>
       {
         this.off(name, listener);
         resolve();
-      }.bind(this);
+      };
 
       this.on(name, listener);
-    }.bind(this));

[Wladimir Palant, 2016/03/23 16:01:07]

Use an arrow function instead of bind(this)?

[Sebastian Noack, 2016/03/23 16:23:43]

I agree that automatically bound arrow functions would be nicer here. However,
jsHydra doesn't do the bind-magic when converting arrow functions. Sure, I could
still use arrow functions and explicitly bind them, but that will cause them to
be bound twice on Firefox.

[Wladimir Palant, 2016/03/23 16:34:09]

It does: https://hg.adblockplus.org/jshydra/file/tip/scripts/abprewrite.js#l462

[Sebastian Noack, 2016/03/23 17:07:16]

Awesome! I didn't realize because it only does if this is used, which is even
more brilliant.

+    });
   },
 
   /**
    * Calls all previously added listeners for the given event name.
    *
    * @param {string} name
    * @param {...*}   [arg]
    */
   emit: function(name)

[Wladimir Palant, 2016/03/23 16:01:07]

Heh, I can see where you got this from but naming this emit will make using it
for Port harder...

[Sebastian Noack, 2016/03/23 16:23:43]

Well, that is just the regular node.js name for that method. As for reusing this
class for messaging.Port, I suggest to wrap the event emitter rather than
inheriting from it. That is how we did it in adblockpluschrome.

   {
     let listeners = this._listeners[name];
     if (listeners)
     {
       let args = [];
       for (let i = 1; i < arguments.length; i++)
         args.push(arguments[i]);

[Wladimir Palant, 2016/03/23 16:01:07]

Just use Array functions:

  let args = [].slice.call(arguments, 1);

I'd suggest using Array.slice() but Chrome doesn't implement it...

[Sebastian Noack, 2016/03/23 16:23:44]

Passing the arguments object to a different function causes a deoptimization on
V8.

[Wladimir Palant, 2016/03/23 16:34:09]

And rest parameters aren't supported before Chrome 47 and completely unsupported
on Safari of course :-(

 
       let currentListeners = listeners.slice();
       for (let listener of currentListeners)
         listener.apply(null, args);

[Wladimir Palant, 2016/03/23 16:01:07]

Catch errors here so that remaining listeners are still called?

[Sebastian Noack, 2016/03/23 16:23:43]

We didn't do that before for FilterStorage, Prefs, etc. Neither does node.js
catch errors when calling listeners. Moreover, any try/catch causes the whole
function to run in interpreted mode on V8. So rather not?

     }
   }
 };