Issue 4593 - Support regular expressions for CSS property matching

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
@@ skipping 1025 matching lines @@
    */
   get regexpString()
   {
     // Despite this property being cached, the getter is called
     // several times on Safari, due to WebKit bug 132872
     let prop = Object.getOwnPropertyDescriptor(this, "regexpString");
     if (prop)
       return prop.value;
 
     let regexp;
-    if (this.regexpSource.charAt(0) == "/" &&

[kzar, 2016/11/03 12:18:16]

Nit: How about using startsWith and endsWith instead?
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...

[Felix Dahlke, 2016/11/03 13:47:51]

Wouldn't work on Safari <9, would it? Maybe this is a good time to start having
Safari support in a branch, because if it wasn't for Safari, we could use this
now that we dropped support for Chrome <41.

[kzar, 2016/11/03 16:03:19]

Right, we wouldn't be able to update the adblockpluscore dependency until
https://issues.adblockplus.org/ticket/4597 is done if we use
`{starts,ends}With`.

Thing is we don't want to anyway though since we don't want any more development
builds of Safari to be made from the master bookmark. (We're aiming for the
current release 1.12.4 as being the last release that includes Safari from
master.)

[Felix Dahlke, 2016/11/03 19:08:02]

Alright, seems safe to break Safari <9 then - done.

-        this.regexpSource.slice(-1) == "/")
+    if (this.regexpSource.length >= 2 && this.regexpSource[0] == "/" &&
+        this.regexpSource[this.regexpSource.length - 1] == "/")

[Wladimir Palant, 2016/11/08 17:41:34]

Well, accessing this.regexpSource four times is obviously not perfect either but
whatever - I guess that by the time this code path becomes relevant
performance-wise we'll have it replaced by C++ anyway.

       regexp = this.regexpSource.slice(1, -1);

[kzar, 2016/11/03 12:18:16]

What about flags, for example for case-insensitive matching? `/whatever/i`
wouldn't match your logic here.

[Felix Dahlke, 2016/11/03 13:47:51]

Hm, good point. How I see it, `i` is actually the only useful flag, and maybe we
should just always do case insensitive matching for CSS properties? Note that
URL filters (which use the same expression language as non-regexp property
selectors) are also case insensitive unless you specify the `match-case` option.
Given how CSS properties and values are case insensitive, I don't think we will
need case sensitivity here, hm?

[kzar, 2016/11/03 16:03:19]

Yea, I think you're right there. I thought perhaps multiline support might be
useful but I checked and the styles are all on one line at the point they're
tested.
I suppose that would slightly change the behaviour of non-regexp CSS property
filters, since before they used case sensitive matching. But if what you say is
right that CSS property names and values are case-insensitive (I had trouble
verifying that either way) then I guess using insensitive matching would be
better if anything. So if you're sure about that then it sounds good to me.

[Felix Dahlke, 2016/11/03 19:08:02]

I dug a bit deeper and while I was apparently right that property names and
values are in general case insensitive (see
https://www.w3.org/TR/CSS2/syndata.html#characters), some property values (e.g.
url and content) are actually case sensitive. I can't quite come up with a use
case for case sensitive matching in those cases, but I find the issue a bit too
complex to tackle it as part of #4593, so I created a followup:
https://issues.adblockplus.org/ticket/4600

I suggest we stick to case sensitive matching without support for additional
flags for #4593. 

[kzar, 2016/11/04 10:06:56]

I would prefer to switch to case insensitive matching now if we're planning to
do it anyway, we warned that CSS property filters might change in the future but
no point making it harder for filter authors than necessary.

Unless I'm mistaken `pattern.regexp` will always be a string initially for both
adblockplus and adblockpluschrome since both send a "filters.get" message to
adblockplusui/messageResponder.js to obtain the filters. The relevant
messageResponder code there looks like this:

filters = CSSRules.getRulesForDomain(sender.frame.url.hostname);
filters = filters.map(function(filter)
{
  return {
    prefix: filter.selectorPrefix,
    suffix: filter.selectorSuffix,
    regexp: filter.regexpString,
    text: filter.text
  };
});
callback(filters);

I think you can therefore just change the line in
adblockpluscore/chrome/content/cssProperties.js to be something like this:

regexp = pattern.regexp = new RegExp(regexp, "i");

[Felix Dahlke, 2016/11/04 13:43:56]

I know it'd be a simple change and I agree we shouldn't wait with this. At the
same time, it's really unrelated to #4593, which is why I created #4600. There
might be some more discussion, too - note that Wladimir hasn't chipped in yet.

But yeah, to speed things up, I'll upload a review for #4600 as soon as #4593
lands.

[kzar, 2016/11/04 14:29:50]

It's not really an unrelated change IMO, since like you say part of the reason
for changing to case-insensitive matching is for consistency with how our other
regular expression filters match.

[Felix Dahlke, 2016/11/04 16:11:44]

Well, it's mainly for consistency with how our other filter expressions match
(although regexp filters also match case insensitively unless you specify
match-case I presume). This is 100% unrelated to the new regular expression
matching for selectors.

I could of course change issue #4593 to cover this as well, into something like
"Support regular expressions for CSS property matching and switch to case
insensitive matching" but that title alone sounds like it should be two issues,
which is why I created #4600 instead.

[kzar, 2016/11/04 16:18:27]

Yea they do, I checked that.
Well since the existing regular expression matching is case insensitive, it
makes sense to make this new regular expression matching case insensitive too. I
don't understand how that's unrelated.

[Felix Dahlke, 2016/11/04 16:51:16]

URL matching is _always_ case insensitive, whether it's a regular expression or
not. So it's completely unrelated to whether we support regular expressions or
not, those are two separate issues. They are just two things we want to do, two
separate things.

     else
       regexp = Filter.toRegExp(this.regexpSource);
     Object.defineProperty(this, "regexpString", {value: regexp});
     return regexp;
   }
 });