Issue 4499 - Consider $generichide exceptions when applying element hiding rules

Unfortunately, I completely forgot about $generichide in my previous commit - if a $generichide exception applies the "global" stylesheet shouldn`t be injected at all and the "local" one should be reduced to specific filters only.

[kzar, 2016-10-07 09:30:21]

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide....
lib/child/elemHide.js:347: let specificOnly = filter;
I don't understand this line, isn't filter being given as filter.text by
elemHideFF.js for "elemhideEnabled"? How can we use the filter.text for
specificOnly?

[Wladimir Palant, 2016-10-07 09:37:50]

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide....
lib/child/elemHide.js:347: let specificOnly = filter;
On 2016/10/07 09:30:21, kzar wrote:
> I don't understand this line, isn't filter being given as filter.text by
> elemHideFF.js for "elemhideEnabled"? How can we use the filter.text for
> specificOnly?

There are two kinds of filters that we can get in the response -
$elemhide/$document filters (disable element hiding altogether) or $generichide
filters (disable only generic filters). If enabled is true then it cannot be the
former. So if we have a filter at this point then a $generichide filter applies.

Not sure how this can be made more obvious, maybe send true/false/"specificonly"
for enabled? Combining boolean and string in the same field seems ugly. And I
don't really want to add more data to be sent.

[kzar, 2016-10-07 09:53:34]

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide....
lib/child/elemHide.js:347: let specificOnly = filter;
On 2016/10/07 09:37:49, Wladimir Palant wrote:
> On 2016/10/07 09:30:21, kzar wrote:
> > I don't understand this line, isn't filter being given as filter.text by
> > elemHideFF.js for "elemhideEnabled"? How can we use the filter.text for
> > specificOnly?
> 
> There are two kinds of filters that we can get in the response -
> $elemhide/$document filters (disable element hiding altogether) or
$generichide
> filters (disable only generic filters). If enabled is true then it cannot be
the
> former. So if we have a filter at this point then a $generichide filter
applies.
> 
> Not sure how this can be made more obvious, maybe send
true/false/"specificonly"
> for enabled? Combining boolean and string in the same field seems ugly. And I
> don't really want to add more data to be sent.

Ah, I finally wrapped my head around it :). Well I suppose `let specificOnly =
!!filter;` might be a bit clearer? I guess a small comment here could also help
explain the thinking.

(I wonder why we need to check the contents of `filter` at all, won't it always
be truthy if enabled is true?)

[kzar, 2016-10-07 10:15:29]

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide....
lib/child/elemHide.js:347: let specificOnly = filter;
On 2016/10/07 09:53:31, kzar wrote:
> On 2016/10/07 09:37:49, Wladimir Palant wrote:
> > On 2016/10/07 09:30:21, kzar wrote:
> > > I don't understand this line, isn't filter being given as filter.text by
> > > elemHideFF.js for "elemhideEnabled"? How can we use the filter.text for
> > > specificOnly?
> > 
> > There are two kinds of filters that we can get in the response -
> > $elemhide/$document filters (disable element hiding altogether) or
> $generichide
> > filters (disable only generic filters). If enabled is true then it cannot be
> the
> > former. So if we have a filter at this point then a $generichide filter
> applies.
> > 
> > Not sure how this can be made more obvious, maybe send
> true/false/"specificonly"
> > for enabled? Combining boolean and string in the same field seems ugly. And
I
> > don't really want to add more data to be sent.
> 
> Ah, I finally wrapped my head around it :). Well I suppose `let specificOnly =
> !!filter;` might be a bit clearer? I guess a small comment here could also
help
> explain the thinking.
> 
> (I wonder why we need to check the contents of `filter` at all, won't it
always
> be truthy if enabled is true?)
> 

(OK, I missed the final `return {enabled: true};` before. Now I see why you need
to check filter.)

[Wladimir Palant, 2016-10-07 10:53:05]

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356270/diff/29356271/lib/child/elemHide....
lib/child/elemHide.js:347: let specificOnly = filter;
On 2016/10/07 09:53:31, kzar wrote:
> Ah, I finally wrapped my head around it :). Well I suppose `let specificOnly =
> !!filter;` might be a bit clearer? I guess a small comment here could also
help
> explain the thinking.

Done.

[kzar, 2016-10-07 10:59:18]

LGTM