Issue 6193 - Allow element hiding emulation exceptions

Issue 6193 - Allow element hiding emulation exceptions

[kzar, 2018-04-05 15:00:27]

Sorry for the huge delay, I'm trying to clear out my backlog of reviews.

Is this one still relevant? I've given some feedback and copied Manish in as
well in case it is.

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
File lib/filterValidation.js (right):

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:116: // Only ElemHideBase has selectors
Perhaps split these up a bit? Something like this (untested)?

if (!(filter instanceof ElemHideBase))
  return true;

// We don't yet check to see if element hiding emulation filters
// have valid selectors yet. See issue xxx.
if (filter instanceof ElemHideEmulationFilter)
  return true;

// Since the #@# syntax is used for exception to both element hiding
// and element hiding emulation filters we now need to make sure only
// the former are checked.
if ((filter instanceof ElemHideException) && filter.selector.includes(":-abp-"))
  return true;

return isValidCSSSelector(filter.selector);

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:119: || (filter instanceof ElemHideEmulationFilter))
Nit: Parenthesis around the second clause seem redundant.
Nit: We would normally put the `||` at the end of the previous line. (Did this
pass linting?)
Nit: Since the clause spans multiple lines please use braces for the body. (Same
below.)

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:120: return true;
Nit: I think a newline between these cases would look cleaner.

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:124: (filter.selector.indexOf(":-abp-") != -1))
Is this check good enough now that we support :has?

[hub, 2018-04-05 17:04:42]

This time I also ran the linter.

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
File lib/filterValidation.js (right):

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:116: // Only ElemHideBase has selectors
On 2018/04/05 15:00:27, kzar wrote:
> Perhaps split these up a bit? Something like this (untested)?
> 
> if (!(filter instanceof ElemHideBase))
>   return true;
> 
> // We don't yet check to see if element hiding emulation filters
> // have valid selectors yet. See issue xxx.
> if (filter instanceof ElemHideEmulationFilter)
>   return true;
> 
> // Since the #@# syntax is used for exception to both element hiding
> // and element hiding emulation filters we now need to make sure only
> // the former are checked.
> if ((filter instanceof ElemHideException) &&
filter.selector.includes(":-abp-"))
>   return true;
> 
> return isValidCSSSelector(filter.selector);

Done.

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:119: || (filter instanceof ElemHideEmulationFilter))
On 2018/04/05 15:00:27, kzar wrote:
> Nit: Parenthesis around the second clause seem redundant.
> Nit: We would normally put the `||` at the end of the previous line. (Did this
> pass linting?)
> Nit: Since the clause spans multiple lines please use braces for the body.
(Same
> below.)

Done.

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:120: return true;
On 2018/04/05 15:00:27, kzar wrote:
> Nit: I think a newline between these cases would look cleaner.

Done.

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:124: (filter.selector.indexOf(":-abp-") != -1))
On 2018/04/05 15:00:27, kzar wrote:
> Is this check good enough now that we support :has?

This idea is that like above if we have one of the ":-abp-" pseudo selector we
don't check if this is a valid CSS selector, and assume it is fine. I don't even
know how I could let the comment above. I'll rephrase it properly.

In the future we should have a proper syntax checking all the way, but right
now, this is just to prevent the filter from being rejected when the user add
it.

[kzar, 2018-04-06 10:05:45]

LGTM, want to take a look as well Sebastian?

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
File lib/filterValidation.js (right):

https://codereview.adblockplus.org/29636648/diff/29636649/lib/filterValidatio...
lib/filterValidation.js:124: (filter.selector.indexOf(":-abp-") != -1))
On 2018/04/05 17:04:41, hub wrote:
> On 2018/04/05 15:00:27, kzar wrote:
> > Is this check good enough now that we support :has?
> 
> This idea is that like above if we have one of the ":-abp-" pseudo selector we
> don't check if this is a valid CSS selector, and assume it is fine. I don't
even
> know how I could let the comment above. I'll rephrase it properly.
> 
> In the future we should have a proper syntax checking all the way, but right
> now, this is just to prevent the filter from being rejected when the user add
> it.

Acknowledged.

[Sebastian Noack, 2018-04-20 16:21:16]

LGTM