Issue 3585 - Merge element hiding rules for the same domain

abp2blocklist.js

Index: abp2blocklist.js
diff --git a/abp2blocklist.js b/abp2blocklist.js
index ff781e695b4e9480f54bf004ddb5a35f2885f114..07199e1eb57783e73cb5f083154e9a8923fe406f 100644
--- a/abp2blocklist.js
+++ b/abp2blocklist.js
@@ -7,6 +7,8 @@ let filterClasses = require("./adblockplus.js");
 
 let typeMap = filterClasses.RegExpFilter.typeMap;
 
+const selectorLimit = 5000;
+
 let requestFilters = [];
 let requestExceptions = [];
 let elemhideFilters = [];
@@ -100,26 +102,7 @@ function convertElemHideFilter(filter)
   parseDomains(filter.domains, included, excluded);
 
   if (excluded.length == 0 && !(filter.selector in elemhideSelectorExceptions))
-  {
-    let action = {
-      type: "css-display-none",
-      selector: filter.selector
-    };
-
-    for (let domain of included)
-      rules.push({
-          trigger: {"url-filter": matchDomain(domain)},
-          action: action
-      });
-
-    if (included.length == 0)
-      rules.push({
-          trigger: {"url-filter": "^https?://"},
-          action: action
-      });
-  }
-
-  return rules;
+    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.

 }
 
 function toRegExp(text)
@@ -281,14 +264,40 @@ function logRules()
       rules.push(rule);
   }
 
-  // HACK: We ignore element hiding filter for now to get the list of
-  // rules down below 50K. This limit is enforced by iOS and Safari.
-  // To be undone with https://issues.adblockplus.org/ticket/3585
+  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) =>
+  {
+    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(", ")
+      });
+    }
+  });
 
-  //for (let filter of elemhideFilters)
-  //  convertElemHideFilter(filter).forEach(addRule);
-  //for (let filter of elemhideExceptions)
-  //  addRule(convertFilter(filter, "ignore-previous-rules", false));
+  for (let filter of elemhideExceptions)
+    addRule(convertFilter(filter, "ignore-previous-rules", false));
 
   for (let filter of requestFilters)
     addRule(convertFilter(filter, "block", true));