Issue 4394 - Create a filter class for element hiding emulation filters

lib/filterClasses.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /**
  * @fileOverview Definition of Filter class and its subclasses.
  */
 
 let {FilterNotifier} = require("filterNotifier");
 let {extend} = require("coreUtils");
-let {elemHideEmulationFeatureMap, filterToRegExp} = require("shared");
+let {filterToRegExp} = require("common");
 
 /**
  * Abstract base class for filters
  *
  * @param {String} text   string representation of the filter
  * @constructor
  */
 function Filter(text)
 {
   this.text = text;
@@ skipping 870 matching lines @@
 
   // We don't allow ElemHide filters which have any empty domains.
   // Note: The ElemHide.prototype.domainSeparator is duplicated here, if that
   // changes this must be changed too.
   if (domain && /(^|,)~?(,|$)/.test(domain))
     return new InvalidFilter(text, "filter_invalid_domain");
 
   if (isException)
     return new ElemHideException(text, domain, selector);
 
-  let emulatedFeatures = 0;
   if (selector.indexOf("[-abp-properties") != -1)
-    emulatedFeatures |= ElemHideEmulationFilter.featureMap.PROPERTY_SELECTOR;
-  if (selector.indexOf(":has(") != -1)

[kzar, 2016/11/04 15:45:56]

The feature flags are a nice idea but I wonder what will happen if we add a new
feature in the future. Since the old versions of ElemHideBase.fromText won't
have any logic to detect a new and unsupported feature it won't assign the
feature flag for it either. So then the content script won't know that a new and
unsupported feature has been used for a filter and it could barf when attempting
to parse and apply it.

[Felix Dahlke, 2016/11/04 16:43:35]

Well, if we detect NO feature here, this would just become a regular element
hiding filter, so the browser will have to deal with the unsupported syntax.

I guess the situation you mean is that there could be a filter that has one
supported feature, but then something that is really just an invalid CSS
selector, which could confuse our parsing code in the elemHideEmulation content
script? I guess the best we can do is make sure that parsing logic is robust
against invalid syntax?

[kzar, 2016/11/07 12:36:26]

I guess my question is what's the point of having the features flags at all? On
the surface having it sounds like a good idea, but I'm not sure how it helps
anything. If we add a new emulation feature in the future the old code won't
know how to detect the feature anyway. It only does anything here as we're
getting ahead of ourselves and adding the :has detection before adding :has
support. (Which arguably is an unrelated change.)

Wouldn't it make more sense to just consider a filter an emulation filter or
not? Especially since we apparently don't intend to leave it up to the platform
which emulation filter features are used.

[Felix Dahlke, 2016/11/07 14:41:24]

Well, we have to check for emulated features before we create the filter object
- otherwise we wouldn't know whether it's an emulation filter or not. We could
of course just set a single boolean instead of setting feature flags, then we
could determine what features are being used in the content script. Right now
the main benefit of having the feature flags here is that it makes testing and
debugging easier.

Note that we're not getting ahead of ourselves here by detecting :has - #4394 is
a direct prerequisite of #3143, it doesn't make any sense in isolation actually.
It was just too big for a single issue in my mind, that's all. This way, #3143
is only going to be a change to the content script, won't touch anything else.
That makes sense, since it's going to be a somewhat complex change.

[kzar, 2016/11/07 15:59:44]

Well we don't need to set a boolean flag here either, we would simply return a
new ElemHideEmulationFilter instead of a new ElemHideFilter.
Isn't that more or less what you're planning to do anyway, parsing and handling
emulation filters in the content script? If we have a filter with nested
features we'll have to do more work to find and split them out then (possibly
recursively), so I guess I don't see the utility of figuring out the features an
emulation filter uses here too.

(The only advantage I can see would be that filters using unsupported features
could be ignored, except that they can't since the logic here doesn't know how
to set the feature flags for those anyway.)

[Felix Dahlke, 2016/11/07 16:51:24]

Yeah of course, my bad.
Feature detection are simple string checks. Note that we have to do the checking
here no matter what, the question is whether we record the results or not. To
make testing and debugging easier I'd prefer to keep the flags. But I guess
we'll have to agree to disagree on this, too, let's just wait for Wladimir.

[kzar, 2016/11/07 17:10:22]

That's not quite true, since if we're recording used features here we need to
test for all known patterns. If we just want to decide if a filter is a
emulation filter then we "only" need to search for the presence of any feature.

