Issue 2325 - Add a way to set settings in libadblockplus for FRP and automatic updates

lib/prefs.js

Index: lib/prefs.js
===================================================================
--- a/lib/prefs.js
+++ b/lib/prefs.js
@@ -41,10 +41,14 @@
   update_hard_expiration: 0,
   currentVersion: "0.0",
   notificationdata: {},
-  notificationurl: "https://notification.adblockplus.org/notification.json"
+  notificationurl: "https://notification.adblockplus.org/notification.json",
+  suppress_first_run_page: false,
+  disable_auto_updates: false
 };
 
-let values = Object.create(defaults);
+let preconfigurable = ["suppress_first_run_page", "disable_auto_updates"];

[sergei, 2015/06/19 10:07:16]

I saw the comment that `preconfigurable` is an array because it's done the same
way as in FF. However, I find it a little bit awkward because of code
duplication. Why is it better than to have `preconfigurable` object with the
default values (which are now in `defaults` object) instead of array of strings?

[Felix Dahlke, 2015/06/22 07:33:58]

You mean splitting up defaults into two objects, preconfigurable and
non-preconfigurable ones? While that would be possible, I don't really see a
downside of an explicit whitelist, we should stay consistent with the other
platforms here.

[Oleksandr, 2015/06/22 07:47:10]

I don't think this is a code duplication, really. Having all defaults listed in
one place is better for readability, IMHO. And in any case, I think we should
use the same logic we use in Firefox, so I'll leave this as is.

[sergei, 2015/06/22 09:10:49]

Let me break the original quotes to comment them and I'm answering my question
myself.
Felix Dahlke wrote:
But we should not blindly borrow suspicious approaches used on other platforms,
we should rather fix them there or explain the benefit from them in some
comment. It's because I'm asking, why it is better.

Felix Dahlke wrote:
Oleksandr wrote:
Despite it's absolutely not critical here, it merely does not look simple.
And now I see the benefit of it, using such approach we can potentially extend
few properties with attributes without involving complicated syntax for
"defaults". Additional attributes can be like "preconfigurable", "read-only",
"in-memory" (they are not saved) an so on. So, I agree, let's leave it as is.

Nevertheless, the downside is worse code quality. Here is unnecessary code
duplication where property names are hardcoded as strings in the second place
which complicates the maintainability as well as it complicates the
understanding of the code by causing additional questions, for example, what
happens if we specify pre-configurable setting without having any default value
for it?

[Eric, 2015/06/25 17:44:40]

It is bad practice not to specify default values for these settings. Don't do
that.

It would be useful to have unit tests to ensure that they all do have default
values, but I'm not going to insist on it now.

+
+let values;
 let path = _fileSystem.resolve("prefs.json");
 let listeners = [];
 let isDirty = false;
@@ -131,7 +135,16 @@
   },
 };
 
+// Update the default prefs with what was preconfigured
+for (let key in _preconfiguredPrefs)
+  if (preconfigurable.indexOf(key) != -1)
+    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();