Issue 3585 - Merge element hiding rules for the same domain

abp2blocklist.js

 "use strict";
 
 let readline = require("readline");
 let punycode = require("punycode");
 let tldjs = require("tldjs");
 let filterClasses = require("./adblockplus.js");
 
 let typeMap = filterClasses.RegExpFilter.typeMap;
 
 const selectorLimit = 5000;
@@ skipping 84 matching lines @@
 
 function convertElemHideFilter(filter)
 {
   let included = [];
   let excluded = [];
   let rules = [];
 
   parseDomains(filter.domains, included, excluded);
 
   if (excluded.length == 0 && !(filter.selector in elemhideSelectorExceptions))
-    return [included.map(matchDomain), filter.selector];

[Sebastian Noack, 2016/02/15 19:48:48]

How about returning an object here? That would make the calling code a little
more straight-forward, by using descriptive properties rather than bogus numeric
indices.

[kzar, 2016/02/16 15:05:08]

Done.

+    return {matchDomains: included.map(matchDomain), selector: filter.selector};
 }
 
 function toRegExp(text)
 {
   let result = "";
   let lastIndex = text.length - 1;
 
   for (let i = 0; i < text.length; i++)
   {
     let c = text[i];
@@ skipping 147 matching lines @@
     if (!hasNonASCI(rule))
       rules.push(rule);
   }
 
   let groupedElemhideFilters = new Map();
   for (let filter of elemhideFilters)
   {
     let result = convertElemHideFilter(filter);
     if (!result)
       continue;
-    let targetDomains = result[0];
-    let selector = result[1];
-
-    if (targetDomains.length == 0)
-      targetDomains = ["^https?://"];
-
-    for (let domain of targetDomains)
-    {
-      if (!groupedElemhideFilters.has(domain))

[Sebastian Noack, 2016/02/15 19:48:48]

The additional lookup is redundant. IMO, better:

  let group = groupedElemhideFilters.get(domain);
  if (!group)
  {
    group = [];
    groupedElemhideFilters.set(domain, group);
  }

[kzar, 2016/02/16 15:05:08]

Done, kind of.

We're either going to have to perform an extra lookup, an extra assignment or
set a flag like `wasEmpty` to check before performing the assignment. I don't
mind which, so far I went with the extra assignment.

-        groupedElemhideFilters.set(domain, []);
-      groupedElemhideFilters.get(domain).push(selector);
-    }
-  }
-
-  groupedElemhideFilters.forEach((selectors, domain) =>
+
+    if (result.matchDomains.length == 0)
+      result.matchDomains = ["^https?://"];
+
+    for (let matchDomain of result.matchDomains)
+    {
+      let group = groupedElemhideFilters.get(matchDomain) || [];
+      group.push(result.selector);
+      groupedElemhideFilters.set(matchDomain, group);
+    }
+  }
+
+  groupedElemhideFilters.forEach((selectors, matchDomain) =>
   {
     while (selectors.length)
     {
       addRule({
-        trigger: {"url-filter": domain},

[Sebastian Noack, 2016/02/15 19:48:47]

The variable name isn't accurate. It's not a domain but a regexp matching the
URL. Perhaps, it makes even more sense to generate the regexp here rather than
passing it around, as the variable names already indicate.

[kzar, 2016/02/16 15:05:08]

Acknowledged.

-        action: {type: "css-display-none"},
-        selector: selectors.splice(0, selectorLimit).join(", ")
+        trigger: {"url-filter": matchDomain},
+        action: {type: "css-display-none",
+                 selector: selectors.splice(0, selectorLimit).join(", ")}
       });
     }
   });
 
   for (let filter of elemhideExceptions)
     addRule(convertFilter(filter, "ignore-previous-rules", false));
 
   for (let filter of requestFilters)
     addRule(convertFilter(filter, "block", true));
   for (let filter of requestExceptions)
     addRule(convertFilter(filter, "ignore-previous-rules", true));
 
   console.log(JSON.stringify(rules, null, "\t"));
 }
 
 let rl = readline.createInterface({input: process.stdin, terminal: false});
 rl.on("line", parseFilter);
 rl.on("close", logRules);