Issue 4658 - Incorporate element hiding emulation filters

Issue 4658 - Incorporate element hiding emulation filters

[Wladimir Palant, 2016-12-13 14:44:28]

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/content/ui/f...
File chrome/content/ui/filters-backup.js (right):

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/content/ui/f...
chrome/content/ui/filters-backup.js:328: minVersion = "2.9";
No, as far as our users are concerned the syntax is supported starting with
2.7.3 - different class name changes nothing. Once the :has syntax is added we
will need to bump the version here, any only for filters actually using :has.

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
File chrome/locale/en-US/global.properties (right):

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
chrome/locale/en-US/global.properties:35: filter_elemhideemulation_nodomain=No
active domain specified for element hiding emulation filter
This is a user-visible error message, do we really want users to know what
"element hiding emulation" is? How about "extended element hiding filter"?

[Felix Dahlke, 2016-12-13 15:55:39]

Thanks for the review! New patch set is up.

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/content/ui/f...
File chrome/content/ui/filters-backup.js (right):

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/content/ui/f...
chrome/content/ui/filters-backup.js:328: minVersion = "2.9";
On 2016/12/13 14:44:27, Wladimir Palant wrote:
> No, as far as our users are concerned the syntax is supported starting with
> 2.7.3 - different class name changes nothing. Once the :has syntax is added we
> will need to bump the version here, any only for filters actually using :has.

Acknowledged. (Oh boy, sounds like we'll have to reintroduce the feature bit
mask after all?)

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
File chrome/locale/en-US/global.properties (right):

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
chrome/locale/en-US/global.properties:35: filter_elemhideemulation_nodomain=No
active domain specified for element hiding emulation filter
On 2016/12/13 14:44:27, Wladimir Palant wrote:
> This is a user-visible error message, do we really want users to know what
> "element hiding emulation" is? How about "extended element hiding filter"?

Done, although I went for "emulated element hiding filter" because I find
"extended element hiding filter" too ambiguous. I think "emulated element hiding
filter" is how we should call those when talking to filter list authors, because
that's really what they are: Element hiding filters that are not natively
supported and thus emulated.

[Wladimir Palant, 2016-12-13 16:33:03]

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/content/ui/f...
File chrome/content/ui/filters-backup.js (right):

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/content/ui/f...
chrome/content/ui/filters-backup.js:328: minVersion = "2.9";
On 2016/12/13 15:55:39, Felix Dahlke wrote:
> Acknowledged. (Oh boy, sounds like we'll have to reintroduce the feature bit
> mask after all?)

No, this feature is purely informative - meaning that looking for ":has(" in the
selector will be sufficient.

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
File chrome/locale/en-US/global.properties (right):

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
chrome/locale/en-US/global.properties:35: filter_elemhideemulation_nodomain=No
active domain specified for element hiding emulation filter
On 2016/12/13 15:55:39, Felix Dahlke wrote:
> Done, although I went for "emulated element hiding filter" because I find
> "extended element hiding filter" too ambiguous. 

And "emulated" is not? As a user without knowledge of technical details, how are
you going to understand what is being emulated here and why these filters are
special? I still think that we need to call those "extended" and explain
everything further in the documentation.

[Felix Dahlke, 2016-12-13 17:07:49]

New patch set is up.

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
File chrome/locale/en-US/global.properties (right):

https://codereview.adblockplus.org/29364102/diff/29364103/chrome/locale/en-US...
chrome/locale/en-US/global.properties:35: filter_elemhideemulation_nodomain=No
active domain specified for element hiding emulation filter
On 2016/12/13 16:33:03, Wladimir Palant wrote:
> On 2016/12/13 15:55:39, Felix Dahlke wrote:
> > Done, although I went for "emulated element hiding filter" because I find
> > "extended element hiding filter" too ambiguous. 
> 
> And "emulated" is not? As a user without knowledge of technical details, how
are
> you going to understand what is being emulated here and why these filters are
> special? I still think that we need to call those "extended" and explain
> everything further in the documentation.

True, probably not that great either. I guess we'll have to think some more
about how to communicate this feature to users. But until then let's just go
with "extended", we can always change this error message later if we feel the
need to.

[Felix Dahlke, 2016-12-13 17:34:47]

Another new patch set is up - I figured now that #4684 has landed I want to
include it in the dependency update.

[Wladimir Palant, 2016-12-13 19:04:45]

LGTM