Opened on 05/18/2017 at 02:17:51 PM
Closed on 08/29/2019 at 05:43:52 PM
#5258 closed change (rejected)
[emscripten] Make filters and subscriptions easier to use from C++
Reported by: | trev | Assignee: | |
---|---|---|---|
Priority: | P2 | Milestone: | |
Module: | Core | Keywords: | closed-in-favor-of-gitlab |
Cc: | Blocked By: | ||
Blocking: | #4122, #5141, #5259 | Platform: | Unknown / Cross platform |
Ready: | yes | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
https://codereview.adblockplus.org/29574591/ |
Description
Background
We made filter and subscription classes easy to use from JavaScript, yet from C++ it is rather awkward. There is no easy way to convert a Filter instance to something like ActiveFilter safely, and only Filter references can be kept easily (no ActiveFilterPtr). Same issues apply to subscription, and one more: iterating over subscription's filters in C++ is quite complicated.
What to change
- Implement Filter::As<FilterSubClass>() that will either cast the filter to FilterSubClass or return nullptr if the instance is incompatible. Similarly, implement Subscription::As<SubscriptionSubClass().
- Define ActiveFilterPtr and the like in addition to FilterPtr.
- Expose begin() and end() methods on Subscription forwarding the corresponding methods from Subscription.mFilters.
Attachments (0)
Change History (7)
comment:1 Changed on 10/12/2017 at 10:21:51 AM by trev
- Blocking 5259 added; 5137 removed
comment:2 Changed on 10/12/2017 at 11:57:13 AM by trev
- Blocking 5141 added
comment:3 Changed on 10/12/2017 at 12:15:40 PM by trev
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:5 Changed on 12/05/2017 at 03:32:46 PM by abpbot
comment:6 Changed on 12/21/2017 at 10:58:29 AM by trev
- Owner trev deleted
comment:7 Changed on 08/29/2019 at 05:43:52 PM 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.
Some commits referencing this issue have landed: