Opened 7 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#7094 closed change (fixed)

Lose unnecessary references to filter objects in subscription objects

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

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

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

We can start with the first one.

What to change

See patches submitted so far in the review URLs field.

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.

With changeset b0170eda8137, the filters() generator in the Subscription class is replaced with a filterText() generator that yields the text of the filter. The text can be converted into a Filter object using the Filter.fromText() function wherever needed. Also the filterAt() method has been replaced with filterTextAt(), which also now returns the text of the filter.

Hints for testers

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 (25)

comment:1 Changed 7 months ago by mjethani

  • Blocking 7000 added

comment:2 Changed 7 months ago by mjethani

  • Blocked By 7095 added

comment:3 Changed 7 months ago by mjethani

  • Review URL(s) modified (diff)

comment:4 Changed 7 months ago by mjethani

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

comment:5 Changed 7 months ago by mjethani

  • Blocked By 7097 added; 7095 removed

comment:6 Changed 7 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 6 months ago by abpbot

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

comment:8 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:9 Changed 6 months ago by mjethani

  • Review URL(s) modified (diff)

comment:10 Changed 6 months ago by jsonesen

Could you maybe update the tester hints?

comment:11 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:13 Changed 6 months ago by mjethani

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

comment:14 Changed 3 months ago by abpbot

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

comment:15 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Looks to be working as described with no errors in the background console.

ABP 3.4.3.2253
Firefox 65.0.1 / 51 / Windows 10
Firefox Mobile 65.0.1 / Android 7.1.1
Chrome 72.0.3626.109 / 49.0.2623.75 / Windows 10
Opera 58.0.3135.65 / 36.0.2130.80 / Windows 10
Edge 44.17763.1.0 / Windows 10

comment:16 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 7094 - Keep subscription filters by text only

comment:17 follow-up: Changed 3 months ago by greiner

  • Cc greiner added

Since this ticket has already been marked as verified by a tester, why are there still changes landing in here? That's making it quite hard to keep track of which changes have already landed and which ones are yet to come.

comment:18 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 7094 - Link filters with subscriptions in INI parser

comment:19 in reply to: ↑ 17 ; follow-up: Changed 2 months ago by mjethani

Replying to greiner:

Since this ticket has already been marked as verified by a tester, why are there still changes landing in here? That's making it quite hard to keep track of which changes have already landed and which ones are yet to come.

This is a work-in-progress ticket, similar to #6891 and #6741. What is the proper way to land changes against such tickets? Perhaps these should be meta tickets and there should be a specific ticket for each change that lands.

comment:20 in reply to: ↑ 19 Changed 2 months ago by greiner

Yes, that's how we've been handling such long-running tickets in the past and I can confirm that splitting tickets up like that would help us with keeping track of and making the necessary adjustments for any breaking changes that make it into a specific extension release.

comment:21 Changed 2 months ago by mjethani

Fair enough, I am starting to close tickets like these. Moving forward I'll try to make sure that changes land against a ticket only if it is clearly defined what changes need to be made. If it sounds like a meta ticket (make such and such changes everywhere in general), we'll file specific tickets for specific changes and make them block the meta ticket until all changes are done.

comment:22 Changed 2 months ago by greiner

Thanks, that's very much appreciated!

comment:23 Changed 7 weeks ago by mjethani

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

Since this issue is already in a dependency update, I am limiting the scope of the changes here and closing this. I shall file a new issue for further changes (which in fact depend on other changes elsewhere (e.g. #7029)).

comment:24 Changed 7 weeks ago by mjethani

  • Description modified (diff)
  • Summary changed from Lose unnecessary references to filter objects to Lose unnecessary references to filter objects in subscription objects

comment:25 Changed 7 weeks ago by mjethani

  • Description modified (diff)
Note: See TracTickets for help on using tickets.