Issue 3585 - Merge element hiding rules for the same domain

abp2blocklist.js

Index: abp2blocklist.js
diff --git a/abp2blocklist.js b/abp2blocklist.js
index 6e757873b3652596645a443d44963bbb28d65c15..1f584081ce4af1001ee6db31b0a6317ed6bb59f7 100644
--- a/abp2blocklist.js
+++ b/abp2blocklist.js
@@ -1,3 +1,5 @@
+"use strict";
+
 var readline = require("readline");
 var punycode = require("punycode");
 var tldjs = require("tldjs");
@@ -98,26 +100,7 @@ function convertElemHideFilter(filter)
   parseDomains(filter.domains, included, excluded);
 
   if (excluded.length == 0 && !(filter.selector in elemhideSelectorExceptions))
-  {
-    var action = {
-      type: "css-display-none",
-      selector: filter.selector
-    };
-
-    for (var i = 0; i < included.length; i++)
-      rules.push({
-          trigger: {"url-filter": matchDomain(included[i])},
-          action: action
-      });
-
-    if (included.length == 0)
-      rules.push({
-          trigger: {"url-filter": "^https?://"},
-          action: action
-      });
-  }
-
-  return rules;
+    return [included.map(matchDomain), filter.selector];
 }
 
 function toRegExp(text)
@@ -283,14 +266,41 @@ 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)

[Sebastian Noack, 2016/02/15 14:33:11]

Does these ES2015 features even work in node.js without any additional command
line options? I remember that when I initially wrote that code I couldn't use
let statements.

Moreover, regardless of ES2015 support in the latest node version, I think we
should either stick to var, or use let consistently in the whole file.

[kzar, 2016/02/15 18:19:20]

(As discussed in IRC we will use ES2015 features that are supported by node.js
stable without special arguments.)

+  {
+    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))
+        groupedElemhideFilters.set(domain, []);
+      groupedElemhideFilters.get(domain).push(selector);
+    }
+  }
+
+  groupedElemhideFilters.forEach((selectors, domain) =>
+  {
+    let rule = {trigger: {"url-filter": domain},
+                action: {type: "css-display-none"}};
+
+    if (selectors.length == 1)
+      rule["action"]["selector"] = selectors[0];
+    else
+      rule["action"]["selector"] = ":matches(" + selectors.join(", ") + ")";

[kzar, 2016/02/13 19:33:24]

Note: I wasn't sure if we need to escape the selectors here.

[Sebastian Noack, 2016/02/15 14:33:11]

Well, if any CSS selector is invalid it will break the whole group of selectors.
But that is the same with the logic we have on Chrome. So I don't think we need
to bother. And as far as I understand :matches, it's not possible to prevent
that by escaping as we need raw selectors here.

However, I wonder whether we even need :matches() or whether we can simply join
the selectors by comma. It seems that is what AdBlock is doing. While looking at
their code I also figured that they never merge more than 1000 selectors into
one rule. Perhaps, also Dean can give some advice here?

[kzar, 2016/02/15 18:19:20]

Acknowledged.

+
+    addRule(rule);
+  });
 
-  //for (i = 0; i < elemhideFilters.length; i++)
-  //  convertElemHideFilter(elemhideFilters[i]).forEach(addRule);
-  //for (i = 0; i < elemhideExceptions.length; i++)
-  //  addRule(convertFilter(elemhideExceptions[i], "ignore-previous-rules", false));
+  for (i = 0; i < elemhideExceptions.length; i++)
+    addRule(convertFilter(elemhideExceptions[i], "ignore-previous-rules", false));
 
   for (i = 0; i < requestFilters.length; i++)
     addRule(convertFilter(requestFilters[i], "block", true));