Opened 15 months ago

Closed 15 months ago

Last modified 13 months ago

#6927 closed change (fixed)

Add pattern property to RegExpFilter to expose already extracted pattern

Reported by: mjethani Assignee: mjethani
Priority: P3 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29874589/

Description (last modified by mjethani)

Background

The findKeyword method in lib/matcher.js tries to extract the pattern part of the filter text. This is already extracted at the time of creating the Filter object and is available as the value of the internal regexpSource property until the regexp getter is called for the first time. Since the text never really goes away from memory (sliced strings in V8), it makes sense to hold on to it and make it available as a property on the Filter object itself. This way findKeyword can avoid doing all the redundant work.

What to change

Rename RegExpFilter's regexpSource to pattern and make it officially public (whatever this means). In the regexp getter, do not set this property to null. In lib/matcher.js, use the value of this property instead of extracting the same value from the filter text again.

Also delete Filter.regexpRegExp since it is no longer required.

Hints for testers

In general make sure that blocking filters are working correctly:

  1. A filter with a pattern like foo*bar should block a URL like https://example.com/foo/a/b/bar?c&d
  2. A filter with a regular expression like /foo.*bar/ should block a URL like https://example.com/foo/a/b/bar?c&d

Change History (6)

comment:1 Changed 15 months ago by mjethani

  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

comment:2 Changed 15 months ago by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:3 Changed 15 months ago by mjethani

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed 15 months ago by abpbot

A commit referencing this issue has landed:
Issue 6927 - Expose pattern from RegExpFilter

comment:5 Changed 15 months ago by mjethani

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

comment:6 Changed 13 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

Note: See TracTickets for help on using tickets.