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

Issue 29801609: Issue 6733 - Allow empty values in filter options (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 2 weeks ago by Manish Jethani
Modified:
2 months, 1 week ago
Reviewers:
hub, kzar
CC:
sergei
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6733 - Allow empty values in filter options

Patch Set 1 #

Total comments: 4

Patch Set 2 : Rebase and simplify #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -2 lines) Patch
M lib/filterClasses.js View 1 2 chunks +2 lines, -2 lines 5 comments Download
M test/filterClasses.js View 1 2 chunks +12 lines, -0 lines 1 comment Download

Messages

Total messages: 8
Manish Jethani
3 months, 2 weeks ago (2018-06-07 11:51:14 UTC) #1
Manish Jethani
Patch Set 1
3 months, 2 weeks ago (2018-06-07 11:52:55 UTC) #2
kzar
https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js#oldcode592 lib/filterClasses.js:592: if (sitekeys != null) How come you remove this ...
3 months, 2 weeks ago (2018-06-07 13:21:56 UTC) #3
Manish Jethani
Patch Set 2: Rebase and simplify https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js#oldcode592 lib/filterClasses.js:592: if (sitekeys != ...
2 months, 3 weeks ago (2018-06-25 13:13:45 UTC) #4
kzar
https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js#newcode804 lib/filterClasses.js:804: if (value == null) On 2018/06/25 13:13:44, Manish Jethani ...
2 months, 1 week ago (2018-07-12 10:36:13 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js#newcode804 lib/filterClasses.js:804: if (value == null) On 2018/07/12 10:36:13, kzar wrote: ...
2 months, 1 week ago (2018-07-12 10:52:55 UTC) #6
kzar
LGTM https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js#newcode804 lib/filterClasses.js:804: if (value == null) On 2018/07/12 10:52:55, Manish ...
2 months, 1 week ago (2018-07-12 11:04:20 UTC) #7
Manish Jethani
2 months, 1 week ago (2018-07-12 11:11:13 UTC) #8
On 2018/07/12 11:04:20, kzar wrote:
> LGTM

Thanks!

https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.j...
lib/filterClasses.js:804: if (value == null)
On 2018/07/12 11:04:20, kzar wrote:
> On 2018/07/12 10:52:55, Manish Jethani wrote:
> > On 2018/07/12 10:36:13, kzar wrote:
> > > On 2018/06/25 13:13:44, Manish Jethani wrote:
> > > > Blank value is allowed for $rewrite here while still an error for
$domain,
> > > > $sitekeys, and $csp.
> > > 
> > > So by checking for `value == null` instead of `!value` we'd allow
> > "...$rewrite="
> > > but not "rewrite$", or do I misunderstand? Either way, why don't you check
> for
> > > `!value` here?
> > 
> > Yes, we're allowing $rewrite= but not $rewrite, because this is not a "flag"
> > like $match-case, it needs a value but it can take an empty value.
> 
> Ah right I see, we anticipate the confusion about adding the "=" for the users
> who do want an empty value is less than the confusion of users who don't
> understand the option at all and attempt to use it without a value.

Yup.
Sign in to reply to this message.

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