Issue 5735 - Use JS Map instead of Object for property domains of Filter objects

lib/elemHide.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 48 matching lines @@
 /**
  * This array caches the values of filterKeyBySelector table (filterIds for
  * selectors which unconditionally apply on all domains). It will be null if the
  * cache needs to be rebuilt.
  */
 let unconditionalFilterKeys = null;
 
 /**
  * Object to be used instead when a filter has a blank domains property.
  */
-let defaultDomains = Object.create(null);
-defaultDomains[""] = true;
+let defaultDomains = new Map();
+defaultDomains.set("", true);

[Wladimir Palant, 2017/09/21 08:11:44]

You can initialize the Map object immediately: new Map([["", true]])

[sergei, 2017/09/21 10:50:33]

Done.

 
 /**
  * Lookup table, keys are known element hiding exceptions
  * @type {Object}
  */
 let knownExceptions = Object.create(null);
 
 /**
  * Lookup table, lists of element hiding exceptions by selector
  * @type {Object}
@@ skipping 16 matching lines @@
     filterKeyBySelector = Object.create(null);
     unconditionalSelectors = unconditionalFilterKeys = null;
     knownExceptions = Object.create(null);
     exceptions = Object.create(null);
     FilterNotifier.emit("elemhideupdate");
   },
 
   _addToFiltersByDomain(key, filter)
   {
     let domains = filter.domains || defaultDomains;
-    for (let domain in domains)
+    for (let [domain, isIncluded] of domains)
     {
       let filters = filtersByDomain[domain];
       if (!filters)
         filters = filtersByDomain[domain] = Object.create(null);

[Wladimir Palant, 2017/09/21 08:11:44]

Shouldn't this be turned in to a Map object as well while we are at it?

[sergei, 2017/09/21 10:50:33]

No, I find the change of filtersByDomain self-contained enough because the
current change is rather aimed at filterClasses module and the change in
elemHide.js is a consequence. In addition smaller commits are always better, for
instance we can easier compare the contribution in the performance and memory
usage from a change of type of one object with many entries with the change of
types of many objects with not so many entries.

 
-      if (domains[domain])
+      if (isIncluded)
         filters[key] = filter;
       else
         filters[key] = false;
     }
   },
 
   /**
    * Add a new element hiding filter
    * @param {ElemHideFilter} filter
    */
@@ skipping 51 matching lines @@
     if (filterKeyBySelector[filter.selector] == key)
     {
       delete filterKeyBySelector[filter.selector];
       unconditionalSelectors = unconditionalFilterKeys = null;
       return;
     }
 
     // We haven't found this filter in unconditional filters, look in
     // filtersByDomain.
     let domains = filter.domains || defaultDomains;
-    for (let domain in domains)
+    for (let domain of domains.keys())
     {
       let filters = filtersByDomain[domain];
       if (filters)
         delete filters[key];
     }
   },
 
   /**
    * Removes an element hiding filter
    * @param {ElemHideFilter} filter
@@ skipping 191 matching lines @@
 
       let nextDot = currentDomain.indexOf(".");
       currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
     }
 
     if (provideFilterKeys)
       return [selectors, filterKeys];
     return selectors;
   }
 };