Refactor settings storage and make it support empty strings.

It appears that localStorage keys with the empty string as their value
will have their values changed to undefined after a browser restart. The
DOM Inspector shows that the keys are still present, but '{{key}} in
localStorage' returns false. Convert all localStorage values to JSON as
a workaround.

This allows us to store null, numerical etc values seamlessly.

Closes #434.

Disable Vimium in the options page, due to a name collision.

3 files changed, 85 insertions, 83 deletions

diff --git a/background/settings.js b/background/settings.js
new file mode 100644
index 00000000..4792429d
--- /dev/null
+++ b/background/settings.js
@@ -0,0 +1,61 @@
+/*
+ * Used by everyone to manipulate localStorage.
+ */
+var settings = {
+
+  defaultSettings: {
+    scrollStepSize: 60,
+    linkHintCharacters: "sadfjklewcmpgh",
+    filterLinkHints: false,
+    userDefinedLinkHintCss:
+      "#vimiumHintMarkerContainer .vimiumHintMarker {" + "\n" +
+      "/* linkhint boxes */ " + "\n" +
+      "background-color: yellow;" + "\n" +
+      "border: 1px solid #E3BE23;" + "\n" +
+      "}" + "\n\n" +
+      "#vimiumHintMarkerContainer .vimiumHintMarker span {" + "\n" +
+      "/* linkhint text */ " + "\n" +
+      "color: black;" + "\n" +
+      "font-weight: bold;" + "\n" +
+      "font-size: 12px;" + "\n" +
+      "}" + "\n\n" +
+      "#vimiumHintMarkerContainer .vimiumHintMarker > .matchingCharacter {" + "\n" +
+      "}",
+    excludedUrls: "http*://mail.google.com/*\n" +
+                  "http*://www.google.com/reader/*\n",
+
+    // NOTE : If a page contains both a single angle-bracket link and a double angle-bracket link, then in
+    // most cases the single bracket link will be "prev/next page" and the double bracket link will be
+    // "first/last page", so we put the single bracket first in the pattern string so that it gets searched
+    // for first.
+
+    // "\bprev\b,\bprevious\b,\bback\b,<,←,«,≪,<<"
+    previousPatterns: "prev,previous,back,<,\u2190,\xab,\u226a,<<",
+    // "\bnext\b,\bmore\b,>,→,»,≫,>>"
+    nextPatterns: "next,more,>,\u2192,\xbb,\u226b,>>",
+  },
+
+  get: function(key) {
+    if (!(key in localStorage))
+      return this.defaultSettings[key];
+    else
+      return JSON.parse(localStorage[key]);
+  },
+
+  set: function(key, value) {
+    // don't store the value if it is equal to the default, so we can change the defaults in the future
+    if (value === this.defaultSettings[key])
+      this.clear(key);
+    else
+      localStorage[key] = JSON.stringify(value);
+  },
+
+  clear: function(key) {
+    delete localStorage[key];
+  },
+
+  has: function(key) {
+    return key in localStorage;
+  },
+
+};
diff --git a/background_page.html b/background_page.html
index 95765e2a..7f8bb2a6 100644
--- a/background_page.html
+++ b/background_page.html
@@ -3,6 +3,7 @@
 <script type="text/javascript" src="commands.js"></script>
 <script type="text/javascript" src="lib/clipboard.js"></script>
 <script type="text/javascript" src="lib/utils.js"></script>
+<script type="text/javascript" src="background/settings.js"></script>
 <script type="text/javascript" charset="utf-8">
   // Chromium #15242 will make this XHR request to access the manifest unnecessary.
   var manifestRequest = new XMLHttpRequest();
@@ -25,39 +26,6 @@
   // the string.
   var namedKeyRegex = /^(<(?:[amc]-.|(?:[amc]-)?[a-z0-9]{2,5})>)(.*)$/;
 
