Issue 29876573: Issue 6937 - Lowercase RegExpFilter domains on demand (Closed)
|
Created:
Sept. 7, 2018, 3:04 a.m. by Manish Jethani Modified:
Sept. 17, 2018, 10:54 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #
Total comments: 4
MessagesTotal messages: 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(); Im super happy to see things like this go away hehe. Why did subclasses ever change case?
I'll file an issue, but here's a brief explanation. The domains property of RegExpFilter objects is processed on demand, yet we're lowercasing the domain source first (basically we're doing it backwards, because the domains property of ContentFilter objects is in fact processed right away). We can always lowercase on demand, which means there's no need for the `domainIsLowerCase` property, and this removes one condition from the domains getter and simplifies the code further. It also saves ~1 MB in the memory footprint.
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/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.
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/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
LGTM
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. 
|
||||||||||||||||||||||||||||||||||||||||||||||
