Issue 29746563: Noissue - Trim spaces around element hiding emulation selectors (Closed)
|
Created:
April 8, 2018, 9:35 a.m. by Manish Jethani Modified:
April 19, 2018, 9:34 a.m. CC:
sergei Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add tests #
Total comments: 4
Patch Set 3 : Update elemhideRegExp #Patch Set 4 : Revert Patch Set 3 #Patch Set 5 : Add tests for incorrect syntax #MessagesTotal messages: 16
Patch Set 1 https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.j... lib/filterClasses.js:191: let [, domain, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text); It seems we forgot to include the "#?#" case here. If you run Filter.normalize on "foo.com, bar.com #?# #header, .footer" it gives "foo.com,bar.com#?# #header, .footer" rather than "foo.com,bar.com#?##header, .footer".
test/filterClasses.js should be updated to test this.
Well spotted Manish. > test/filterClasses.js should be updated to test this. Agreed. https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.j... lib/filterClasses.js:191: let [, domain, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text); Shouldn't the question mark be escaped?
https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.j... lib/filterClasses.js:191: let [, domain, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text); On 2018/04/09 10:38:29, kzar wrote: > Shouldn't the question mark be escaped? In between the [], it doesn't need to. It is taken literally.
https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.j... lib/filterClasses.js:191: let [, domain, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text); On 2018/04/09 15:02:07, hub wrote: > On 2018/04/09 10:38:29, kzar wrote: > > Shouldn't the question mark be escaped? > > In between the [], it doesn't need to. It is taken literally. Acknowledged.
Patch Set 2: Add tests On 2018/04/09 04:03:10, hub wrote: > test/filterClasses.js should be updated to test this. Done.
LGTM
https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.... test/filterClasses.js:435: test.equal(Filter.normalize(" domain.c om#?# # sele ctor "), What about a test for something like "domain.com# ?#selector"?
Patch Set 3: Update elemhideRegExp https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.... test/filterClasses.js:435: test.equal(Filter.normalize(" domain.c om#?# # sele ctor "), On 2018/04/11 11:29:49, kzar wrote: > What about a test for something like "domain.com# ?#selector"? Interesting, the test would pass for "# ?#" but not for "# ##" and "# @##", because Filter.elemhideRegExp does not include a "?". I've updated it now. Specifically "domain.com# ?## selector " now normalizes to "domain.com#?##selector" (note: no spaces) because elemhideRegExp no longer matches. This is consistent with "domain.com# ## selector " and "domain.com# @## selector ". I'm not sure if it is right though.
Copying in Sergei since we're now talking about filter syntax. https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.... test/filterClasses.js:435: test.equal(Filter.normalize(" domain.c om#?# # sele ctor "), On 2018/04/11 11:50:19, Manish Jethani wrote: > Specifically "domain.com# ?## selector " now normalizes to > "domain.com#?##selector" (note: no spaces) because elemhideRegExp no longer > matches. This is consistent with "domain.com# ## selector " and "domain.com# > @## selector ". I'm not sure if it is right though. Well it's good that we're being consistent now, but I agree that maybe we're being consistently wrong! Really if we're to be as thorough with the normalisation of whitespace with #?# etc filters as we were with $csp filters we should handle this case too... but I wonder at what cost that will come, or if there's any benefit to it.
Patch Set 4 https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.... test/filterClasses.js:435: test.equal(Filter.normalize(" domain.c om#?# # sele ctor "), On 2018/04/11 12:04:09, kzar wrote: > On 2018/04/11 11:50:19, Manish Jethani wrote: > > Specifically "domain.com# ?## selector " now normalizes to > > "domain.com#?##selector" (note: no spaces) because elemhideRegExp no longer > > matches. This is consistent with "domain.com# ## selector " and "domain.com# > > @## selector ". I'm not sure if it is right though. > > Well it's good that we're being consistent now, but I agree that maybe we're > being consistently wrong! Really if we're to be as thorough with the > normalisation of whitespace with #?# etc filters as we were with $csp filters we > should handle this case too... but I wonder at what cost that will come, or if > there's any benefit to it. OK, I thought about this a little bit. elemhideRegExp does not accept spaces within the separator (i.e. "# ?#" is not a valid separator). We could allow spaces, but that would be a change in behavior, not sure we want that (at least not as part of this patch). Since spaces are not valid, the normalize function will not treat it as an element hiding filter; this is the current behavior, we should not change it as part of this patch. Therefore the test you suggested is not valid, so let's not include that one. The rest of this change is still valid though, we still can trim spaces around the selector for "#?#" filters.
LGTM (since technically it hasn't changed since last time)
On 2018/04/11 17:34:54, Manish Jethani wrote: > Therefore the test you suggested is not valid, so let's not include that one. Well fair enough about keeping the behaviour the same, but I think we should keep the test in order to demonstrate it since it's kind of surprising IMO. Please could you add a comment for it to explain too?
Patch Set 5: Add tests for incorrect syntax Note that the behavior between "# ?##" and "# @##" is different, the tests cover both cases.
Nice thanks, I'm really happy with this change now. LGTM. 
|
