#6689 closed change (fixed)
Add anti-circumvention filter list to the recommended subscription list
Reported by: | hfiguiere | Assignee: | |
---|---|---|---|
Priority: | P2 | Milestone: | |
Module: | Core | Keywords: | circumvention |
Cc: | arthur, kzar, sebastian, mjethani | Blocked By: | |
Blocking: | #6538, #6699, #6781 | Platform: | Unknown / Cross platform |
Ready: | yes | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
https://codereview.adblockplus.org/29824575/ |
Description (last modified by hfiguiere)
Background
We want to have our own ABP filter list (mostly to deal with anti-circumvention). Therefore we want to have it in the recommended subscriptions list so that it can be easily enabled or disable by the user. See http://hub.eyeo.com/issues/11069 for the rationale.
Once the UI module is modified to allow having more filter lists (see https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/85 - might require permission to view), we can supply that new subscription. (trac issue #6731)
An "Anti-Cirucumvention" section of the option page will allow enabling or disabling this filter list subscription.
The list of subscriptions is provided by adblockpluscore as chrome/content/ui/subscriptions.xml
See also http://hub.eyeo.com/issues/11271
What to change
- Update the subscriptions.xml with a newer version, regenerated, that includes this new filter list subscription.
- Add the optional type field to Subscription.
Note
adblockpluschrome will need to be updated to include both this change and the UI change. Also the subscription init code will need to be adapted to support more than one list: https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/subscriptionInit.js#l202
(issue #6699)
Also this issue doesn't deal about having it enabled by default or when the extension is upgraded. This is will come at a later date (trac issue TBD)
Change History (30)
comment:1 Changed 19 months ago by hfiguiere
- Sensitive set
comment:2 Changed 19 months ago by hfiguiere
- Keywords circumvention added
comment:3 Changed 19 months ago by hfiguiere
- Cc kzar sebastian mjethani added
comment:4 Changed 19 months ago by hfiguiere
- Description modified (diff)
- Summary changed from Add anti-cv filter list to Add anti-cv filter list to the recommended subscription list
comment:5 Changed 19 months ago by mjethani
- Description modified (diff)
- Summary changed from Add anti-cv filter list to the recommended subscription list to Add anti-circumvention filter list to the recommended subscription list
comment:6 Changed 19 months ago by mjethani
@hfiguiere can you add a little more detail to the ticket? What is this filter list supposed to be called (I'm assuming it's not literally called "anti-circumvention")? Is it going to be enough to update subscriptions.xml? I mean, what about the extension's UI, is it going to appear like a regular filter list or as a special list like Acceptable Ads with its own checkbox?
comment:7 Changed 19 months ago by hfiguiere
The UI details are in https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/85
We need a change in the UI to do this properly (this wasn't really known when I filed the issue).
Right now it will be called "ABP filters"
comment:8 follow-up: ↓ 9 Changed 19 months ago by kzar
That link isn't public apparently Hubert, also I agree with Manish that this ticket really does need more information. Like the last issue #6622 please add some background at the start as an introduction and work from there. For example please can you explain why do we want to have our own filter list?
comment:9 in reply to: ↑ 8 ; follow-ups: ↓ 10 ↓ 12 Changed 19 months ago by hfiguiere
Replying to kzar:
That link isn't public apparently Hubert,
Much like this issue, it is marked as "confidential", and should be readable by anyone that have permission for adblockplusui on gitlab (I'm not sure who deal with that, definitely not me).
The tl;dr is that "MORE FILTERS" in the UI will be reworked to allow selecting this filter list (and others).
This issue is only about updating the subscription.xml in core after it gets regenerated. I need to file the issue for the WebExtension to update core + ui. (and I'm not sure yet how to submit to the subscriptionlist repository)
For example please can you explain why do we want to have our own filter list?
This is covered by http://hub.eyeo.com/issues/11069
comment:10 in reply to: ↑ 9 Changed 19 months ago by kzar
Replying to hfiguiere:
Much like this issue, it is marked as "confidential"...
Fair enough, sorry I did not realise this issue was marked confidential.
This issue is only about updating the subscription.xml in core after it gets regenerated...
I understand that, but we still need a more useful issue description.
For example please can you explain why do we want to have our own filter list?
This is covered by http://hub.eyeo.com/issues/11069
Sorry, but I really am asking you to write proper issue descriptions here and going forward. Please can you start doing that.
I know it's a kind of a pain, but it's necessary. Until an issue has a proper description it cannot be triaged and therefore I end up spending a not insignificant amount of my time asking you to add information to your issues or simply giving up and adding it myself (after spending a bunch of time figuring out what it's about).
Code changes are really hard to review before the issue has been fleshed out as well. For example it took a lot of effort to get the description of #6622 into good shape - even though it related to the $rewrite option for which the changes you were chasing me to review. I couldn't even begin the reviews until I understood the context of what we were changing and why, so I had to first nag you to add more details. In the end I had to get clarification from Sebastian in IRC, I had no idea how $redirect and $rewrite were different.
Putting "Look at Hub issue X" in a comment is not enough. You need to put the required information and background into the Trac issue's description. If you're referencing other resources or issues, e.g. a Hub issue, then you should be linking them in the description too.
Don't bury the description in the comments
Don't put any part of the ticket into a comment. Instead change the description and comment your change. The comments are not part of the ticket and all additional parts of the ticket (priority, stage, etc....) don't and cannot apply to the comments.
Imagine the reader has no idea about the plan to add this new filter list, why we're adding it, or where they can go to read more about it. If you'd start the description off with a brief introduction aimed at them it would go a long way.
comment:11 Changed 19 months ago by hfiguiere
- Description modified (diff)
Updated the description. There are a few related trac issues that are currently marked as TBD. I'll be filling these gaps ASAP.
comment:12 in reply to: ↑ 9 Changed 19 months ago by arthur
Replying to hfiguiere:
and I'm not sure yet how to submit to the subscriptionlist repository)
I'm taking care of that usually.
comment:13 Changed 19 months ago by hfiguiere
- Blocked By 6699 added
comment:14 Changed 19 months ago by hfiguiere
- Description modified (diff)
comment:15 Changed 19 months ago by hfiguiere
- Blocked By 6699 removed
- Blocking 6699 added
comment:16 Changed 19 months ago by kzar
- Description modified (diff)
Thanks Hubert, the issue is looking way better now.
comment:17 Changed 19 months ago by hfiguiere
- Description modified (diff)
comment:18 Changed 18 months ago by hfiguiere
- Description modified (diff)
comment:19 Changed 17 months ago by mjethani
- Priority changed from Unknown to P2
- Ready set
comment:20 Changed 17 months ago by mjethani
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:21 Changed 17 months ago by mjethani
- Blocking 6538 added
comment:22 Changed 17 months ago by abpbot
A commit referencing this issue has landed:
Issue 6689 - Add type property to Subscription class
comment:23 Changed 17 months ago by hfiguiere
- Review URL(s) modified (diff)
comment:24 Changed 17 months ago by abpbot
A commit referencing this issue has landed:
Issue 6689 - Added anti-CV filter list subscription
comment:25 Changed 17 months ago by hfiguiere
- Resolution set to fixed
- Status changed from reviewing to closed
comment:26 Changed 17 months ago by hfiguiere
- Sensitive unset
comment:27 Changed 17 months ago by mjethani
- Blocking 6781 added
comment:28 Changed 16 months ago by Ross
Done. Tested with #6783.
ABP 3.2.2102
Chrome 66 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51
comment:29 Changed 11 months ago by mayduavongts
spam
comment:30 Changed 9 months ago by nhaphangali
spam
Rationale http://hub.eyeo.com/issues/11069