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

Issue 29800595: Issue 6735 - Store domains in lower case (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by Manish Jethani
Modified:
1 year, 5 months ago
Reviewers:
kzar, sergei
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 5

Patch Set 2 : Rebase #

Patch Set 3 : Update tests to check for casing #

Total comments: 7

Patch Set 4 : Rebase #

Patch Set 5 : Update logic to minimize code #

Total comments: 14

Patch Set 6 : Use Map version of RegExpFilter.typeMap #

Patch Set 7 : Disallow missing values in domain, sitekey, and rewrite #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -67 lines) Patch
M lib/elemHide.js View 1 chunk +1 line, -1 line 0 comments Download
M lib/filterClasses.js View 1 2 3 4 5 6 5 chunks +59 lines, -39 lines 5 comments Download
M test/filterClasses.js View 1 2 4 chunks +27 lines, -27 lines 0 comments Download

Messages

Total messages: 21
Manish Jethani
1 year, 6 months ago (2018-06-06 14:56:35 UTC) #1
Manish Jethani
Patch Set 1 Domains are lowercase by default, both in the filter list and as ...
1 year, 6 months ago (2018-06-06 14:59:13 UTC) #2
sergei
https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js#newcode471 test/filterClasses.js:471: "b$la$sitekey=foo,domain=domain.com|foo.com,csp=c s p" it seems the input should be ...
1 year, 6 months ago (2018-06-07 09:25:17 UTC) #3
Manish Jethani
Patch Set 2: Rebase https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js#newcode471 test/filterClasses.js:471: "b$la$sitekey=foo,domain=domain.com|foo.com,csp=c s p" On 2018/06/07 ...
1 year, 6 months ago (2018-06-07 10:22:14 UTC) #4
sergei
https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js#newcode471 test/filterClasses.js:471: "b$la$sitekey=foo,domain=domain.com|foo.com,csp=c s p" On 2018/06/07 10:22:14, Manish Jethani wrote: ...
1 year, 6 months ago (2018-06-07 10:44:49 UTC) #5
Manish Jethani
Patch Set 3: Update tests to check for casing https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29800595/diff/29800596/test/filterClasses.js#newcode471 test/filterClasses.js:471: ...
1 year, 6 months ago (2018-06-07 11:03:31 UTC) #6
sergei
LGTM, @Dave could you please take a look?
1 year, 6 months ago (2018-06-07 11:09:06 UTC) #7
kzar
Overall seems like a cool idea, I like the implementation too the new logic is ...
1 year, 6 months ago (2018-06-07 13:16:03 UTC) #8
sergei
On 2018/06/07 13:16:03, kzar wrote: > Overall seems like a cool idea, I like the ...
1 year, 6 months ago (2018-06-07 13:20:49 UTC) #9
Manish Jethani
On 2018/06/07 13:20:49, sergei wrote: > On 2018/06/07 13:16:03, kzar wrote: > > [...] > ...
1 year, 6 months ago (2018-06-07 14:06:32 UTC) #10
kzar
On 2018/06/07 14:06:32, Manish Jethani wrote: > On 2018/06/07 13:20:49, sergei wrote: > > On ...
1 year, 6 months ago (2018-06-07 14:24:26 UTC) #11
Manish Jethani
Patch Set 4: Rebase Patch Set 5: Update logic to minimize code The changes are ...
1 year, 6 months ago (2018-06-08 05:00:42 UTC) #12
Manish Jethani
Patch Set 6: Use Map version of RegExpFilter.typeMap Here's another idea: We can avoid some ...
1 year, 6 months ago (2018-06-08 05:31:15 UTC) #13
Manish Jethani
So I think Patch Set 5 and 6 should go into their own patch, perhaps ...
1 year, 6 months ago (2018-06-08 05:38:18 UTC) #14
Manish Jethani
On 2018/06/08 05:31:15, Manish Jethani wrote: > Patch Set 6: Use Map version of RegExpFilter.typeMap ...
1 year, 6 months ago (2018-06-08 06:06:13 UTC) #15
kzar
Patch Set 5 LGTM, though I'd like to get Sergei's opinion on the slight change ...
1 year, 6 months ago (2018-06-08 09:30:32 UTC) #16
sergei
https://codereview.adblockplus.org/29800595/diff/29801686/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29800595/diff/29801686/lib/filterClasses.js#newcode788 lib/filterClasses.js:788: if (value) On 2018/06/08 09:30:32, kzar wrote: > On ...
1 year, 6 months ago (2018-06-08 13:28:11 UTC) #17
Manish Jethani
https://codereview.adblockplus.org/29800595/diff/29801686/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29800595/diff/29801686/lib/filterClasses.js#newcode788 lib/filterClasses.js:788: if (value) On 2018/06/08 13:28:11, sergei wrote: > On ...
1 year, 6 months ago (2018-06-08 14:12:39 UTC) #18
Manish Jethani
Patch Set 7: Disallow missing values in domain, sitekey, and rewrite https://codereview.adblockplus.org/29800595/diff/29802583/lib/filterClasses.js File lib/filterClasses.js (right): ...
1 year, 6 months ago (2018-06-08 14:19:42 UTC) #19
Manish Jethani
https://codereview.adblockplus.org/29800595/diff/29802583/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29800595/diff/29802583/lib/filterClasses.js#newcode776 lib/filterClasses.js:776: if (type == RegExpFilter.typeMap.CSP && value) By the way ...
1 year, 6 months ago (2018-06-08 14:21:14 UTC) #20
kzar
1 year, 5 months ago (2018-06-15 10:44:03 UTC) #21
LGTM

https://codereview.adblockplus.org/29800595/diff/29802583/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29800595/diff/29802583/lib/filterClasses.j...
lib/filterClasses.js:776: if (type == RegExpFilter.typeMap.CSP && value)
On 2018/06/08 14:21:14, Manish Jethani wrote:
> By the way in the case of csp we quietly ignore the option if it has no value,
> so we are not being consistent. Anyway we should address these things
> separately, not as part of this patch.

Yea, agreed. By the way Rosie is reimplementing this stuff in Python ATM, so if
you change the behaviour ping her in IRC.

https://codereview.adblockplus.org/29800595/diff/29802583/lib/filterClasses.j...
lib/filterClasses.js:799: if (!value)
On 2018/06/08 14:19:42, Manish Jethani wrote:
> There's a bit of code repetition here but I'm OK with it.

Yea, I think it's fine.
Sign in to reply to this message.

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