Opened 3 years ago

Closed 3 years ago

#4127 closed change (fixed)

[emscripten] Convert subscription classes to C++

Reported by: trev Assignee: trev
Priority: P2 Milestone:
Module: Core Keywords:
Cc: Blocked By: #4989
Blocking: #4122 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29384812/
https://codereview.adblockplus.org/29385742/

Description (last modified by trev)

Background

See #4122 for the rationale.

What to change

Reimplement most of the current API in C++. Quite a few properties don't need to be exposed however, these are only used internally anyway. Other differences:

  • ExternalSubscription shouldn't be implemented, API for third parties adding subscriptions is going away.
  • JavaScript code obtaining references to filters has to call subscription.delete() in order to release the reference.
  • Subscription.fixedTitle is exposed as DownloadableSubscription.fixedTitle (this is where this property belongs).
  • Creating a subscription with a random ID is done via Subscription.fromURL(null) rather than SpecialSubscription.create().
  • Making a subscription to be the default for particular types is done via SpecialSubscription.makeDefaultFor() rather than SpecialSubscription.createForFilter().
  • Subscription.filters array replaced by Subscription.filterCount property and Subscription.filterAt() method. In addition, there should be SpecialSubscription.removeFilterAt() and SpecialSubscription.insertFilterAt() methods to manipulate the filters.
  • Subscription.fromObject() in unimplemented (to become an internal method once FilterStorage has been converted).
  • Subscription.knownSubscriptions lookup table replaced by static Subscription.getKnownSubscription() method.
  • A replacement for Filter.subscriptions reverse mapping should not be implemented. Instead, a Subscription.indexOfFilter() method should be implemented - returning -1 for "no such filter." Later, a Filter.subscriptionCount property will be implemented to indicate the number of active subscriptions the filter is part of (not part of this issue).

Change History (13)

comment:1 Changed 3 years ago by trev

  • Description modified (diff)

comment:2 Changed 3 years ago by trev

  • Priority changed from P3 to P2

comment:3 Changed 3 years ago by trev

  • Blocked By 4989 added

comment:4 Changed 3 years ago by trev

  • Owner set to trev

comment:5 Changed 3 years ago by trev

  • Blocked By 4991 added

comment:6 Changed 3 years ago by trev

  • Blocked By 4991 removed

comment:7 Changed 3 years ago by trev

  • Description modified (diff)

comment:8 Changed 3 years ago by trev

  • Review URL(s) modified (diff)

comment:9 Changed 3 years ago by trev

  • Status changed from new to reviewing

comment:10 Changed 3 years ago by trev

  • Description modified (diff)

I've had a look at our use of the Filter.subscriptions reverse mapping. We really need the subscriptions for a filter in the list of blockable items ("source" column and tooltip entry), issue reporter and filter preferences (selecting the right subscription when opening to edit a particular filter). With the exception of the "source" column which can be considered an optional feature, all these use cases aren't performance critical and can take the slow route by searching all subscriptions.

Elsewhere this list is used to determine whether a filter is currently part of an active subscription, most importantly in FilterListener. This use case can be covered by a numerical Filter.subscriptionCount property counting the number of references to this filter from subscriptions (a subscription could contribute to it multiple times if the filter is duplicated in the list).

comment:11 Changed 3 years ago by trev

  • Review URL(s) modified (diff)

comment:12 Changed 3 years ago by abpbot

Some commits referencing this issue have landed:

comment:13 Changed 3 years ago by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.