Issue 2040 - Replaced localStorage with chrome.storage.local

lib/prefs.js

Index: lib/prefs.js
===================================================================
--- a/lib/prefs.js
+++ b/lib/prefs.js
@@ -15,78 +15,11 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-//
-// The values are hardcoded for now.
-//
+const keyPrefix = "pref:";
 
-let defaults = Object.create(null);
-defaults.enabled = true;
-defaults.data_directory = "";
-defaults.patternsbackups = 5;
-defaults.patternsbackupinterval = 24;
-defaults.savestats = false;
-defaults.privateBrowsing = false;
-defaults.subscriptions_fallbackerrors = 5;
-defaults.subscriptions_fallbackurl = "https://adblockplus.org/getSubscription?version=%VERSION%&url=%SUBSCRIPTION%&downloadURL=%URL%&error=%ERROR%&channelStatus=%CHANNELSTATUS%&responseStatus=%RESPONSESTATUS%";
-defaults.subscriptions_autoupdate = true;
-defaults.subscriptions_exceptionsurl = "https://easylist-downloads.adblockplus.org/exceptionrules.txt";
-defaults.subscriptions_antiadblockurl = "https://easylist-downloads.adblockplus.org/antiadblockfilters.txt";
-defaults.documentation_link = "https://adblockplus.org/redirect?link=%LINK%&lang=%LANG%";
-defaults.notificationdata = {};
-defaults.notificationurl = "https://notification.adblockplus.org/notification.json";
-defaults.stats_total = {};
-defaults.show_statsinicon = true;
-defaults.show_statsinpopup = true;
-defaults.shouldShowBlockElementMenu = true;
-defaults.hidePlaceholders = true;
-
+let prefs = Object.create(null);
 let listeners = [];
 
-function defineProperty(key)
-{
-  let value = null;
-  Prefs.__defineGetter__(key, function()
-  {
-    if (value === null)
-    {
-      if (key in ext.storage)
-      {
-        try
-        {
-          value = JSON.parse(ext.storage[key]);
-        }
-        catch(e)
-        {
-          Cu.reportError(e);
-        }
-      }
-
-      if (value === null)
-        value = JSON.parse(JSON.stringify(defaults[key]));
-    }
-    return value;
-  });
-  Prefs.__defineSetter__(key, function(newValue)
-  {
-    if (typeof newValue != typeof defaults[key])
-      throw new Error("Attempt to change preference type");
-
-    let stringified = JSON.stringify(newValue);
-    if (stringified != JSON.stringify(defaults[key]))
-      ext.storage[key] = stringified;
-    else
-      delete ext.storage[key];
-
-    value = newValue;
-
-    for (let listener of listeners)
-      listener(key);
-
-    return value;
-  });
-}
-
-
 let Prefs = exports.Prefs = {
   addListener: function(listener)
   {
@@ -99,8 +32,100 @@
     let index = listeners.indexOf(listener);
     if (index >= 0)
       listeners.splice(index, 1);
-  },
+  }
 };
 
-for (let key in defaults)
-  defineProperty(key);
+function keyToPref(key)
+{
+  if (key.indexOf(keyPrefix) != 0)
+    return null;
+
+  return key.substr(keyPrefix.length);
+}
+
+function prefToKey(pref)
+{
+  return keyPrefix + pref;
+}
+
+function addPreference(pref, defaultValue)
+{
+  prefs[pref] = undefined;
+
+  Object.defineProperty(Prefs, pref, {
+    get: function()
+    {
+      let value = prefs[pref];
+
+      if (typeof value != "undefined")
+        return value;
+
+      return defaultValue;
+    },
+    set: function(value)
+    {
+      if (typeof value != typeof defaultValue)
+        throw new Error("Attempt to change preference type");
+
+      if (value == defaultValue)
+      {
+        prefs[pref] = undefined;
+        ext.storage.remove(prefToKey(pref));
+      }
+      else
+      {
+        prefs[pref] = value;
+        ext.storage.set(prefToKey(pref), value);
+      }
+    },
+    enumerable: true
+  });
+}
+
+function setPreference(pref, value)
+{
+  prefs[pref] = value;
+
+  for (let listener of listeners)
+    listener(pref);
+}

[Wladimir Palant, 2015/03/12 23:05:37]

The flow here is rather convoluted, a change to prefs.foo does through several
instances and prefs[pref] is set twice. The point seems to be considering
external changes (is there even a UI for that?). However, if we do that more
effort is required:

* What if an external change changes preference type? We probably want to log an
error and fall back to default.

* What if an external change stores the default value in storage? We probably
want prefs[pref] to be undefined then.

Did you find code paths that rely on Prefs.foo value changing synchronously?

[Sebastian Noack, 2015/03/12 23:35:29]

Currently there is only one case where we externally change prefs, that is the
migration code. However, in the long-term when we don't import the Prefs object
into the options page anymore, calling ext.storage.local.set will be the easiest
and only cross-platform way for users to set hidden preferences. Moreover, when
we implement chrome.storage.managed it there can be external changes we have to
respond to via chrome.storage.on changed as well. So yes we need to handle
external changes.

Do we need to check the type for those? Not for chrome.storage.managed since it
is strictly typed anyway (we have to provide a manifest listing the possible
keys and the type of their values). For the migration code it can be ignored
IMO. The prefs imported there were matter of the same type checks before. So
they should be valid unless somebody messed with localStorage directly, a case
that also wasn't handled before. For the case that some user manually calls
chrhrome.storage.set this might make sense, but when messing with the JS console
there are so many ways to screw up ABP that it isnt probably worth handling this
particular one.

+
+addPreference("enabled", true);
+addPreference("data_directory", "");
+addPreference("patternsbackups", 5);
+addPreference("patternsbackupinterval", 24);
+addPreference("savestats", false);
+addPreference("privateBrowsing", false);
+addPreference("subscriptions_fallbackerrors", 5);
+addPreference("subscriptions_fallbackurl", "https://adblockplus.org/getSubscription?version=%VERSION%&url=%SUBSCRIPTION%&downloadURL=%URL%&error=%ERROR%&channelStatus=%CHANNELSTATUS%&responseStatus=%RESPONSESTATUS%");
+addPreference("subscriptions_autoupdate", true);
+addPreference("subscriptions_exceptionsurl", "https://easylist-downloads.adblockplus.org/exceptionrules.txt");
+addPreference("subscriptions_antiadblockurl", "https://easylist-downloads.adblockplus.org/antiadblockfilters.txt");
+addPreference("documentation_link", "https://adblockplus.org/redirect?link=%LINK%&lang=%LANG%");
+addPreference("notificationdata", {});
+addPreference("notificationurl", "https://notification.adblockplus.org/notification.json");
+addPreference("stats_total", {});
+addPreference("show_statsinicon", true);
+addPreference("show_statsinpopup", true);
+addPreference("shouldShowBlockElementMenu", true);
+addPreference("hidePlaceholders", true);
+
+// Migrate preferences for users updating from old versions.
+// TODO: Remove the migration code after a few releases.
+ext.storage.migratePrefs(prefs);
+
+ext.storage.get(Object.keys(prefs).map(prefToKey), function(items)
+{
+  for (let key in items)
+    setPreference(keyToPref(key), items[key]);
+
+  ext.storage.onChanged.addListener(function(changes)
+  {
+    for (let key in changes)
+    {
+      let pref = keyToPref(key);
+      if (pref && pref in prefs)
+        setPreference(pref, changes[key].newValue);
+    }
+  });
+});