Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: abp2blocklist.js

Issue 29336349: Issue 3585 - Merge element hiding rules for the same domain (Closed)
Patch Set: Rebased to use ES2015 features everywhere, limit to 5000 selectors per rule and comma delimit selec… Created Feb. 15, 2016, 6:17 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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 mak
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:
kzar 2016/02/16 15:05:08 Done, kind of. We're either going to have to perf
+ 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 domai
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));
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld