Opened 15 months ago

Closed 14 months ago

Last modified 10 months ago

#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):

https://codereview.adblockplus.org/29876573/

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.

Change History (5)

comment:1 Changed 15 months ago by mjethani

  • Cc sebastian jsonesen hfiguiere added

comment:2 Changed 15 months ago by mjethani

  • Cc sergz added

comment:3 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 6937 - Lowercase RegExpFilter domains on demand

comment:4 Changed 14 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

comment:5 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 6937 - Lowercase RegExpFilter domains on demand

Note: See TracTickets for help on using tickets.