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

Issue 29876573: Issue 6937 - Lowercase RegExpFilter domains on demand (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by Manish Jethani
Modified:
1 day, 19 hours ago
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -19 lines) Patch
M lib/filterClasses.js View 3 chunks +2 lines, -19 lines 4 comments Download

Messages

Total messages: 7
Manish Jethani
1 week, 5 days ago (2018-09-07 03:04:38 UTC) #1
Jon Sonesen
https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.js#newcode460 lib/filterClasses.js:460: let source = this.domainSource.toLowerCase(); Im super happy to see ...
1 week, 5 days ago (2018-09-07 03:09:33 UTC) #2
Manish Jethani
I'll file an issue, but here's a brief explanation. The domains property of RegExpFilter objects ...
1 week, 5 days ago (2018-09-07 03:10:03 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.js#newcode460 lib/filterClasses.js:460: let source = this.domainSource.toLowerCase(); On 2018/09/07 03:09:33, Jon Sonesen ...
1 week, 5 days ago (2018-09-07 03:42:44 UTC) #4
Jon Sonesen
https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.js#newcode460 lib/filterClasses.js:460: let source = this.domainSource.toLowerCase(); On 2018/09/07 03:42:44, Manish Jethani ...
2 days ago (2018-09-17 18:11:33 UTC) #5
hub
LGTM
1 day, 23 hours ago (2018-09-17 18:58:36 UTC) #6
Sebastian Noack
1 day, 23 hours ago (2018-09-17 19:20:59 UTC) #7
https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29876573/diff/29876574/lib/filterClasses.j...
lib/filterClasses.js:460: let source = this.domainSource.toLowerCase();
On 2018/09/17 18:11:33, Jon Sonesen wrote:
> On 2018/09/07 03:42:44, Manish Jethani wrote:
> > On 2018/09/07 03:09:33, Jon Sonesen wrote:
> > > Im super happy to see things like this go away hehe. Why did subclasses
ever
> > > change case?
> > 
> > I'm guessing "historical reasons." It may have been the case that the
domains
> > were already being lowercased as part of the parsing in
> `RegExpFilter.fomText`,
> > so it made sense to avoid lowercasing them again. This is definitely not the
> > case now; as in, we can just choose not to lowercase the domains for any
> filter
> > and then lowercase them later when needed.
> 
> Yeah makes sense, thanks for explaining. I tested this and seems good so
unless
> hub or snoack have an issue I say LGTM

This was an optimization at some point, but indeed, with the current design not
so much anymore. LGTM.
Sign in to reply to this message.

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