Opened on 09/10/2018 at 09:53:05 AM
Closed on 09/17/2018 at 11:01:42 PM
Last modified on 02/07/2019 at 03:23:34 AM
#6937 closed change (fixed)
Lowercase RegExpFilter domains on demand
| Reported by: | mjethani | Assignee: | mjethani |
|---|---|---|---|
| Priority: | P2 | Milestone: | |
| Module: | Core | Keywords: | |
| Cc: | sebastian, jsonesen, hfiguiere, sergz | Blocked By: | |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | yes | Confidential: | no |
| Tester: | Unknown | Verified working: | no |
| Review URL(s): | |||
Description (last modified by mjethani)
Background
The domains property of RegExpFilter objects is processed on demand, yet we're
lowercasing the domain source in advance. In the domains getter we check if the domain source is already lowercased to avoid lowercasing again. This is unnecessary; we can always lowercase on demand, which means there's no need for the domainSourceIsLowerCase property. This removes one condition from the domains getter and simplifies the code further.
It also saves ~1 MB in the memory footprint.
What to change
In RegExpFilter.fromText, do not lowercase the value of the domain option. In the domains getter, always lowercase the domain source. Remove the domainSourceIsLowerCase property since it is no longer necessary.
Hints for testers
Test that blocking filters with uppercase and mixed-case domains are still working. For example, the filter ||foo.com^$domain=BaR.cOM should block any requests to a foo.com URL on a page on bar.com (but not on pages on any other domain). Likewise, ||foo.com^$domain=BaR.cOM,~WwW.BAr.COm should block requests to a foo.com URL from a page on bar.com but not on www.bar.com.
Attachments (0)
Change History (5)
comment:1 Changed on 09/10/2018 at 09:54:27 AM by mjethani
- Cc sebastian jsonesen hfiguiere added
comment:2 Changed on 09/10/2018 at 09:55:17 AM by mjethani
- Cc sergz added
comment:3 Changed on 09/17/2018 at 10:53:08 PM by abpbot
comment:4 Changed on 09/17/2018 at 11:01:42 PM by mjethani
- Description modified (diff)
- Resolution set to fixed
- Status changed from new to closed
comment:5 Changed on 02/07/2019 at 03:23:34 AM by abpbot
A commit referencing this issue has landed:
Issue 6937 - Lowercase RegExpFilter domains on demand
A commit referencing this issue has landed:
Issue 6937 - Lowercase RegExpFilter domains on demand