Noissue - Make _listeners a Set object

ext/common.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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";
 
 (function()
 {
   window.ext = {};
 
   let EventTarget = ext._EventTarget = function()
   {
-    this._listeners = [];
+    this._listeners = new Set();
   };
   EventTarget.prototype = {
     addListener(listener)
     {
-      if (this._listeners.indexOf(listener) == -1)
-        this._listeners.push(listener);
+      this._listeners.add(listener);
     },
     removeListener(listener)
     {
-      let idx = this._listeners.indexOf(listener);
-      if (idx != -1)
-        this._listeners.splice(idx, 1);
+      this._listeners.delete(listener);
     },
     _dispatch(...args)
     {
       let results = [];
-      let listeners = this._listeners.slice();
+      let listeners = [...this._listeners];

[Sebastian Noack, 2017/04/12 12:32:14]

Is turning the set into an array even necessary, can't you iterate over the set
directly below?

[Manish Jethani, 2017/04/12 12:37:50]

Yeah, I'm trying to be compatible with the current implementation. If a value is
removed from the set while you're iterating over it, it'll be skipped. It seems
we want to call all the listeners in the current iteration, even if the set is
modified.

https://hg.adblockplus.org/adblockpluschrome/rev/f81e41006b94

[Sebastian Noack, 2017/04/12 12:42:36]

I see. This might have been the reason why the list of listeners has been copied
in the first place.

[Wladimir Palant, 2017/04/12 14:43:02]

It is. However, is that an efficient way of copying a set? I can imagine new
Set(this._listeners) being considerably better.

[Sebastian Noack, 2017/04/12 14:49:30]

We don't need a set, an array is sufficient. So if [...] has performance issues
(I wouldn't know why it should), we should rather use Array.from(), I guess.

 
       for (let listener of listeners)
         results.push(listener(...args));
 
       return results;
     }
   };
 
   // Workaround since HTMLCollection and NodeList didn't have iterator support
   // before Chrome 51.
@@ skipping 18 matching lines @@
       return chrome.extension.getBackgroundPage();
     }
   };
 
 
   /* Utils */
 
   ext.getURL = chrome.extension.getURL;
   ext.i18n = chrome.i18n;
 }());