Noissue - Use Map to store EventEmitter listeners

lib/events.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
@@ skipping 18 matching lines @@
 
 exports.EventEmitter.prototype = {
   /**
    * Adds a listener for the specified event name.
    *
    * @param {string}   name
    * @param {function} listener
    */
   on(name, listener)
   {
-    if (this._listeners.has(name))

[Manish Jethani, 2018/04/01 08:44:16]

It's usually not worth calling Map.has if you're going to call Map.get anyway.
It would be better to do this instead:

  let listeners = this._listeners.get(name);
  if (listeners)
    listeners.push(listener);
  else
    this._listeners.set(name, [listener]);

[kzar, 2018/04/03 17:27:55]

Done.

-      this._listeners.get(name).push(listener);
+    let listeners = this._listeners.get(name);
+    if (listeners)
+      listeners.push(listener);
     else
       this._listeners.set(name, [listener]);
   },
 
   /**
    * Removes a listener for the specified event name.
    *
    * @param {string}   name
    * @param {function} listener
    */
   off(name, listener)
   {
     let listeners = this._listeners.get(name);

[Manish Jethani, 2018/04/01 08:44:16]

While we're at it, perhaps it would be a good idea to make listeners a Set
object. Two benefits: (1) no duplicate listeners (similar to other event APIs),
and (2) faster deletion.

[kzar, 2018/04/03 17:27:55]

Good idea, but I'd rather not since this would change behaviour subtly and if
we're unlucky lead to a regression somewhere. I would support the change in the
future if someone™ takes the time to verify nothing (including libadblockplus)
breaks!

[Sebastian Noack, 2018/04/03 20:42:34]

What change of behavior are you concerned about? Mind that Set() objects
preserve the order in which values are added. So the only difference in behavior
would be that if the same listener is attempted to be added more than once, it
will effectively only be registered once. I cannot think of any situation where
it would be intended that the same listener is called more than once. On the
other hand it would notably speedup listener removal (and simplifies the code a
bit). So I think the risk here (if any) would be justified.

FWIW, replacing a plain object with a Map() already introduces (similarly small)
changes in behavior, e.g. keys are no longer coerced to strings.

[Manish Jethani, 2018/04/04 06:54:49]

Dave has a point that this would be more likely to break something. For example
every time you open a window you might add the same "close" listener, which
operates on the event's target and removes itself. The number of times it's
added would be equal to the number of windows open. Now this code would break
because the listener would get removed after the first window is closed.

So perhaps it should be a separate patch.
This is less serious in my opinion.

     if (listeners)
     {
       let idx = listeners.indexOf(listener);
       if (idx != -1)
         listeners.splice(idx, 1);
     }
   },
 
   /**
    * Adds a one time listener and returns a promise that
@@ skipping 33 matching lines @@
    * @param {string} name
    * @param {...*}   [arg]
    */
   emit(name, ...args)
   {
     let listeners = this.listeners(name);
     for (let listener of listeners)
       listener(...args);
   }
 };