Opened 5 months ago

Closed 3 months ago

Last modified 3 months ago

#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/
https://codereview.adblockplus.org/29827630/

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 (28)

comment:1 Changed 5 months ago by hfiguiere

  • Sensitive set

comment:2 Changed 5 months ago by hfiguiere

  • Keywords circumvention added

comment:3 Changed 5 months ago by hfiguiere

  • Cc kzar sebastian mjethani added

comment:4 Changed 5 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 5 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 5 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 5 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: Changed 5 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: Changed 5 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 5 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.

From the TracGuide;

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.

Last edited 5 months ago by kzar (previous) (diff)

comment:11 Changed 5 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 5 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 5 months ago by hfiguiere

  • Blocked By 6699 added

comment:14 Changed 5 months ago by hfiguiere

  • Description modified (diff)

comment:15 Changed 5 months ago by hfiguiere

  • Blocked By 6699 removed
  • Blocking 6699 added

comment:16 Changed 5 months ago by kzar

  • Description modified (diff)

Thanks Hubert, the issue is looking way better now.

comment:17 Changed 5 months ago by hfiguiere

  • Description modified (diff)

comment:18 Changed 4 months ago by hfiguiere

  • Description modified (diff)

comment:19 Changed 4 months ago by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:20 Changed 4 months ago by mjethani

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:21 Changed 4 months ago by mjethani

  • Blocking 6538 added

comment:22 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6689 - Add type property to Subscription class

comment:23 Changed 3 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:24 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6689 - Added anti-CV filter list subscription

comment:25 Changed 3 months ago by hfiguiere

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

comment:26 Changed 3 months ago by hfiguiere

  • Sensitive unset

comment:27 Changed 3 months ago by mjethani

  • Blocking 6781 added

comment:28 Changed 3 months ago by Ross

Done. Tested with #6783.

ABP 3.2.2102
Chrome 66 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51

Note: See TracTickets for help on using tickets.