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

lib/prefs.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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 23 matching lines @@
   subscriptions_exceptionsurl: "https://easylist-downloads.adblockplus.org/exceptionrules.txt",
   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"
+  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;
 let isSaving = false;
 
 function defineProperty(key)
 {
   Object.defineProperty(Prefs, key,
   {
     get: () => values[key],
@@ skipping 66 matching lines @@
   },
 
   removeListener: function(listener)
   {
     let index = listeners.indexOf(listener);
     if (index >= 0)
       listeners.splice(index, 1);
   },
 };
 
+// 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();