Issue 4329 - Add $genericblock support

Issue 4329 - Add $genericblock support

This affects 23,579 rules in EasyList+AA

[Manish Jethani, 2017-05-19 01:47:26]

Patch Set 1

[Manish Jethani, 2017-05-19 02:12:32]

Patch Set 2: Add unit tests

[kzar, 2017-05-19 12:05:39]

LGTM

https://codereview.adblockplus.org/29441592/diff/29441595/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29441592/diff/29441595/lib/abp2blocklist.j...
lib/abp2blocklist.js:394: if (filter.contentType & typeMap.GENERICBLOCK)
Nit: The indentation looks weird here, I guess it's actually the previous lines
that are wrong though.

[Manish Jethani, 2017-05-20 00:22:10]

https://codereview.adblockplus.org/29441592/diff/29441595/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29441592/diff/29441595/lib/abp2blocklist.j...
lib/abp2blocklist.js:394: if (filter.contentType & typeMap.GENERICBLOCK)
On 2017/05/19 12:05:38, kzar wrote:
> Nit: The indentation looks weird here, I guess it's actually the previous
lines
> that are wrong though.

Yeah, I'm not sure how it ended up that way. I'll change it as part of the other
change for $generichide since I'm already modifying those lines.

[Manish Jethani, 2017-05-20 02:00:57]

Patch Set 3: Fix bug: Make a copy of the exceptionsDomains array

We have to make a copy of this array because the code that follows will modify
it.

[Sebastian Noack, 2017-05-20 06:25:29]

https://codereview.adblockplus.org/29441592/diff/29442771/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29441592/diff/29442771/lib/abp2blocklist.j...
lib/abp2blocklist.js:256: let excluded = exceptionDomains &&
exceptionDomains.slice() || [];
Nit: The ternary operator seems more appropriate here.

[Manish Jethani, 2017-05-20 19:30:23]

Patch Set 4: Add $genericblock domains after filter-specific exception domains

https://codereview.adblockplus.org/29441592/diff/29442771/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29441592/diff/29442771/lib/abp2blocklist.j...
lib/abp2blocklist.js:256: let excluded = exceptionDomains &&
exceptionDomains.slice() || [];
On 2017/05/20 06:25:28, Sebastian Noack wrote:
> Nit: The ternary operator seems more appropriate here.

Actually now that I think of it, the $genericblock domains should be added after
the filter-specific exception domains. I've updated this now.

[Manish Jethani, 2017-05-20 19:32:53]

On 2017/05/20 19:30:23, Manish Jethani wrote:

> Actually now that I think of it, the $genericblock domains should be added
after
> the filter-specific exception domains. I've updated this now.

Reason for this is that it would be easier to review the rules when you're
reviewing them manually (e.g. Mario, Arthur), to make changes and so on, and
also seems more logical to add the additional stuff at the end rather than at
the beginning.

[Sebastian Noack, 2017-05-21 20:44:35]

https://codereview.adblockplus.org/29441592/diff/29443570/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29441592/diff/29443570/lib/abp2blocklist.j...
lib/abp2blocklist.js:261: excluded = excluded.concat(exceptionDomains);
I wonder whether it would be better to avoid concat() + reassignment and rather
change the array in-place?

  excluded.push(...exceptionDomains);

[Manish Jethani, 2017-05-21 21:03:57]

https://codereview.adblockplus.org/29441592/diff/29443570/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29441592/diff/29443570/lib/abp2blocklist.j...
lib/abp2blocklist.js:261: excluded = excluded.concat(exceptionDomains);
On 2017/05/21 20:44:35, Sebastian Noack wrote:
> I wonder whether it would be better to avoid concat() + reassignment and
rather
> change the array in-place?
> 
>   excluded.push(...exceptionDomains);

JSHydra does not support the spread operator. It's also supposed to be
significantly slower this way, if we want to run this in the browser.

[Sebastian Noack, 2017-05-21 21:05:04]

LGTM

[Manish Jethani, 2017-05-21 21:27:51]

Patch Set 5: Rebase

[Sebastian Noack, 2017-05-21 21:31:23]

Still LGTM