Issue 4612 - enable AA on first run and make automatic adding of any subscription optional

lib/prefs.js

Index: lib/prefs.js
diff --git a/lib/prefs.js b/lib/prefs.js
index d15c3b02092a21ad843a44f03b6974937f47a555..04cb1b6b608d71d23b6516d1b4736c028b37366d 100644
--- a/lib/prefs.js
+++ b/lib/prefs.js
@@ -32,6 +32,7 @@ let defaults = {
   subscriptions_fallbackurl: "https://adblockplus.org/getSubscription?version=%VERSION%&url=%SUBSCRIPTION%&downloadURL=%URL%&error=%ERROR%&channelStatus=%CHANNELSTATUS%&responseStatus=%RESPONSESTATUS%",
   subscriptions_autoupdate: true,
   subscriptions_exceptionsurl: "https://easylist-downloads.adblockplus.org/exceptionrules.txt",
+  first_run_enable_acceptable_ads: true,

[Felix Dahlke, 2016/11/21 11:41:11]

Why not move this down, so it's right next to the other one?

[sergei, 2016/11/21 14:03:20]

I wanted to keep it close to subscriptions_exceptionsurl because they both
belong to AA functionality. Should it be moved down?

[Felix Dahlke, 2016/11/22 11:45:08]

IMO yes, but you could also move the other thing up I guess.

   documentation_link: "https://adblockplus.org/redirect?link=%LINK%&lang=%LANG%",
   update_url_release: "https://update.adblockplus.org/%NAME%/update.json?type=%TYPE%",
   update_url_devbuild: "https://adblockplus.org/devbuilds/%NAME%/update.json?type=%TYPE%",
@@ -44,10 +45,12 @@ let defaults = {
   notificationurl: "https://notification.adblockplus.org/notification.json",
   suppress_first_run_page: false,
   disable_auto_updates: false,
+  first_run_enable_current_locale_subscription: true,

[Felix Dahlke, 2016/11/21 11:41:11]

Hm, I think the name could be improved. How about
`first_run_enable_ad_blocking`? I guess _how_ we enable ad blocking doesn't
really matter here, because it's obvious that we need to add some list for that.

[sergei, 2016/11/21 14:03:20]

I think that "enable_ad_blocking" is confusing because it implies that there is
a way to disable ad blocking at all which is not correct because here it is
about choosing of the initial subscription. What about
first_run_enable_auto_choosing_subscription?

[sergei, 2016/11/22 09:43:03]

Or maybe first_run_enable_subscription_auto_choosing?

[Felix Dahlke, 2016/11/22 11:45:08]

There is a way to disable ad blocking - this pref :) If we don't auto select an
ad blocking filter list, there is no ad blocking, at least from our perspective.
So that name makes most sense to me.

But then again, I'm wondering if we really want to split these prefs up. There's
no point in automatically adding the AA filter list, since that one works only
in tandem with the ad blocking subscriptions we auto select. So we could combine
it into one pref along the lines of `first_run_subscription_auto_select`, or
maybe in the same vein as the other options rather
`enable_subscription_auto_selection`.

[sergei, 2016/11/22 11:58:43]

No, if these pref values are false there is still ad blocking which is based on
manually added filters, that's how I thought to have tests for
FilterEngine::Matches.
Right, I guess, currently there is no real demand to have them separate, so it
seems to be better to have only one controlling parameter for both of them. I'm
going to use 'first_run_subscription_auto_select'.

   notifications_ignoredcategories: [],
 };
 
-let preconfigurable = ["suppress_first_run_page", "disable_auto_updates"];
+let preconfigurable = ["suppress_first_run_page", "disable_auto_updates",
+  "first_run_enable_acceptable_ads", "first_run_enable_current_locale_subscription"];

[Felix Dahlke, 2016/11/21 11:41:11]

Back when we discussed what should be preconfigurable in our products, those two
things weren' on that list, so let's not make it preconfigurable.

[sergei, 2016/11/21 14:03:20]

Could you please remind where it had been discussed, just wonder about the
reasons that it cannot be here? It seems a bit strange to have some restrictions
here because the user of libadblockplus can always do whatever he wants either
by direct modifying of the code or by injecting a custom implementation of
FileSystem which serves a desired prefs.json when it does not exist.

I would say that absence of documentation of preconfigurable values in readme is
already an indication that they are not supposed to be publicly used, isn't it
enough?

Secondly, it actually was one of the ideas of the issue,
first_run_enable_acceptable_ads and first_run_enable_current_locale_subscription
are introduced only to have such functionality optional, so one can easily
disable it in particular tests. If we remove them from preconfigurable then it
will only complicate the code of tests but anyone can still do it.

[Felix Dahlke, 2016/11/22 11:45:08]

My bad, I forgot how that stuff works :P Yeah of course, if those aren't
preconfigurable, that defeats the purpose.

 
 let values;
 let path = _fileSystem.resolve("prefs.json");