Issue 29340694: Issue 3956 - Convert domain whitelisting filters (Closed)
|
Created:
April 20, 2016, 5:09 p.m. by kzar Modified:
May 17, 2016, 11:36 a.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 3956 - Convert domain whitelisting filters
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed feedback #
Total comments: 2
Patch Set 3 : Avoid creating so many temporary arrays #
Total comments: 4
Patch Set 4 : Fix whitelisting request type logic #MessagesTotal messages: 8
Patch Set 1 For EasyList this patchset results in the addition of a 8 new rules as before we ignored whitelisting filters with only the $document option. Otherwise no changes. For the special case of filters with only the $document option and nothing after the hostname we now generate appropriate rules. This fixes the domain whitelisting feature of Adblock Plus. (It also doesn't seem to slow the script down when run on EasyList.)
https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.js File lib/abp2blocklist.js (right): https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:227: if (filter.contentType == typeMap.DOCUMENT && parsed.justHostname) For filters like example.com$document,image we would have to generate both, one rule with if-domain and one with url-filter. https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:228: return {trigger: {"url-filter": ".*", Nit: Mind wrapping the nested object for better readability? https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:228: return {trigger: {"url-filter": ".*", Wouldn't an empty string be sufficient as url-filter to match all URLs? https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:234: // Limit rules to to HTTP(S) URLs Typo: to to
Patch Set 2 : Addressed feedback https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.js File lib/abp2blocklist.js (right): https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:227: if (filter.contentType == typeMap.DOCUMENT && parsed.justHostname) On 2016/05/12 12:12:25, Sebastian Noack wrote: > For filters like example.com$document,image we would have to generate both, one > rule with if-domain and one with url-filter. Done. https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:228: return {trigger: {"url-filter": ".*", On 2016/05/12 12:12:26, Sebastian Noack wrote: > Nit: Mind wrapping the nested object for better readability? Done. https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:228: return {trigger: {"url-filter": ".*", On 2016/05/12 12:12:26, Sebastian Noack wrote: > Wouldn't an empty string be sufficient as url-filter to match all URLs? Unfortunately this causes a "Extension compilation failed: Invalid url-filter object." exception. https://codereview.adblockplus.org/29340694/diff/29340695/lib/abp2blocklist.j... lib/abp2blocklist.js:234: // Limit rules to to HTTP(S) URLs On 2016/05/12 12:12:26, Sebastian Noack wrote: > Typo: to to Done.
https://codereview.adblockplus.org/29340694/diff/29341424/lib/abp2blocklist.js File lib/abp2blocklist.js (right): https://codereview.adblockplus.org/29340694/diff/29341424/lib/abp2blocklist.j... lib/abp2blocklist.js:463: addRules([{ We are creating quite a few temporary arrays now. I suppose it will be faster, and perhaps also simpler, if we pass the "rules" array to convertFilter() rather than having it return a new array. And then iterate over all generated rules in the end to remove those that that have non-ascii characters.
Patch Set 3 : Avoid creating so many temporay arrays https://codereview.adblockplus.org/29340694/diff/29341424/lib/abp2blocklist.js File lib/abp2blocklist.js (right): https://codereview.adblockplus.org/29340694/diff/29341424/lib/abp2blocklist.j... lib/abp2blocklist.js:463: addRules([{ On 2016/05/17 10:17:54, Sebastian Noack wrote: > We are creating quite a few temporary arrays now. I suppose it will be faster, > and perhaps also simpler, if we pass the "rules" array to convertFilter() rather > than having it return a new array. And then iterate over all generated rules in > the end to remove those that that have non-ascii characters. Done.
https://codereview.adblockplus.org/29340694/diff/29341470/lib/abp2blocklist.js File lib/abp2blocklist.js (right): https://codereview.adblockplus.org/29340694/diff/29341470/lib/abp2blocklist.j... lib/abp2blocklist.js:240: if (filter.contentType == typeMap.DOCUMENT) What if the filter is @@||example.com$document,elemhide? Then we wouldn't bail out here, though we don't have to create another rule either. I think we have to check for IMAGE, SUBDOCUMENT, SCRIPT, etc. explicitly below. Perhaps move the respective mask to a const to avoid duplication. https://codereview.adblockplus.org/29340694/diff/29341470/lib/abp2blocklist.j... lib/abp2blocklist.js:275: return rules; You don't have to return the rules here anymore.
Patch Set 4 : Fix whitelisting request type logic https://codereview.adblockplus.org/29340694/diff/29341470/lib/abp2blocklist.js File lib/abp2blocklist.js (right): https://codereview.adblockplus.org/29340694/diff/29341470/lib/abp2blocklist.j... lib/abp2blocklist.js:240: if (filter.contentType == typeMap.DOCUMENT) On 2016/05/17 10:55:24, Sebastian Noack wrote: > What if the filter is mailto:@@||example.com$document,elemhide? > > Then we wouldn't bail out here, though we don't have to create another rule > either. I think we have to check for IMAGE, SUBDOCUMENT, SCRIPT, etc. explicitly > below. Perhaps move the respective mask to a const to avoid duplication. Done. https://codereview.adblockplus.org/29340694/diff/29341470/lib/abp2blocklist.j... lib/abp2blocklist.js:275: return rules; On 2016/05/17 10:55:24, Sebastian Noack wrote: > You don't have to return the rules here anymore. Oops, Done.
LGTM 
|