(By the way I wasn't trying to say that I am definitely right and that having
the feature flags for emulation filters definitely isn't useful. Just that I
realised it doesn't actually help us much to check for feature support for only
the features we know and therefore probably support anyway! But if you can't see
what it's for either perhaps we should just remove it, no point having extra
complexity that doesn't have a purpose.)

[Felix Dahlke, 2016/11/08 17:45:35]

Well, sure, we don't have to run all checks if one matches unless we go for
flags. I doubt this has a relevant impact on performance however - this is only
problematic if we have a large number of emulation filters. The complexity /
amount of code is about the same.
I can absolutely see your point. The flags are currently only needed to test the
detection logic. Having code that is only useful for tests is generally a bit of
a red flag to me (although it does sometimes make sense).

I'm planning to add proper tests for the content script with #3143. So then we
could test everything there and, if it turns out we don't need the flags for
anything else, get rid of them for good. However, for #4394 I would kind of
prefer to leave them in for the sake of having these tests.

For this I would add a comment to the flags that explains this situation, and
also mention it in #3143 as something we need to look into. Does that make
sense?

[kzar, 2016/11/15 16:54:13]

Well no I think that if the only reason to add all this complexity is for the
unit tests then we shouldn't have it. If you want to check if a filter contains
":has" or "[-abp-properties" you can easily do that in the tests. (If it's less
convenient we could have a utility function that does that in the tests too.)

(I've always been told to make things as simple as possible in code reviews,
only adding things when needed. This seems like the opposite logic, let's keep
it in since it looks nice superficially and maybe we'll need it one day.)
I still think we should remove the `if (selector.indexOf(":has(") != -1)` check
since we don't support those filters yet. I think that check should be added at
the same time :has selector support is added. So what if two extra lines in here
get added at the same time as a content script is modified? If anything that
would be clearer since the change would be titled something like "Add :has
support to elemhide emulation filters" and all the changes for that are grouped
together.

[Felix Dahlke, 2016/11/15 21:11:44]

I still think it makes sense to start detecting (yet ignoring) filters that use
:has now, as part of #4394 to avoid undefined behaviour and restrict #3143 to
rewriting the content script. Bear in mind that this is a preparatory change for
#3143 that is not meant to make any sense in isolation. We can either duplicate
the check in filterClasses and the content script or go for the flag, I prefer
the latter.

But we're moving in circles again, so let's leave this one for Wladimir.

[Wladimir Palant, 2016/11/21 11:39:14]

I actually have to agree with Dave here. You are doing preparation work for the
:has selector, sure. But you are not adding support for it yet - so it makes no
sense to detect it at this point. Right now :has should still be treated as a
normal CSS selector meaning that it will be ignored by the browser's CSS engine.

Also, I agree that the features bitmask is unnecessary. The content script will
apply its regexp anyway, so it will detect this way whether the filter is a
proper CSS property filter. And unit tests can still test recognition of
emulation filters indirectly - the filterListener unit test will see these
filters being sorted into a different container.

[Felix Dahlke, 2016/11/21 14:38:58]

Fair enough, I guess if we're not worried about backwards compatibility there's
really no reason to deal with :has at all, at this stage. Simplifies everything
quite a bit for now.

-    emulatedFeatures |= ElemHideEmulationFilter.featureMap.HAS_PSEUDO_CLASS;
-
-  if (emulatedFeatures != 0)
   {
     // Element hiding emulation filters are inefficient so we need to make sure
     // that they're only applied if they specify active domains
     if (!/,[^~][^,.]*\.[^,]/.test("," + domain))
       return new InvalidFilter(text, "filter_elemhideemulation_nodomain");
 
-    return new ElemHideEmulationFilter(text, domain, selector,

[kzar, 2016/11/04 15:45:56]

I wonder if it's really a good idea to move the logic for parsing element hiding
emulation filters to the content script? Especially since when :has support is
added the parsing logic will be even more complex. Perhaps the parsing logic
should stay here like it does for all other types of filters?

I guess I'm curious what your reasoning for moving it to the content script is?
(You mention it's cleaner that way and we'll need to do this anyway, but that's
kind of vague.) Maybe it'd help to share your WIP changes to adblockpluschrome
so I can get my head around it all a bit better?

[Felix Dahlke, 2016/11/04 16:43:35]

Well, one part of the truth is that I don't know how the parsing logic is going
to work yet. Are we going to have a separate parsing step, or are we going to
interpret the filter on the fly? I think we should leave that decision to the
content script, I don't feel it belongs here. In addition, having complex
pre-parsing logic here can slow down filter loading. I'd rather do this lazily
and slow down the content script, which is only active on sites where we need
those filters anyway.

[kzar, 2016/11/07 17:19:25]

That's a pretty good point actually, I didn't think of that.

-                                       emulatedFeatures);
+    return new ElemHideEmulationFilter(text, domain, selector);
   }
 
   return new ElemHideFilter(text, domain, selector);
 };
 
 /**
  * Class for element hiding filters
  * @param {String} text see Filter()
  * @param {String} domains  see ElemHideBase()
  * @param {String} selector see ElemHideBase()
@@ skipping 26 matching lines @@
 
 ElemHideException.prototype = extend(ElemHideBase, {
   type: "elemhideexception"
 });
 
 /**
  * Class for element hiding emulation filters
  * @param {String} text           see Filter()
  * @param {String} domains        see ElemHideBase()
  * @param {String} selector       see ElemHideBase()
- * @param {Integer} features      see ElemHideEmulationFilter.features
  * @constructor
  * @augments ElemHideBase
  */
-function ElemHideEmulationFilter(text, domains, selector, features)
+function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
-
-  this.features = features;
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
-  type: "elemhideemulation",
-
-  /**
-   * Features used in this filter, combination of values from

[kzar, 2016/11/04 15:45:56]

Perhaps describe it as bit flags instead of combination of values?

[Felix Dahlke, 2016/11/04 16:43:35]

I thought I'd keep this in line with the doc comment for the only other bitmap
we have here, which is RegExpFilter.contentType.

[kzar, 2016/11/07 17:19:25]

Well bitmap works too. (I guess if you're worried about consistency you could
improve the other comment too.)

-   * ElemHideEmulationFilter.featureMap
-   * @type Integer
-   */
-  features: 0
-});
-
-/**
- * @see elemHideEmulationFeatureMap
- */
-ElemHideEmulationFilter.featureMap = elemHideEmulationFeatureMap;
+  type: "elemhideemulation"
+});