Issue 5730585574113280: Issue 2390 - Created filter class for CSS property filters (Closed)
|
Created:
May 4, 2015, 6:05 p.m. by Thomas Greiner Modified:
June 10, 2015, 5:02 p.m. Visibility:
Public. |
DescriptionIssue 2390 - Created filter class for CSS property filters
Patch Set 1 #
Total comments: 19
Patch Set 2 : #
Total comments: 8
Patch Set 3 : Addressed missing nits #
Total comments: 2
Patch Set 4 : Rebased changes #Patch Set 5 : Modified constructor signature #
MessagesTotal messages: 11
http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... File lib/filterClasses.js (left): http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:890: else Nit: Redundant else statement. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:908: else Nit: Redundant else statement. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:93: Filter.csspropertyRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/; So we don't allow quotes in the value here. Should be fine for now, as I can only think of a few esoteric CSS properties that use quotes. But please mention it in the comment above. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:922: if (/,[^~]/.test("," + domain)) I wonder whether we should also look for at least one dot. If we want to require CSS poeperty filters to be limitted to a domain, allowing top level domains would be kinda silly IMO. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:924: else Nit: Redundant else statement. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:927: else Nit: Redundant else statement. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:1016: get regexp() Hmm, this is almost the same code we already have in RegExpFilter.regexp. How about moving the whole logic to ActiveFiler._getRegExp()? However, on second thought, creating a RegExp object here, seems to be even unneeded, as we have to match the regexp in the content script. So I think we actually just need a regexp string here.
http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... File lib/filterClasses.js (left): http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:890: else On 2015/05/05 15:47:02, Sebastian Noack wrote: > Nit: Redundant else statement. Done. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:908: else On 2015/05/05 15:47:02, Sebastian Noack wrote: > Nit: Redundant else statement. Done. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:93: Filter.csspropertyRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/; On 2015/05/05 15:47:02, Sebastian Noack wrote: > So we don't allow quotes in the value here. Should be fine for now, as I can > only think of a few esoteric CSS properties that use quotes. But please mention > it in the comment above. Done, I added a comment as suggested. We could change this as soon as we need it without breaking backwards compatibility because for now it would make the regex unnecessarily complicated. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:922: if (/,[^~]/.test("," + domain)) On 2015/05/05 15:47:02, Sebastian Noack wrote: > I wonder whether we should also look for at least one dot. If we want to require > CSS poeperty filters to be limitted to a domain, allowing top level domains > would be kinda silly IMO. Good point but note that this still doesn't cover common two-part TLDs such as "co.uk"? http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:924: else On 2015/05/05 15:47:02, Sebastian Noack wrote: > Nit: Redundant else statement. Done. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:927: else On 2015/05/05 15:47:02, Sebastian Noack wrote: > Nit: Redundant else statement. Done. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:1016: get regexp() On 2015/05/05 15:47:02, Sebastian Noack wrote: > Hmm, this is almost the same code we already have in RegExpFilter.regexp. How > about moving the whole logic to ActiveFiler._getRegExp()? > > However, on second thought, creating a RegExp object here, seems to be even > unneeded, as we have to match the regexp in the content script. So I think we > actually just need a regexp string here. You're right, the issue description doesn't explicitly say that `regexp()` should be of type `RegExp`. It might be a bit unexpected though, that it returns a string so I renamed it. However, there might be reasons for converting it right away so I'd like to hear Wladimir's opinion on that as well since he created the issue.
http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:93: Filter.csspropertyRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/; On 2015/05/06 12:08:32, Thomas Greiner wrote: > On 2015/05/05 15:47:02, Sebastian Noack wrote: > > So we don't allow quotes in the value here. Should be fine for now, as I can > > only think of a few esoteric CSS properties that use quotes. But please > mention > > it in the comment above. > > Done, I added a comment as suggested. We could change this as soon as we need it > without breaking backwards compatibility because for now it would make the regex > unnecessarily complicated. I think this wouldn't event possible with a regex, but would require a state machine. :( But as I said, this limitation shouldn't be a big deal for now. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:922: if (/,[^~]/.test("," + domain)) On 2015/05/06 12:08:32, Thomas Greiner wrote: > On 2015/05/05 15:47:02, Sebastian Noack wrote: > > I wonder whether we should also look for at least one dot. If we want to > require > > CSS poeperty filters to be limitted to a domain, allowing top level domains > > would be kinda silly IMO. > > Good point but note that this still doesn't cover common two-part TLDs such as > "co.uk"? Hmm, if we want to do it properly, I suppose we have to use basedomain functionality. It's built into Gecko, and we already have an own implementation of it in the platform code for isThirdParty(). But it's probably not necessary here. http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:1016: get regexp() On 2015/05/06 12:08:32, Thomas Greiner wrote: > On 2015/05/05 15:47:02, Sebastian Noack wrote: > > Hmm, this is almost the same code we already have in RegExpFilter.regexp. How > > about moving the whole logic to ActiveFiler._getRegExp()? > > > > However, on second thought, creating a RegExp object here, seems to be even > > unneeded, as we have to match the regexp in the content script. So I think we > > actually just need a regexp string here. > > You're right, the issue description doesn't explicitly say that `regexp()` > should be of type `RegExp`. It might be a bit unexpected though, that it returns > a string so I renamed it. > > However, there might be reasons for converting it right away so I'd like to hear > Wladimir's opinion on that as well since he created the issue. Well, objects get serialized when passed to content scripts. So dependent on the browser it either won't be able to serialize complex objects like regular expressions, or will just convert it back into a string and reconstruct it during deserialization. http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... File lib/filterClasses.js (left): http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:890: else Nit: redundant else statement. http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:908: else Nit: redundant else statement. http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:927: if (!/,[^~][^,.]*\.[^,]/.test("," + domain)) This regex will allow "foo.." (note that this is equvalent to just "foo") and "..foo".
http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:93: Filter.csspropertyRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/; On 2015/05/06 13:07:48, Sebastian Noack wrote: > On 2015/05/06 12:08:32, Thomas Greiner wrote: > > On 2015/05/05 15:47:02, Sebastian Noack wrote: > > > So we don't allow quotes in the value here. Should be fine for now, as I > can > > > only think of a few esoteric CSS properties that use quotes. But please > > mention > > > it in the comment above. > > > > Done, I added a comment as suggested. We could change this as soon as we need > it > > without breaking backwards compatibility because for now it would make the > regex > > unnecessarily complicated. > > I think this wouldn't event possible with a regex, but would require a state > machine. :( But as I said, this limitation shouldn't be a big deal for now. Yes, there's currently no negative lookbehind in JavaScript which would allow us to recognize `\"` as an escaped quotation mark. Just FYI: Using /\[\-abp\-properties=(["'])((?:(?!\1).)+)\1\]/ we could at least allow the other character that's not used for limiting the string. The only strings that aren't allowed in this scenario are strings which contain both quotation characters (e.g. `[-abp-properties="content: \"Don't do this\";"]`). The issue with it is performance, however, since it requires a negative lookahead (see http://jsperf.com/allowing-quotation-marks-in-strings). http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:927: if (!/,[^~][^,.]*\.[^,]/.test("," + domain)) On 2015/05/06 13:07:48, Sebastian Noack wrote: > This regex will allow "foo.." (note that this is equvalent to just "foo") and > "..foo". While "foo.." might be equivalent to "foo" when used in the browser, it's treated separately when matching the filter against a domain. That means that the filter `adblockplus.org.##body` is only applied on "adblockplus.org." but not on "adblockplus.org" or "adblockplus.org.." Therefore I'd consider this regex to be sufficient.
http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib/... lib/filterClasses.js:93: Filter.csspropertyRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/; On 2015/05/06 15:33:07, Thomas Greiner wrote: > On 2015/05/06 13:07:48, Sebastian Noack wrote: > > On 2015/05/06 12:08:32, Thomas Greiner wrote: > > > On 2015/05/05 15:47:02, Sebastian Noack wrote: > > > > So we don't allow quotes in the value here. Should be fine for now, as I > > can > > > > only think of a few esoteric CSS properties that use quotes. But please > > > mention > > > > it in the comment above. > > > > > > Done, I added a comment as suggested. We could change this as soon as we > need > > it > > > without breaking backwards compatibility because for now it would make the > > regex > > > unnecessarily complicated. > > > > I think this wouldn't event possible with a regex, but would require a state > > machine. :( But as I said, this limitation shouldn't be a big deal for now. > > Yes, there's currently no negative lookbehind in JavaScript which would allow us > to recognize `\"` as an escaped quotation mark. > > Just FYI: Using /\[\-abp\-properties=(["'])((?:(?!\1).)+)\1\]/ we could at least > allow the other character that's not used for limiting the string. The only > strings that aren't allowed in this scenario are strings which contain both > quotation characters (e.g. `[-abp-properties="content: \"Don't do this\";"]`). > The issue with it is performance, however, since it requires a negative > lookahead (see http://jsperf.com/allowing-quotation-marks-in-strings). Yeah, let's stick to not allowing any quotes for now then, though it only seems to make a notable difference on modern Firefox versions. http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... File lib/filterClasses.js (left): http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:890: else On 2015/05/06 13:07:48, Sebastian Noack wrote: > Nit: redundant else statement. It seems, you ignored this comment. http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:908: else On 2015/05/06 13:07:48, Sebastian Noack wrote: > Nit: redundant else statement. It seems, you ignored this comment.
http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... File lib/filterClasses.js (left): http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:890: else On 2015/05/06 15:42:29, Sebastian Noack wrote: > On 2015/05/06 13:07:48, Sebastian Noack wrote: > > Nit: redundant else statement. > > It seems, you ignored this comment. Done. Sorry, that was unintentional. http://codereview.adblockplus.org/5730585574113280/diff/5757334940811264/lib/... lib/filterClasses.js:908: else On 2015/05/06 15:42:29, Sebastian Noack wrote: > On 2015/05/06 13:07:48, Sebastian Noack wrote: > > Nit: redundant else statement. > > It seems, you ignored this comment. Done.
LGTM
http://codereview.adblockplus.org/5730585574113280/diff/5693417237512192/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5693417237512192/lib/... lib/filterClasses.js:923: if (Filter.csspropertyRegExp.test(selector)) How about actually storing the result of this selector match? let match = Filter.csspropertyRegExp.exec(selector); if (match) ... You can then pass in the match[2], selector.substr(0, match.index) and selector.substr(match.index + match[0].length) as parameters to the CSSPropertyFilter constructor - no need to match again and no need to call String.split().
http://codereview.adblockplus.org/5730585574113280/diff/5693417237512192/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5730585574113280/diff/5693417237512192/lib/... lib/filterClasses.js:923: if (Filter.csspropertyRegExp.test(selector)) On 2015/06/05 22:08:59, Wladimir Palant wrote: > How about actually storing the result of this selector match? > > let match = Filter.csspropertyRegExp.exec(selector); > if (match) > ... > > You can then pass in the match[2], selector.substr(0, match.index) and > selector.substr(match.index + match[0].length) as parameters to the > CSSPropertyFilter constructor - no need to match again and no need to call > String.split(). Done. I don't really mind either approach. Note that my approach focused on avoiding parameter redundancy in the constructor signature since performance should hopefully not be an issue here.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