-  var defaultSettings = {
-    scrollStepSize: 60,
-    linkHintCharacters: "sadfjklewcmpgh",
-    filterLinkHints: false,
-    userDefinedLinkHintCss:
-      "#vimiumHintMarkerContainer .vimiumHintMarker {" + "\n" +
-      "/* linkhint boxes */ " + "\n" +
-      "background-color: yellow;" + "\n" +
-      "border: 1px solid #E3BE23;" + "\n" +
-      "}" + "\n\n" +
-      "#vimiumHintMarkerContainer .vimiumHintMarker span {" + "\n" +
-      "/* linkhint text */ " + "\n" +
-      "color: black;" + "\n" +
-      "font-weight: bold;" + "\n" + 
-      "font-size: 12px;" + "\n" +
-      "}" + "\n\n" +
-      "#vimiumHintMarkerContainer .vimiumHintMarker > .matchingCharacter {" + "\n" +
-      "}",
-
-    excludedUrls: "http*://mail.google.com/*\n" +
-                  "http*://www.google.com/reader/*\n",
-
-    // NOTE : If a page contains both a single angle-bracket link and a double angle-bracket link, then in
-    // most cases the single bracket link will be "prev/next page" and the double bracket link will be
-    // "first/last page", so we put the single bracket first in the pattern string so that it gets searched
-    // for first.
-
-    // "\bprev\b,\bprevious\b,\bback\b,<,←,«,≪,<<"
-    previousPatterns: "prev,previous,back,<,\u2190,\xab,\u226a,<<",
-    // "\bnext\b,\bmore\b,>,→,»,≫,>>"
-    nextPatterns: "next,more,>,\u2192,\xbb,\u226b,>>",
-  };
-
   // Port handler mapping
   var portHandlers = {
     keyDown:              handleKeyDown,
@@ -141,7 +109,7 @@
    */
   function isEnabledForUrl(request) {
     // excludedUrls are stored as a series of URL expressions separated by newlines.
-    var excludedUrls = getSettingFromLocalStorage("excludedUrls").split("\n");
+    var excludedUrls = settings.get("excludedUrls").split("\n");
     var isEnabled = true;
     for (var i = 0; i < excludedUrls.length; i++) {
       // The user can add "*" to the URL which means ".*"
@@ -153,7 +121,7 @@
   }
 
   function saveHelpDialogSettings(request) {
-    localStorage["helpDialog_showAdvancedCommands"] = request.showAdvancedCommands;
+    settings.set("helpDialog_showAdvancedCommands", request.showAdvancedCommands);
   }
 
   function showHelp(callback, frameId) {
@@ -179,8 +147,7 @@
                                         showUnboundCommands, showCommandNames));
     dialogHtml = dialogHtml.replace("{{version}}", currentVersion);
     dialogHtml = dialogHtml.replace("{{title}}", customTitle || "Help");
-    dialogHtml = dialogHtml.replace("{{showAdvancedCommands}}",
-        localStorage["helpDialog_showAdvancedCommands"] == "true");
+    dialogHtml = dialogHtml.replace("{{showAdvancedCommands}}", settings.get("helpDialog_showAdvancedCommands"));
     return dialogHtml;
   }
 
@@ -268,7 +235,7 @@
    * Returns the user-provided CSS overrides.
    */
   function getLinkHintCss(request) {
-    return { linkHintCss: (localStorage['userDefinedLinkHintCss'] || "") };
+    return { linkHintCss: (settings.get("userDefinedLinkHintCss") || "") };
   }
 
   /*
@@ -276,7 +243,7 @@
    * We should now dismiss that message in all tabs.
    */
   function upgradeNotificationClosed(request) {
-    localStorage.previousVersion = currentVersion;
+    settings.set("previousVersion", currentVersion);
     sendRequestToAllTabs({ name: "hideUpgradeNotification" });
   }
 
@@ -292,11 +259,11 @@
    */
   function handleSettings(args, port) {
     if (args.operation == "get") {
-      var value = getSettingFromLocalStorage(args.key);
+      var value = settings.get(args.key);
       port.postMessage({ key: args.key, value: value });
     }
     else { // operation == "set"
-      localStorage[args.key] = args.value;
+      settings.set(args.key, args.value);
     }
   }
 
@@ -306,17 +273,6 @@
     })
   }
 
