Opened 3 months ago

Last modified 8 weeks ago

#7094 reviewing change

Lose unnecessary references to filter objects

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords:
Cc: hfiguiere, kzar, jsonesen Blocked By:
Blocking: #7097 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29934588/
https://codereview.adblockplus.org/29946572/
https://codereview.adblockplus.org/29935564/

Description (last modified by mjethani)

Background

The next step for optimizing the memory usage of Adblock Plus is to make temporary Filter objects, i.e. objects that are created, used, and discarded. In order to do this, we must first lose the unnecessary references to Filter objects that exist in other areas. The only place where Filter objects should be stored permanently is the Filter.knownFilters map.

Right now there are not-exactly-necessary references to Filter objects in at least two places:

  1. The filters property of the Subscription class. We can change this property to keep the filter text instead and then convert it to a Filter object when required using Filter.fromText
  2. The filtersBySelector map in lib/elemHide.js. This doesn't need to keep references to the Filter objects either, it could just store the selector and get the Filter object using Filter.fromText

What to change

[...]

Integration notes

With changeset b0170eda8137, the public filters property (array) of the Subscription class is replaced by the following:

  • A filters generator method that yields Filter objects representing filters in the subscription
  • A filterCount property that returns the number of filters in the subscription
  • Similar to the filters generator method, a filterText generator method that yields the text property of the Filter objects
  • A filterAt(index) method that returns the Filter object at a given index
  • addFilter(filter), insertFilterAt(filter, index), deleteFilterAt(index), and clearFilters methods for adding and removing filters to and from the subscription
  • A searchFilter(filter, fromIndex = 0) method that returns a matching Filter object in the subscription (based on the text property of the first parameter)

Please see the JSDoc documentation for the Subscription class for more details.

(Note: There will likely be more changes related to this ticket, but this is it for now.)

Hints for testers

For changeset b0170eda8137, test that filters in a subscription are loaded correctly. For example, the filter /foo^ should block a request to the URL https://example.com/foo/bar (see the DevTools panel and the console to verify this). If a subscription is disabled, the filters in the subscription should no longer work (except those filters that also belong to another subscription that is not disabled). If a subscription is reenabled, the filters in the subscription should work again. Similarly for when a subscription is deleted and then re-added.

Add a custom (user-defined) filter and see that it works. For example, the filter ###foo should hide the element <div id="foo">Test</div>. Remove the filter and see that it no longer works. Add the filter again and reload the extension and see that the filter still appears in the "My filter list" box in the Advanced section of the options page and that it still works.

Add two subscriptions, each one having the same filter ###bar, then disable one of them, and see that the filter still works (i.e. an element like <div id="bar">Test</div> should be hidden). Disable both the subscriptions and see that the filter no longer works. Reenable one of the subscriptions and see that the filter works again.

Change History (13)

comment:1 Changed 3 months ago by mjethani

  • Blocking 7000 added

comment:2 Changed 3 months ago by mjethani

  • Blocked By 7095 added

comment:3 Changed 3 months ago by mjethani

  • Review URL(s) modified (diff)

comment:4 Changed 3 months ago by mjethani

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

comment:5 Changed 3 months ago by mjethani

  • Blocked By 7097 added; 7095 removed

comment:6 Changed 3 months ago by mjethani

  • Blocked By 7097 removed
  • Blocking 7097 added; 7000 removed
  • Summary changed from Lose unnecessary references to Filter objects to Lose unnecessary references to filter objects

comment:7 Changed 2 months ago by abpbot

A commit referencing this issue has landed:
Issue 7094 - Encapsulate management of subscription filters

comment:8 Changed 2 months ago by mjethani

  • Description modified (diff)

comment:9 Changed 2 months ago by mjethani

  • Review URL(s) modified (diff)

comment:10 Changed 8 weeks ago by jsonesen

Could you maybe update the tester hints?

comment:11 Changed 8 weeks ago by mjethani

  • Description modified (diff)

comment:12 Changed 8 weeks ago by mjethani

  • Description modified (diff)

comment:13 Changed 8 weeks ago by mjethani

@jsonesen I've added integration notes and hints for testers.

Note: See TracTickets for help on using tickets.