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

lib/prefs.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 14 matching lines @@
   patternsfile: "patterns.ini",
   patternsbackups: 5,
   patternsbackupinterval: 24,
   data_directory: "",
   savestats: false,
   privateBrowsing: false,
   subscriptions_fallbackerrors: 5,
   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%",
   update_last_check: 0,
   update_last_error: 0,
   update_soft_expiration: 0,
   update_hard_expiration: 0,
   currentVersion: "0.0",
   notificationdata: {},
   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");
 let listeners = [];
 let isDirty = false;
 let isSaving = false;
 
 function defineProperty(key)
 {
   Object.defineProperty(Prefs, key,
@@ skipping 81 matching lines @@
     defaults[key] = _preconfiguredPrefs[key];
 
 // Define defaults
 for (let key in defaults)
   defineProperty(key);
 
 // Set values of prefs based on defaults
 values = Object.create(defaults);
 
 load();