Issue 1712 - Don't add "Adblock Warning Removal List" subscription on Fennec

This review is related to current ticket:
https://issues.adblockplus.org/ticket/1712

[saroyanm, 2014-12-22 00:11:58]

Wladimir can you please have a look when you'll have time, also looks to be a
quick one.

[Wladimir Palant, 2015-01-06 13:18:12]

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
lib/ui.js:820: if ((Services.vc.compare(prevVersion, "2.5") < 0) &&
!Utils.isFennec)
I guess there is a misunderstanding here - not adding this list will break the
entire anti-adblock feature. We should keep adding it, but we shouldn't consider
it for the subscription selector. See lib/ui.js, function initCheckboxes(). When
currentSubscription is determined, we filter out the acceptable ads list - we
should filter out the anti-adblock list as well.

[saroyanm, 2015-01-06 16:03:06]

Updated

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
lib/ui.js:820: if ((Services.vc.compare(prevVersion, "2.5") < 0) &&
!Utils.isFennec)
On 2015/01/06 13:18:12, Wladimir Palant wrote:
> I guess there is a misunderstanding here - not adding this list will break the
> entire anti-adblock feature. We should keep adding it, but we shouldn't
consider
> it for the subscription selector. See lib/ui.js, function initCheckboxes().
When
> currentSubscription is determined, we filter out the acceptable ads list - we
> should filter out the anti-adblock list as well.

Thanks for pointing this out, but don't we also need to have a checkbox for it
in Fennec ?  Because by default it looks to be disabled, should I create
separate issue for that ?

[Wladimir Palant, 2015-01-06 17:05:21]

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
lib/ui.js:820: if ((Services.vc.compare(prevVersion, "2.5") < 0) &&
!Utils.isFennec)
On 2015/01/06 16:03:07, saroyanm wrote:
> Thanks for pointing this out, but don't we also need to have a checkbox for it
> in Fennec ?  Because by default it looks to be disabled, should I create
> separate issue for that ?

It is normally being enabled via the opt-in prompt. I'm not sure whether it was
ever implemented for Firefox Mobile, you can test it by navigating to
bouweenpc.nl for example.

http://codereview.adblockplus.org/4759471792324608/diff/5639274879778816/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/4759471792324608/diff/5639274879778816/lib/...
lib/ui.js:162: subscription.url != Prefs.subscriptions_exceptionsurl &&
subscription.url != Prefs.subscriptions_antiadblockurl);
Nit: newline after && to keep this line reasonably short?

[saroyanm, 2015-01-06 17:32:36]

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/4759471792324608/diff/5629499534213120/lib/...
lib/ui.js:820: if ((Services.vc.compare(prevVersion, "2.5") < 0) &&
!Utils.isFennec)
On 2015/01/06 17:05:21, Wladimir Palant wrote:
> On 2015/01/06 16:03:07, saroyanm wrote:
> > Thanks for pointing this out, but don't we also need to have a checkbox for
it
> > in Fennec ?  Because by default it looks to be disabled, should I create
> > separate issue for that ?
> 
> It is normally being enabled via the opt-in prompt. I'm not sure whether it
was
> ever implemented for Firefox Mobile, you can test it by navigating to
> bouweenpc.nl for example.

The prompt is not showing up on mobile unfortunately,  anyway I think this can
be separate issue, I'll investigate that.

http://codereview.adblockplus.org/4759471792324608/diff/5639274879778816/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/4759471792324608/diff/5639274879778816/lib/...
lib/ui.js:162: subscription.url != Prefs.subscriptions_exceptionsurl &&
subscription.url != Prefs.subscriptions_antiadblockurl);
On 2015/01/06 17:05:21, Wladimir Palant wrote:
> Nit: newline after && to keep this line reasonably short?

Done.

[Wladimir Palant, 2015-01-06 17:43:23]

LGTM