-  /*
-   * Used by everyone to get settings from local storage.
-   */
-  function getSettingFromLocalStorage(setting) {
-    if (localStorage[setting] != "" && !localStorage[setting]) {
-      return defaultSettings[setting];
-    } else {
-      return localStorage[setting];
-    }
-  }
-
   function getCurrentTimeInSeconds() { Math.floor((new Date()).getTime() / 1000); }
 
   chrome.tabs.onSelectionChanged.addListener(function(tabId, selectionInfo) {
@@ -657,10 +613,10 @@
    * localStorage, and false otherwise.
    */
   function shouldShowUpgradeMessage() {
-    // Avoid showing the upgrade notification when localStorage.previousVersion is undefined, which is the
-    // case for new installs.
-    if (!localStorage.previousVersion)
-      localStorage.previousVersion = currentVersion;
+    // Avoid showing the upgrade notification when previousVersion is undefined, which is the case for new
+    // installs.
+    if (!settings.get("previousVersion"))
+      settings.set("previousVersion", currentVersion);
     return compareVersions(currentVersion, localStorage.previousVersion) == 1;
   }
 
@@ -733,20 +689,20 @@
   function init() {
     clearKeyMappingsAndSetDefaults();
 
-    if (localStorage["keyMappings"])
-      parseCustomKeyMappings(localStorage["keyMappings"]);
+    if (settings.has("keyMappings"))
+      parseCustomKeyMappings(settings.get("keyMappings"));
 
     // In version 1.22, we changed the mapping for "d" and "u" to be scroll page down/up instead of close
     // and restore tab. For existing users, we want to preserve existing behavior for them by adding some
     // custom key mappings on their behalf.
     if (localStorage.previousVersion == "1.21") {
-      var customKeyMappings = localStorage["keyMappings"] || "";
+      var customKeyMappings = settings.get("keyMappings") || "";
       if ((keyToCommandRegistry["d"] || {}).command == "scrollPageDown")
         customKeyMappings += "\nmap d removeTab";
       if ((keyToCommandRegistry["u"] || {}).command == "scrollPageUp")
         customKeyMappings += "\nmap u restoreTab";
       if (customKeyMappings != "") {
-        localStorage["keyMappings"] = customKeyMappings;
+        settings.set("keyMappings", customKeyMappings);
         parseCustomKeyMappings(customKeyMappings);
       }
     }
diff --git a/options.html b/options.html
index 3d4033e3..e4142b4d 100644
--- a/options.html
+++ b/options.html
@@ -1,11 +1,7 @@
 <html>
   <head>
     <title>Vimium Options</title>
-    <script src="lib/utils.js"></script>
-    <script src="lib/keyboardUtils.js"></script>
-    <script src="linkHints.js"></script>
-    <script src="lib/clipboard.js"></script>
-    <script src="vimiumFrontend.js"></script>
+    <script src="background/settings.js"></script>
     <style type="text/css" media="screen">
       body {
         font-family:"helvetica neue", "helvetica", "arial", "sans";
@@ -142,18 +138,12 @@
         field.value = fieldValue;
       }
 
-      var defaultFieldValue = (defaultSettings[fieldName] != null) ?
-        defaultSettings[fieldName].toString() : "";
-
-      // Don't save to storage if it's equal to the default
-      if (fieldValue == defaultFieldValue)
-        delete localStorage[fieldName];
-      // ..or if it's empty and not a field that we allow to be empty.
-      else if (!fieldValue && canBeEmptyFields.indexOf(fieldName) == -1) {
-        delete localStorage[fieldName];
-        fieldValue = defaultFieldValue;
+      // If it's empty and not a field that we allow to be empty, restore to the default value
+      if (!fieldValue && canBeEmptyFields.indexOf(fieldName) == -1) {
+        settings.clear(fieldName);
+        fieldValue = settings.get(fieldName);
       } else
-        localStorage[fieldName] = fieldValue;
+        settings.set(fieldName, fieldValue);
 
       $(fieldName).value = fieldValue;
       $(fieldName).setAttribute("savedValue", fieldValue);
@@ -166,13 +156,8 @@
   // Restores select box state to saved value from localStorage.
   function populateOptions() {
     for (var i = 0; i < editableFields.length; i++) {
-      // If it's null or undefined, let's go to the default. We want to allow empty strings in certain cases.
-      if (localStorage[editableFields[i]] != "" && !localStorage[editableFields[i]]) {
-        var val = defaultSettings[editableFields[i]] || "";
-      } else {
-	    var val = localStorage[editableFields[i]];
-      }
-	  setFieldValue($(editableFields[i]), val);
+      var val = settings.get(editableFields[i]) || "";
+      setFieldValue($(editableFields[i]), val);
     }
     onDataLoaded();
   }