Issue 29737558: Issue 6538, 6781 - Implement support for snippet filters (Closed)
|
Created:
March 30, 2018, 8:18 p.m. by Manish Jethani Modified:
July 11, 2018, 7:10 p.m. CC:
mapx Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : Check for null and blank string instead #Patch Set 3 : Rebase #Patch Set 4 : Make snippet filters similar to element hiding filters #
Total comments: 6
Patch Set 5 : Remove RegExpFilter.typeMap.SNIPPET #Patch Set 6 : Fix normalization and add more tests #Patch Set 7 : Rebase #Patch Set 8 : Return code directly from getScriptsForDomain #Patch Set 9 : Update tests #
Total comments: 2
Patch Set 10 : Rebase #Patch Set 11 : Fix rebase #Patch Set 12 : Add filter to known filters #Patch Set 13 : Rebase, rename to ScriptFilter, ignore element hiding exceptions #Patch Set 14 : Rebase #Patch Set 15 : Rebase #
Total comments: 2
Patch Set 16 : Rename ScriptFilter to ContentFilter #Patch Set 17 : Make ElemHideBase.selector a getter #
Total comments: 5
Patch Set 18 : Change ElemHideBase.fromText to ContentFilter.fromText #Patch Set 19 : Rebase #Patch Set 20 : Fix JSDoc comment #
Total comments: 27
Patch Set 21 : Rename script to body in ContentFilter #
Total comments: 2
Patch Set 22 : Improve comment formatting #
Total comments: 2
MessagesTotal messages: 34
(Adding Mapx to Cc at his request.)
Patch Set 2: Check for null and blank string instead
Could you please add tests?
Patch Set 4: Make snippet filters similar to element hiding filters I've updated the implementation now. There are tests as well. Also see https://codereview.adblockplus.org/29761597/ and https://codereview.adblockplus.org/29761612/ and https://codereview.adblockplus.org/29737561/ The issue description contains a specification.
https://codereview.adblockplus.org/29737558/diff/29761590/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29761590/lib/filterClasses.j... lib/filterClasses.js:872: SNIPPET: 0x8000000, I had to take this slot since I can't go any higher. This is half of the next one. https://codereview.adblockplus.org/29737558/diff/29761590/lib/filterClasses.j... lib/filterClasses.js:884: RegExpFilter.typeMap.SNIPPET | I'm actually not sure that this is needed, $snippet is not a filter option now and has nothing to do with RegExpFilter. I'll check again. https://codereview.adblockplus.org/29737558/diff/29761590/lib/filterClasses.j... lib/filterClasses.js:1002: code: null CSS selectors are also code, so let's make this generic, then we can share this with snippets. https://codereview.adblockplus.org/29737558/diff/29761590/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29761590/lib/snippets.js#new... lib/snippets.js:21: * @fileOverview Snippets implementation. This is almost exactly like lib/elemHideEmulation.js https://codereview.adblockplus.org/29737558/diff/29761590/lib/snippets.js#new... lib/snippets.js:72: !ElemHide.getException(filter, domain)) Note: We're using ElemHide exceptions here. I feel a little odd about this, since SnippetFilter doesn't extend ElemHideBase. It's probably OK though. At some point ElemHideException can be changed to CodeInjectionException. https://codereview.adblockplus.org/29737558/diff/29761590/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29761590/test/snippets.js#ne... test/snippets.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, This is also almost exactly like test/elemHideEmulation.js so far
https://codereview.adblockplus.org/29737558/diff/29768675/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29768675/lib/filterClasses.j... lib/filterClasses.js:136: return new SnippetFilter(text, match[1], match[3]); Here you shouldn't return, but rather assign it to `result`. And put the next statement in an `else` block. Otherwise the Snippet filter won't be added to Filter.knownFilters.
Patch Set 10: Rebase Patch Set 11: Fix rebase Patch Set 12: Add filter to known filters https://codereview.adblockplus.org/29737558/diff/29768675/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29768675/lib/filterClasses.j... lib/filterClasses.js:136: return new SnippetFilter(text, match[1], match[3]); On 2018/05/03 22:59:38, hub wrote: > Here you shouldn't return, but rather assign it to `result`. > > And put the next statement in an `else` block. > > Otherwise the Snippet filter won't be added to Filter.knownFilters. Done.
Patch Set 13: Rebase, rename to ScriptFilter, ignore element hiding exceptions I've updated the implementation now so ElemHideBase extends from "ScriptFilter" Two important changes: 1. ElemHideBase still has selectorDomains and selector properties so that there's no need to change any code that depends on these properties; they are proxies to the scriptDomains and script properties in the base class 2. The code no longer cares about element hiding exceptions; I'm not sure that element hiding exceptions should apply to snippet filters, we're clubbing too many things together, and anyway the initial implementation doesn't need it so I'm leaving it out (so basically there's no way to add an exception for a snippet filter now)
Patch Set 14: Rebase ElemHideBase.selectorDomains is gone, as has ScriptFilter.scriptDomains now. Still not sure about exceptions, but I think eventually ElemHideException should be renamed to ScriptException, extending ScriptFilter or a separate ScriptFilterBase if you will, and it should apply to "##", "#?#", and "#$#" filters. Also see Trac #6726.
Patch Set 15: Rebase Another rebase.
Please see this comment: https://issues.adblockplus.org/ticket/6538#comment:77 Can we start with the code reviews now? I have assigned three reviewers, and I know you are all incredibly busy, but if even one of you gives me a go-ahead I'll feel comfortable landing these patches. Thanks!
https://codereview.adblockplus.org/29737558/diff/29810559/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29810559/lib/filterClasses.j... lib/filterClasses.js:999: function ScriptFilter(text, domains, script) I'm not convinced with the renaming to "ScriptFilter". I'd prefer somnething like "ContentFilter" since we are manipulating the content and it is run by the content script. But this is just a personal preference.
Patch Set 16: Rename ScriptFilter to ContentFilter https://codereview.adblockplus.org/29737558/diff/29810559/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29810559/lib/filterClasses.j... lib/filterClasses.js:999: function ScriptFilter(text, domains, script) On 2018/06/22 20:45:39, hub wrote: > I'm not convinced with the renaming to "ScriptFilter". I'd prefer somnething > like "ContentFilter" since we are manipulating the content and it is run by the > content script. > > But this is just a personal preference. That sounds reasonable to me. Done.
Patch Set 17: Make ElemHideBase.selector a getter https://codereview.adblockplus.org/29737558/diff/29816558/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29816558/lib/filterClasses.j... lib/filterClasses.js:1040: get selector() This can be a getter actually, so we don't create an additional property on every ElemHideBase object. I will make a separate change to lib/elemHide.js so it is accessed as few times as necessary.
https://codereview.adblockplus.org/29737558/diff/29816558/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29816558/lib/filterClasses.j... lib/filterClasses.js:138: filter = ElemHideBase.fromText(text, match[1], match[2], match[3]); I'm wonder if we shouldn't have this in ContentFilter.fromText() since we match `contentRegExp` But then we didn't have the syntax conversion in ElemHideBase.fromText() either, which IMHO should be there too. https://codereview.adblockplus.org/29737558/diff/29816558/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29816558/lib/snippets.js#new... lib/snippets.js:69: let filter = Filter.fromText(text); My concern here is that we parse the filters each time with call this method, which is about every document. Is there a reason why `filters` isn't just a Set of (Snippet)Filter objects? This would avoid the need `Filter.fromText()`
Patch Set 18: Change ElemHideBase.fromText to ContentFilter.fromText https://codereview.adblockplus.org/29737558/diff/29816558/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29816558/lib/filterClasses.j... lib/filterClasses.js:138: filter = ElemHideBase.fromText(text, match[1], match[2], match[3]); On 2018/07/05 08:40:14, hub wrote: > I'm wonder if we shouldn't have this in ContentFilter.fromText() since we match > `contentRegExp` Yeah, this makes sense. Done. https://codereview.adblockplus.org/29737558/diff/29816558/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29816558/lib/snippets.js#new... lib/snippets.js:69: let filter = Filter.fromText(text); On 2018/07/05 08:40:14, hub wrote: > My concern here is that we parse the filters each time with call this method, > which is about every document. This is an exact copy of lib/elemHideEmulation.js with some renaming. The plan is to merge the two along with lib/elemHide.js and as part of that this be taken care of. See: https://codereview.adblockplus.org/29783618/ https://codereview.adblockplus.org/29784555/ If you think it's a good idea to do those first then maybe we should do that. I think maybe it's OK to leave this like it is for now, for the initial implementation. > Is there a reason why `filters` isn't just a Set of (Snippet)Filter objects? > This would avoid the need `Filter.fromText()` I'm not sure why it was done this way in lib/elemHideEmulation.js
Patch Set 19: Rebase
Patch Set 20: Fix JSDoc comment
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1019: * Script that should be executed This comment seems misleading, I think it should be clearer that the value also might be a CSS selector. https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1022: script: null "script" seems a misleading name, how about "body"? (We should also update some of the comments and parameter names to match.) https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or Please could you improve this part of the comment? How about this? The rule type, either: - "" for an element hiding filter - "@" for an exception filter - "?" for an element hiding emulation filter - "$" for a snippet filter I don't care too much about the formatting anyway, so long as it's a bit easier to grok. https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterListener.js File lib/filterListener.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterListener.... lib/filterListener.js:156: else if (filter instanceof ElemHideBase) Wouldn't it be more logical to check for `ContentFilter` first, instead of `ElemHideBase`? https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js#new... lib/snippets.js:64: getScriptsForDomain(domain) This seems pretty inefficient. I would have expected this to be called quite often, and its performance to matter therefore. But no doubt you've thought about this, so could you explain your approach?
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1019: * Script that should be executed On 2018/07/10 12:53:22, kzar wrote: > This comment seems misleading, I think it should be clearer that the value also > might be a CSS selector. This is kinda overridden in the subclass as a new property called "selector" (what it was called previously). The way I think of it, ContentFilter is generic and it always runs a "script" on the document; ElemHideBase is a special case where the "script" is a CSS selector. https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1022: script: null On 2018/07/10 12:53:22, kzar wrote: > "script" seems a misleading name, how about "body"? (We should also update some > of the comments and parameter names to match.) This was originally called "code", then I renamed it to "script". "script" seems OK to me, but I don't really care too much what it's called. What do you think, "code" or "body"? https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterListener.js File lib/filterListener.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterListener.... lib/filterListener.js:156: else if (filter instanceof ElemHideBase) On 2018/07/10 12:53:23, kzar wrote: > Wouldn't it be more logical to check for `ContentFilter` first, instead of > `ElemHideBase`? We're not checking for ContentFilter but SnippetFilter; it's at the same level in the class hierarchy as ElemHideBase. Also ElemHideBase would be a lot more common so we could avoid the additional check for the most common case. https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js#new... lib/snippets.js:64: getScriptsForDomain(domain) On 2018/07/10 12:53:23, kzar wrote: > This seems pretty inefficient. I would have expected this to be called quite > often, and its performance to matter therefore. But no doubt you've thought > about this, so could you explain your approach? See my comment here: https://codereview.adblockplus.org/29737558/#msg17 This is copied from lib/elemHideEmulation.js. Eventually we want to merge all three modules; until then, it shouldn't matter too much (not much worse than what we have now for element hiding emulation). In the beginning I expect not more than two or three snippet filters with lots of domains attached to them.
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1019: * Script that should be executed On 2018/07/10 18:29:20, Manish Jethani wrote: > On 2018/07/10 12:53:22, kzar wrote: > > This comment seems misleading, I think it should be clearer that the value > also > > might be a CSS selector. > > This is kinda overridden in the subclass as a new property called "selector" > (what it was called previously). The way I think of it, ContentFilter is generic > and it always runs a "script" on the document; ElemHideBase is a special case > where the "script" is a CSS selector. To me a CSS selector is not a script (nor code), which is why I think this name is misleading. Also I don't think of element hiding filters to be a special case of content filters, like you mention they're by far the most common. https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1022: script: null On 2018/07/10 18:29:20, Manish Jethani wrote: > On 2018/07/10 12:53:22, kzar wrote: > > "script" seems a misleading name, how about "body"? (We should also update > some > > of the comments and parameter names to match.) > > This was originally called "code", then I renamed it to "script". "script" seems > OK to me, but I don't really care too much what it's called. What do you think, > "code" or "body"? I prefer "body" over "script" and "code". https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterListener.js File lib/filterListener.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterListener.... lib/filterListener.js:156: else if (filter instanceof ElemHideBase) On 2018/07/10 18:29:20, Manish Jethani wrote: > On 2018/07/10 12:53:23, kzar wrote: > > Wouldn't it be more logical to check for `ContentFilter` first, instead of > > `ElemHideBase`? > > We're not checking for ContentFilter but SnippetFilter; it's at the same level > in the class hierarchy as ElemHideBase. I know. > Also ElemHideBase would be a lot more common so we could avoid the additional > check for the most common case. Fair enough. https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js#new... lib/snippets.js:64: getScriptsForDomain(domain) On 2018/07/10 18:29:21, Manish Jethani wrote: > In the beginning I expect not more than two or three snippet filters with lots of domains attached to them. Well, you're the expert. Please could you add that snippet filters will be slow if used commonly to the the known limitations of the issue description though?
https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js#new... lib/snippets.js:64: getScriptsForDomain(domain) On 2018/07/10 18:54:00, kzar wrote: > On 2018/07/10 18:29:21, Manish Jethani wrote: > > In the beginning I expect not more than two or three snippet filters with lots > of domains attached to them. > > Well, you're the expert. > > Please could you add that snippet filters will be slow if used commonly to the > the known limitations of the issue description though? The issue is blocked by #6665 so it won't be closed until #6665 is closed. i.e. only the initial implementation will be slow, not the final implementation. I expect we should be able to resolve #6665 over one or two releases. I also don't mind that we work on #6665 first but that runs counter to our goal of shipping a basic version of this feature as soon as we can.
https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js#new... lib/snippets.js:64: getScriptsForDomain(domain) On 2018/07/11 12:02:09, Manish Jethani wrote: > On 2018/07/10 18:54:00, kzar wrote: > > On 2018/07/10 18:29:21, Manish Jethani wrote: > > > In the beginning I expect not more than two or three snippet filters with > lots > > of domains attached to them. > > > > Well, you're the expert. > > > > Please could you add that snippet filters will be slow if used commonly to the > > the known limitations of the issue description though? > > The issue is blocked by #6665 so it won't be closed until #6665 is closed. i.e. > only the initial implementation will be slow, not the final implementation. I > expect we should be able to resolve #6665 over one or two releases. > > I also don't mind that we work on #6665 first but that runs counter to our goal > of shipping a basic version of this feature as soon as we can. Well if we plan to do a release which includes #6538 before #6665 is fixed then #6665 should not be marked as a blocker. #6538 should be closed, added to the milestone and then verified working before the release if it's to be included. Please could you add a note to explain that the extension might slow down if there are lots of snippet filters to the known limitations of the issue description? You can mention that it will hopefully be resolved with #6665 if you like.
https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js#new... lib/snippets.js:64: getScriptsForDomain(domain) On 2018/07/11 12:17:49, kzar wrote: > On 2018/07/11 12:02:09, Manish Jethani wrote: > > On 2018/07/10 18:54:00, kzar wrote: > > > On 2018/07/10 18:29:21, Manish Jethani wrote: > > > > In the beginning I expect not more than two or three snippet filters with > > lots > > > of domains attached to them. > > > > > > Well, you're the expert. > > > > > > Please could you add that snippet filters will be slow if used commonly to > the > > > the known limitations of the issue description though? > > > > The issue is blocked by #6665 so it won't be closed until #6665 is closed. > i.e. > > only the initial implementation will be slow, not the final implementation. I > > expect we should be able to resolve #6665 over one or two releases. > > > > I also don't mind that we work on #6665 first but that runs counter to our > goal > > of shipping a basic version of this feature as soon as we can. > > Well if we plan to do a release which includes #6538 before #6665 is fixed then > #6665 should not be marked as a blocker. #6538 should be closed, added to the > milestone and then verified working before the release if it's to be included. OK, but we set the milestone on the Platform issue (#6539), is that right? In that case I'll make the description of that issue clearer, including known limitations. Wait, I really want #6539 to remain open as well, I think I'll file a separate Platform issue for the initial implementation and let #6539 be for the overall work. > Please could you add a note to explain that the extension might slow down if > there are lots of snippet filters to the known limitations of the issue > description? You can mention that it will hopefully be resolved with #6665 if > you like. Yes, I'll update the issue description for #6538 as well.
Patch Set 21: Rename script to body in ContentFilter Patch Set 22: Improve comment formatting https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1022: script: null On 2018/07/10 18:53:59, kzar wrote: > On 2018/07/10 18:29:20, Manish Jethani wrote: > > On 2018/07/10 12:53:22, kzar wrote: > > > "script" seems a misleading name, how about "body"? (We should also update > > some > > > of the comments and parameter names to match.) > > > > This was originally called "code", then I renamed it to "script". "script" > seems > > OK to me, but I don't really care too much what it's called. What do you > think, > > "code" or "body"? > > I prefer "body" over "script" and "code". Done. https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/10 12:53:22, kzar wrote: > Please could you improve this part of the comment? How about this? > > The rule type, either: > - "" for an element hiding filter > - "@" for an exception filter > - "?" for an element hiding emulation filter > - "$" for a snippet filter > > I don't care too much about the formatting anyway, so long as it's a bit easier > to grok. Done. https://codereview.adblockplus.org/29737558/diff/29827618/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29827618/lib/filterClasses.j... lib/filterClasses.js:1169: get script() I would prefer to stick to this terminology for Snippets: snippet - some executable function (JS or WebAssembly, doesn't matter what it is) that takes parameters snippet script - code that invokes one or more snippets with zero or more arguments each snippet filter - a list of domains plus a snippet script https://codereview.adblockplus.org/29737558/diff/29827624/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29827624/lib/filterClasses.j... lib/filterClasses.js:1033: * <li>"" for an element hiding filter</li> Using just "-" doesn't work in the HTML version, but we can use <li> instead. We should really switch to Markdown in the comments.
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/11 13:04:24, Manish Jethani wrote: > On 2018/07/10 12:53:22, kzar wrote: > > Please could you improve this part of the comment? How about this? > > > > The rule type, either: > > - "" for an element hiding filter > > - "@" for an exception filter > > - "?" for an element hiding emulation filter > > - "$" for a snippet filter > > > > I don't care too much about the formatting anyway, so long as it's a bit > easier > > to grok. > > Done. By the way, in case you're wondering if it shouldn't be called "content exception" rather than "element hiding exception", I have thought about this. In the initial version these exceptions won't apply to snippet filters. When we get around to it finally, the class should also be renamed to ContentException and should be moved one level up to extend directly from ContentFilter. But I'm not fully sure about this. Maybe snippet filters should have their own exceptions like "#$@#"? Something to think about.
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/11 13:16:31, Manish Jethani wrote: > On 2018/07/11 13:04:24, Manish Jethani wrote: > > On 2018/07/10 12:53:22, kzar wrote: > > > Please could you improve this part of the comment? How about this? > > > > > > The rule type, either: > > > - "" for an element hiding filter > > > - "@" for an exception filter > > > - "?" for an element hiding emulation filter > > > - "$" for a snippet filter > > > > > > I don't care too much about the formatting anyway, so long as it's a bit > > easier > > > to grok. > > > > Done. > > By the way, in case you're wondering if it shouldn't be called "content > exception" rather than "element hiding exception", I have thought about this. In > the initial version these exceptions won't apply to snippet filters. When we get > around to it finally, the class should also be renamed to ContentException and > should be moved one level up to extend directly from ContentFilter. But I'm not > fully sure about this. Maybe snippet filters should have their own exceptions > like "#$@#"? Something to think about. Yea, this is something I asked you about on one of the other reviews. I disagree, I think #@# exceptions should apply to snippet filters too. Unless I'm mistaken "example.com#@#.some_class" isn't going to matter to a snippet filter, but it would be logical to expect "example.com#@#foo arg" to work. Sebastian/Sergz what do you guys think?
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/11 16:33:12, kzar wrote: > On 2018/07/11 13:16:31, Manish Jethani wrote: > > On 2018/07/11 13:04:24, Manish Jethani wrote: > > > On 2018/07/10 12:53:22, kzar wrote: > > > > Please could you improve this part of the comment? How about this? > > > > > > > > The rule type, either: > > > > - "" for an element hiding filter > > > > - "@" for an exception filter > > > > - "?" for an element hiding emulation filter > > > > - "$" for a snippet filter > > > > > > > > I don't care too much about the formatting anyway, so long as it's a bit > > > easier > > > > to grok. > > > > > > Done. > > > > By the way, in case you're wondering if it shouldn't be called "content > > exception" rather than "element hiding exception", I have thought about this. > In > > the initial version these exceptions won't apply to snippet filters. When we > get > > around to it finally, the class should also be renamed to ContentException and > > should be moved one level up to extend directly from ContentFilter. But I'm > not > > fully sure about this. Maybe snippet filters should have their own exceptions > > like "#$@#"? Something to think about. > > Yea, this is something I asked you about on one of the other reviews. I > disagree, I think #@# exceptions should apply to snippet filters too. Unless I'm > mistaken mailto:"example.com#@#.some_class" isn't going to matter to a snippet filter, > but it would be logical to expect "example.com#@#foo arg" to work. > > Sebastian/Sergz what do you guys think? Yeah, I too think that those exceptions could just apply to snippet filters as well. We will have to update the code and the documentation, that's all. One challenge with the exceptions was that the selector property was being escaped. This is mostly resolved now, because we have a separate code/script/body property (unescaped) in the superclass, which we should use for matching. The selector property should be used only to get the actual (escaped) CSS selector to inject.
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/11 16:33:12, kzar wrote: > On 2018/07/11 13:16:31, Manish Jethani wrote: > > On 2018/07/11 13:04:24, Manish Jethani wrote: > > > On 2018/07/10 12:53:22, kzar wrote: > > > > Please could you improve this part of the comment? How about this? > > > > > > > > The rule type, either: > > > > - "" for an element hiding filter > > > > - "@" for an exception filter > > > > - "?" for an element hiding emulation filter > > > > - "$" for a snippet filter > > > > > > > > I don't care too much about the formatting anyway, so long as it's a bit > > > easier > > > > to grok. > > > > > > Done. > > > > By the way, in case you're wondering if it shouldn't be called "content > > exception" rather than "element hiding exception", I have thought about this. > In > > the initial version these exceptions won't apply to snippet filters. When we > get > > around to it finally, the class should also be renamed to ContentException and > > should be moved one level up to extend directly from ContentFilter. But I'm > not > > fully sure about this. Maybe snippet filters should have their own exceptions > > like "#$@#"? Something to think about. > > Yea, this is something I asked you about on one of the other reviews. I > disagree, I think #@# exceptions should apply to snippet filters too. Unless I'm > mistaken mailto:"example.com#@#.some_class" isn't going to matter to a snippet filter, > but it would be logical to expect "example.com#@#foo arg" to work. > > Sebastian/Sergz what do you guys think? So far for misusing element-hiding-like syntax for snippets. Just another example why this analogy lacks (and causes confusion). But well, assuming it's now established that we go with this obscure syntax, I agree with Manish here that #@# exceptions rules probably shouldn't apply to snippet filters. The problem is "x y" could either be a CSS selector targeting an element with the name "y" inside another element with the name "x", or it could mean snippet "x" with argument "y".
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/11 16:47:30, Sebastian Noack wrote: > On 2018/07/11 16:33:12, kzar wrote: > > On 2018/07/11 13:16:31, Manish Jethani wrote: > > > On 2018/07/11 13:04:24, Manish Jethani wrote: > > > > On 2018/07/10 12:53:22, kzar wrote: > > > > > Please could you improve this part of the comment? How about this? > > > > > > > > > > The rule type, either: > > > > > - "" for an element hiding filter > > > > > - "@" for an exception filter > > > > > - "?" for an element hiding emulation filter > > > > > - "$" for a snippet filter > > > > > > > > > > I don't care too much about the formatting anyway, so long as it's a bit > > > > easier > > > > > to grok. > > > > > > > > Done. > > > > > > By the way, in case you're wondering if it shouldn't be called "content > > > exception" rather than "element hiding exception", I have thought about > this. > > In > > > the initial version these exceptions won't apply to snippet filters. When we > > get > > > around to it finally, the class should also be renamed to ContentException > and > > > should be moved one level up to extend directly from ContentFilter. But I'm > > not > > > fully sure about this. Maybe snippet filters should have their own > exceptions > > > like "#$@#"? Something to think about. > > > > Yea, this is something I asked you about on one of the other reviews. I > > disagree, I think #@# exceptions should apply to snippet filters too. Unless > I'm > > mistaken mailto:"example.com#@#.some_class" isn't going to matter to a snippet > filter, > > but it would be logical to expect "example.com#@#foo arg" to work. > > > > Sebastian/Sergz what do you guys think? > > So far for misusing element-hiding-like syntax for snippets. Just another > example why this analogy lacks (and causes confusion). But well, assuming it's > now established that we go with this obscure syntax, I agree with Manish here > that #@# exceptions rules probably shouldn't apply to snippet filters. I'm not sure I agree with that, but since we are undecided, and we don't need exceptions to work in the initial version, we can postpone the decision. > The problem is "x y" could either be a CSS selector targeting an element with > the name "y" inside another element with the name "x", or it could mean snippet > "x" with argument "y". I highly doubt that this will be a problem in practice, but we could also just have a naming convention for snippet functions so they never clash with CSS selectors. For example simply following the practice of surrounding all snippet function names with single quotes, in the snippet filter itself (which is supported by the syntax), would be enough.
https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/11 16:55:05, Manish Jethani wrote: > On 2018/07/11 16:47:30, Sebastian Noack wrote: > > On 2018/07/11 16:33:12, kzar wrote: > > > On 2018/07/11 13:16:31, Manish Jethani wrote: > > > > On 2018/07/11 13:04:24, Manish Jethani wrote: > > > > > On 2018/07/10 12:53:22, kzar wrote: > > > > > > Please could you improve this part of the comment? How about this? > > > > > > > > > > > > The rule type, either: > > > > > > - "" for an element hiding filter > > > > > > - "@" for an exception filter > > > > > > - "?" for an element hiding emulation filter > > > > > > - "$" for a snippet filter > > > > > > > > > > > > I don't care too much about the formatting anyway, so long as it's a > bit > > > > > easier > > > > > > to grok. > > > > > > > > > > Done. > > > > > > > > By the way, in case you're wondering if it shouldn't be called "content > > > > exception" rather than "element hiding exception", I have thought about > > this. > > > In > > > > the initial version these exceptions won't apply to snippet filters. When > we > > > get > > > > around to it finally, the class should also be renamed to ContentException > > and > > > > should be moved one level up to extend directly from ContentFilter. But > I'm > > > not > > > > fully sure about this. Maybe snippet filters should have their own > > exceptions > > > > like "#$@#"? Something to think about. > > > > > > Yea, this is something I asked you about on one of the other reviews. I > > > disagree, I think #@# exceptions should apply to snippet filters too. Unless > > I'm > > > mistaken mailto:"example.com#@#.some_class" isn't going to matter to a > snippet > > filter, > > > but it would be logical to expect "example.com#@#foo arg" to work. > > > > > > Sebastian/Sergz what do you guys think? > > > > So far for misusing element-hiding-like syntax for snippets. Just another > > example why this analogy lacks (and causes confusion). But well, assuming it's > > now established that we go with this obscure syntax, I agree with Manish here > > that #@# exceptions rules probably shouldn't apply to snippet filters. > > I'm not sure I agree with that, but since we are undecided, and we don't need > exceptions to work in the initial version, we can postpone the decision. To give some background on this, the initial version is an "alpha" or even "pre-alpha" if you will. It will only be used by our own filter list authors (we will discard snippet filters in any other filter lists [1]). This means the syntax can be changed, everything can be changed easily. We should really stop worrying about this right now. When we have developed the feature fully and we know that it is giving results (which we can only know if we try it out in production), then we can flip the switch and make it available to all filter list authors. Until then, we have time to figure out the details. [1]: https://codereview.adblockplus.org/29824589/
LGTM https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or On 2018/07/11 17:12:10, Manish Jethani wrote: > ...We should really stop worrying about this right now... Well I was already in the process of replying... I can only do a review so fast. https://codereview.adblockplus.org/29737558/diff/29826561/lib/filterClasses.j... lib/filterClasses.js:1032: * rule type, either empty or @ (exception) or ? (emulation rule) or OK, seems the vote is #@# exception filters shouldn't apply to snippet filters for now. Please just add a note about this in the issue to explain the situation, and ideally link to an issue about adding the #@$# or similiar syntax. https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737558/diff/29826561/lib/snippets.js#new... lib/snippets.js:64: getScriptsForDomain(domain) On 2018/07/11 12:45:18, Manish Jethani wrote: > On 2018/07/11 12:17:49, kzar wrote: > > On 2018/07/11 12:02:09, Manish Jethani wrote: > > > On 2018/07/10 18:54:00, kzar wrote: > > > > On 2018/07/10 18:29:21, Manish Jethani wrote: > > > > > In the beginning I expect not more than two or three snippet filters > with > > > lots > > > > of domains attached to them. > > > > > > > > Well, you're the expert. > > > > > > > > Please could you add that snippet filters will be slow if used commonly to > > the > > > > the known limitations of the issue description though? > > > > > > The issue is blocked by #6665 so it won't be closed until #6665 is closed. > > i.e. > > > only the initial implementation will be slow, not the final implementation. > I > > > expect we should be able to resolve #6665 over one or two releases. > > > > > > I also don't mind that we work on #6665 first but that runs counter to our > > goal > > > of shipping a basic version of this feature as soon as we can. > > > > Well if we plan to do a release which includes #6538 before #6665 is fixed > then > > #6665 should not be marked as a blocker. #6538 should be closed, added to the > > milestone and then verified working before the release if it's to be included. > > OK, but we set the milestone on the Platform issue (#6539), is that right? That's right. > In that case I'll make the description of that issue clearer, > including known limitations. > ... > Yes, I'll update the issue description for #6538 as well. Thanks, that will help the testers and also properly set expectations. > Wait, I really want #6539 to remain open as well, I think I'll file a separate > Platform issue for the initial implementation and let #6539 be for the overall > work. OK, either way sounds OK to me. https://codereview.adblockplus.org/29737558/diff/29827618/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29827618/lib/filterClasses.j... lib/filterClasses.js:1169: get script() On 2018/07/11 13:04:25, Manish Jethani wrote: > I would prefer to stick to this terminology for Snippets: > > snippet - some executable function (JS or WebAssembly, doesn't matter what it > is) that takes parameters > > snippet script - code that invokes one or more snippets with zero or more > arguments each > > snippet filter - a list of domains plus a snippet script Yes, I agree, "script" makes sense for snippet filters. https://codereview.adblockplus.org/29737558/diff/29827624/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29737558/diff/29827624/lib/filterClasses.j... lib/filterClasses.js:1033: * <li>"" for an element hiding filter</li> On 2018/07/11 13:04:26, Manish Jethani wrote: > Using just "-" doesn't work in the HTML version, but we can use <li> instead. > > We should really switch to Markdown in the comments. Acknowledged.
On 2018/07/11 17:17:54, kzar wrote: > LGTM Woohoo! Thanks :) 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
