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/ |
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: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: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)
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:
- Issue 4127 - [emscripten Convert subscription classes to C++ - Part 1]
- Issue 4127 - [emscripten Convert subscription classes to C++ - Part 2]
comment:13 Changed on 04/13/2017 at 01:50:00 PM by trev
- Resolution set to fixed
- Status changed from reviewing to closed
Note: See
TracTickets for help on using
tickets.
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).