Opened 6 months ago

Closed 3 weeks ago

#7452 closed change (rejected)

Stop keeping element hiding filter objects in memory

Reported by: mjethani Assignee: mjethani
Priority: P3 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
Cc: hfiguiere Blocked By: #7453
Blocking: #7097 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

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

Description (last modified by mjethani)

Background

After #7094 and #7029, the only remaining references to Filter instances are in the "containers" (matcher, element hiding module, etc.). Of these, element hiding filters are the easiest to work with without keeping references to the Filter instances themselves (see patch). We could lose the references to the Filter instances in the element hiding implementation module and then make the objects ephemeral (see idea in #7097).

For this to work however we need to find a solution for the state properties disabled, hitCount, and lastHit (see discussion in #7095). We cannot remove these properties, but it is also true that in most cases these properties are never set (in fact, in the WebExt version of Adblock Plus they are not used at all). It may be possible to implement something where a Filter instance is cached only if one of these properties is set.

What to change

At a high level:

  • In lib/elemHide.js, keep filters by text rather than by Filter instances
  • In lib/filterClasses.js, in the Filter.fromText() function, do not cache the return value in Filter.knownFilters if the object has certain properties; in this case, do not cache it if it's an instance of ElemHideFilter
  • TBD: Find a way to preserve the values of the state properties disabled, hitCount, and lastHit if any of the values are set to anything other than the default value of the property (in which case the state is also serialized) (see #7453)

Change History (7)

comment:1 Changed 6 months ago by mjethani

  • Blocking 7097 added; 7000 removed

comment:2 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 6 months ago by mjethani

  • Owner set to mjethani

comment:4 Changed 6 months ago by mjethani

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

comment:5 Changed 6 months ago by mjethani

  • Blocked By 7453 added

comment:6 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 3 weeks ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from reviewing to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Note: See TracTickets for help on using tickets.