Opened 3 years ago

Closed 2 years ago

#5137 closed change (fixed)

[emscripten] Add implementation of basic filter storage functionality

Reported by: trev Assignee: trev
Priority: P2 Milestone:
Module: Core Keywords:
Cc: Blocked By: #5216, #5662
Blocking: #4122, #4128, #5138, #5144, #5146, #5259 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29426559/

Description (last modified by trev)

Background

In preparation for #4128, we should move filter list management into Emscripten.

What to change

  • Update bindings generation to allow producing singletons, with this syntax:
    singleton_("FilterStorage")
        .function("addFilter", &FilterStorage::addFilter)
        .property("initialized", &FilterStorage::getInitialized);
    
  • Create compiled/storage/FilterStorage.{cpp|h} defining the FilterStorage singleton class.
  • Expose subscription list as subscriptionCount property and methods subscriptionAt, indexOfSubscription. In JavaScript, implement an iteratable FilterStorage.subscription property that will delete references automatically once the loop iteration is done.
  • Also implement methods getSubscriptionForFilter (aliased to getGroupForFilter for backwards compatibility), addSubscription, removeSubscription, moveSubscription.
  • Add Subscription.listed flag, set by FilterStorage.addSubscription and unset by FilterStorage.removeSubscription - this replaces FilterStorage.knownSubscriptions map.
  • FilterStorage.moveFilter should not be implemented, the same effect can be achieved by calling UserDefinedSubscription.removeFilterAt and UserDefinedSubscription.insertFilterAt. These methods should trigger notifications for listed subscriptions however.
  • Implement FilterStorage.addFilter and FilterStorage.removeFilter helpers without subscription parameter - these are only to be used if the filter location is unknown, otherwise the respective UserDefinedSubscription methods are preferred.

Note

Current filter storage API has considerably more functionality, it should be added in the follow-up issues.

Change History (17)

comment:1 Changed 3 years ago by trev

  • Blocking 5138 added

comment:2 Changed 3 years ago by trev

  • Description modified (diff)

comment:3 Changed 3 years ago by trev

  • Blocking 5144 added

comment:4 Changed 3 years ago by trev

  • Owner set to trev

comment:5 Changed 2 years ago by trev

  • Description modified (diff)

comment:6 Changed 2 years ago by trev

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

comment:7 Changed 2 years ago by trev

  • Blocked By 5216 added

comment:8 Changed 2 years ago by trev

  • Description modified (diff)

comment:9 Changed 2 years ago by trev

  • Blocked By 5258 added

comment:10 Changed 2 years ago by trev

  • Blocking 5259 added

comment:11 Changed 2 years ago by trev

  • Blocking 5146 added

comment:12 Changed 2 years ago by trev

  • Description modified (diff)
  • Summary changed from [emscripten] Convert filter storage to C++ to [emscripten] Add implementation of basic filter storage functionality

I've updated description to describe the current state of affairs. Also, hit counts functionality moved into #5259 and FilterStorage.updateSubscriptionFilters() replacement into #5146.

comment:13 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5137 - [emscripten Added basic filter storage implementation]

comment:14 Changed 2 years ago by sergz

  • Blocked By 5661 added

comment:15 Changed 2 years ago by sergz

  • Blocked By 5662 added

comment:16 Changed 2 years ago by trev

  • Blocked By 5258 removed

comment:17 Changed 2 years ago by trev

  • Blocked By 5661 removed
  • Resolution set to fixed
  • Status changed from reviewing to closed

This isn't really blocked by 5661. Basic functionality has been implemented, parsing and serialization as well as hit counts are addressed by other issues.

Note: See TracTickets for help on using tickets.