Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(380)

Issue 29680684: [$csp1 adblockpluscore] Issue 6329 - Allow whitespace in filter option values

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 2 days ago by kzar
Modified:
5 days, 10 hours ago
CC:
sergei
Visibility:
Public.

Description

Issue 6329 - Allow whitespace in filter option values This change is necessary for the addition of the $csp filter option since whitespace in Content Security Policies is significant. I've skipped (most of) the normalisation step, instead doing it at the point the filter is parsed. My key insight there was that we always seem to call Filter.fromText directly afterward the normalise function anyway. This makes the code a little harder to follow perhaps, but has the benefit that we don't wastefully parse element hiding filters during normalisation, only to repeat much of that work again in Filters.fromText. Note that normalisation is not identical as before for RegExpFilters, previously something like "example $ domain=foo.com" might have worked, but no longer.

Patch Set 1 #

Total comments: 15

Patch Set 2 : Addressed some of Manish's initial feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -50 lines) Patch
M lib/filterClasses.js View 1 6 chunks +37 lines, -49 lines 0 comments Download
M lib/synchronizer.js View 1 chunk +1 line, -1 line 0 comments Download
M test/filterClasses.js View 1 1 chunk +28 lines, -0 lines 0 comments Download

Messages

Total messages: 4
kzar
Patch Set 1
3 weeks, 2 days ago (2018-01-26 18:11:39 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode104 lib/filterClasses.js:104: * basic normalisation and parsing where possible before deferring ...
1 week, 6 days ago (2018-02-05 16:09:58 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode721 lib/filterClasses.js:721: let options = match[1].replace(/\s/g, ""); On 2018/02/05 16:09:58, Manish ...
1 week, 5 days ago (2018-02-06 05:14:39 UTC) #3
kzar
5 days, 10 hours ago (2018-02-13 11:56:47 UTC) #4
Patch Set 2 : Addressed some of Manish's initial feedback

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...
lib/filterClasses.js:104: * basic normalisation and parsing where possible
before deferring to the right
On 2018/02/05 16:09:57, Manish Jethani wrote:
> I noticed that the documentation is now using the British spelling of
> "normalization". I don't have a preference either way other than maybe for
> consistency it should use the American spelling.

Kind of ironic heh, Done.

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...
lib/filterClasses.js:111: let filter = Filter.knownFilters.get(text);
On 2018/02/05 16:09:57, Manish Jethani wrote:
> This means that two otherwise identical filters (post normalization) could be
> treated as different filters after this change.

Any ideas? This whole change was a pain, perhaps there is a better approach
which I missed.

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...
lib/filterClasses.js:711: let origText;
On 2018/02/05 16:09:58, Manish Jethani wrote:
> Why not initialize origText to null for consistency?

Done.

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...
lib/filterClasses.js:721: let options = match[1].replace(/\s/g, "");
On 2018/02/06 05:14:39, Manish Jethani wrote:
> On 2018/02/05 16:09:58, Manish Jethani wrote:
> > I don't get this part. Doesn't this mean that "$csp=script-src 'none'" will
> > become "$csp=script-src'none'"?
> 
> I see, so the other patch builds on this. Got it.

Sorry I should have left a comment on the review here to explain, but yep you
got it.

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...
lib/filterClasses.js:731: option = option.substring(0, separatorIndex);
On 2018/02/05 16:09:58, Manish Jethani wrote:
> Any particular reason this is now using String.substring?

Well since I noticed that `substr` was being used mistakenly all this time, but
happened to work since the offset was always 0. Quite misleading.

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...
lib/filterClasses.js:779: return new InvalidFilter(origText,
"filter_invalid_regexp");
On 2018/02/05 16:09:58, Manish Jethani wrote:
> origText is no longer the original text so it has basically lost its meaning.
On
> the other hand it's not very useful either. Maybe we can get rid of the extra
> variable?

The point of `origText` is to show a more useful warning to the user when they
have added an invalid filter. It's still useful for that purpose, though I agree
the name sucks. I'm open to suggestions there.

https://codereview.adblockplus.org/29680684/diff/29680685/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29680684/diff/29680685/lib/synchronizer.js...
lib/synchronizer.js:283: line = Filter.stripJunk(line);
On 2018/02/05 16:09:58, Manish Jethani wrote:
> Since most of the code has moved out anyway, why not just move the rest of it
> into Filter.fromText and get rid of Filter.stripJunk?
> 
> On the other hand, it seems like there might have been a good reason for
> separating out the normalization from the rest of the parsing, since
> Filter.normalize is called from only two places whereas Filter.fromText is
> called from multiple places.
> 
> Anyway I'm in favor of simplifying this further and just keep everything in
> Filter.fromText unless it's a performance concern.

Yea, perhaps we should do that if we continue with this approach. I wonder if
there's a better way, I quite like normalisation happening in separate step, but
it's a pain to allow the whitespace we need that way.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5