Issue 29827646: Issue 6783 - opt-in to anti circumvention filter list on upgrade (Closed)
|
Created:
July 12, 2018, 2:24 a.m. by hub Modified:
July 17, 2018, 2:37 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 6783 - opt-in to anti circumvention filter list on upgrade
Patch Set 1 #
Total comments: 3
Patch Set 2 : Don't re-ad default easylist #
Total comments: 2
Patch Set 3 : Updated comments #
Total comments: 7
Patch Set 4 : Updated comments #
Total comments: 4
Patch Set 5 : Change the name of the preference key #MessagesTotal messages: 13
https://codereview.adblockplus.org/29827646/diff/29827647/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29827647/lib/subscriptionIni... lib/subscriptionInit.js:215: if (shouldAddDefaultSubscription() || !Prefs.subscriptions_checkedanticv) But this will add an "ads" list as well. Shouldn't there be an additional check somewhere to add only the "circumvention" list?
https://codereview.adblockplus.org/29827646/diff/29827647/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29827647/lib/subscriptionIni... lib/subscriptionInit.js:215: if (shouldAddDefaultSubscription() || !Prefs.subscriptions_checkedanticv) On 2018/07/12 07:54:27, Manish Jethani wrote: > But this will add an "ads" list as well. Shouldn't there be an additional check > somewhere to add only the "circumvention" list? Yea, this logic doesn't look right. Perhaps split out a function to add and enable default subscriptions, then just hardcode the part that adds the anti-circumvention subscription if the preference is falsey.
Updated patch. https://codereview.adblockplus.org/29827646/diff/29827647/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29827647/lib/subscriptionIni... lib/subscriptionInit.js:215: if (shouldAddDefaultSubscription() || !Prefs.subscriptions_checkedanticv) On 2018/07/12 10:07:59, kzar wrote: > On 2018/07/12 07:54:27, Manish Jethani wrote: > > But this will add an "ads" list as well. Shouldn't there be an additional > check > > somewhere to add only the "circumvention" list? > > Yea, this logic doesn't look right. > > Perhaps split out a function to add and enable default subscriptions, then just > hardcode the part that adds the anti-circumvention subscription if the > preference is falsey. This came to my mind before falling asleep. I'll fix it.
Looks good to me, except for the one comment. Let's get feedback from Dave and/or Sebastian. https://codereview.adblockplus.org/29827646/diff/29828607/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29828607/lib/subscriptionIni... lib/subscriptionInit.js:214: // Ensure the Anti Circumvention is added on upgrade. You mean anti-circumvention subscription?
https://codereview.adblockplus.org/29827646/diff/29828607/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29828607/lib/subscriptionIni... lib/subscriptionInit.js:214: // Ensure the Anti Circumvention is added on upgrade. On 2018/07/13 15:20:25, Manish Jethani wrote: > You mean anti-circumvention subscription? Done.
https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... lib/subscriptionInit.js:61: * Determines whether to add the default ad blocking subscription. Please can you change "subscription" to "subscriptions" and otherwise adjust this comment? Also the function name itself. https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... lib/subscriptionInit.js:213: // Add default ad blocking subscription (e.g. EasyList) Please could you fix this comment? https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... lib/subscriptionInit.js:214: // Ensure the anti-circumvention subscription is added on upgrade. Nit: Please could you move this line of the comment that you added above the code you added line 234? https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... lib/subscriptionInit.js:234: let type = node.getAttribute("type"); I wonder why we do this before the `url` check, since our circumvention filter will always have a URL right?
Updated patch. https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... lib/subscriptionInit.js:213: // Add default ad blocking subscription (e.g. EasyList) On 2018/07/16 13:39:23, kzar wrote: > Please could you fix this comment? Done. https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... lib/subscriptionInit.js:214: // Ensure the anti-circumvention subscription is added on upgrade. On 2018/07/16 13:39:23, kzar wrote: > Nit: Please could you move this line of the comment that you added above the > code you added line 234? Done (and edited). https://codereview.adblockplus.org/29827646/diff/29829608/lib/subscriptionIni... lib/subscriptionInit.js:234: let type = node.getAttribute("type"); On 2018/07/16 13:39:23, kzar wrote: > I wonder why we do this before the `url` check, since our circumvention filter > will always have a URL right? True that. Moved inside the if (url)
LGTM https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... lib/subscriptionInit.js:249: Prefs.subscriptions_checkedanticv = true; Nit: Maybe "added" instead of "checked" in the preference name? To me that would be more illustrative of this logic.
https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... lib/subscriptionInit.js:249: Prefs.subscriptions_checkedanticv = true; On 2018/07/16 16:37:47, kzar wrote: > Nit: Maybe "added" instead of "checked" in the preference name? To me that would > be more illustrative of this logic. I use "checked" because the code checked it and took action. I don't mind changing the key "subscriptions_addedanticv" if you prefer.
https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... lib/subscriptionInit.js:249: Prefs.subscriptions_checkedanticv = true; On 2018/07/16 19:07:35, hub wrote: > On 2018/07/16 16:37:47, kzar wrote: > > Nit: Maybe "added" instead of "checked" in the preference name? To me that > would > > be more illustrative of this logic. > > I use "checked" because the code checked it and took action. I don't mind > changing the key "subscriptions_addedanticv" if you prefer. Up to you.
https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29827646/diff/29831570/lib/subscriptionIni... lib/subscriptionInit.js:249: Prefs.subscriptions_checkedanticv = true; On 2018/07/17 10:32:32, kzar wrote: > On 2018/07/16 19:07:35, hub wrote: > > On 2018/07/16 16:37:47, kzar wrote: > > > Nit: Maybe "added" instead of "checked" in the preference name? To me that > > would > > > be more illustrative of this logic. > > > > I use "checked" because the code checked it and took action. I don't mind > > changing the key "subscriptions_addedanticv" if you prefer. > > Up to you. Changed it.
LGTM 
|
