Opened on 06/07/2016 at 03:09:28 PM

Closed on 04/13/2017 at 01:50:00 PM

#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).

Attachments (0)

Change History (13)

comment:1 Changed on 06/07/2016 at 03:11:03 PM by trev

  • Description modified (diff)

comment:2 Changed on 06/07/2016 at 03:19:48 PM by trev

  • Priority changed from P3 to P2

comment:3 Changed on 03/14/2017 at 03:34:00 PM by trev

  • Blocked By 4989 added

comment:4 Changed on 03/14/2017 at 04:15:03 PM by trev

  • Owner set to trev

comment:5 Changed on 03/15/2017 at 10:27:05 AM by trev

  • Blocked By 4991 added

comment:6 Changed on 03/15/2017 at 11:43:00 AM by trev

  • Blocked By 4991 removed

comment:7 Changed on 03/15/2017 at 12:45:56 PM by trev

  • Description modified (diff)

comment:8 Changed on 03/15/2017 at 03:09:10 PM by trev

  • Review URL(s) modified (diff)

comment:9 Changed on 03/15/2017 at 03:24:25 PM by trev

  • Status changed from new to reviewing

comment:10 Changed on 03/16/2017 at 06:35:44 AM 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 on 03/16/2017 at 06:31:35 PM by trev

  • Review URL(s) modified (diff)

comment:12 Changed on 04/13/2017 at 01:49:03 PM by abpbot

Some commits referencing this issue have landed:

comment:13 Changed on 04/13/2017 at 01:50:00 PM by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from trev.
 
Note: See TracTickets for help on using tickets.