Re-work unified settings.

This is a minor re-working of #1705 from @mrmr1993.

The main changes are:
   - Simplify initialisation logic.
   - Always initialise Settings immediately and automatically (ie. don't
     initialise Settings separately and manually in the background,
     content scripts, options and tests).
   - Get rid of addEventListener (it's only being used for on "load").
   - Add Settings.use() in its place.

9 files changed, 44 insertions, 82 deletions

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index e40b03be..980f8e18 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -728,6 +728,4 @@ chrome.windows.getAll { populate: true }, (windows) ->
         (response) -> updateScrollPosition(tab, response.scrollX, response.scrollY) if response?
       chrome.tabs.sendMessage(tab.id, { name: "getScrollPosition" }, createScrollPositionHandler())
 
-# Start pulling changes from synchronized storage.
-Settings.init()
 showUpgradeMessage()
diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index daf738a3..0ea40bd3 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -26,7 +26,7 @@ LinkHints =
   linkActivator: undefined
   # While in delayMode, all keypresses have no effect.
   delayMode: false
-  # Handle the link hinting marker generation and matching. Must be initialized after settings have been
+  # Handle the link hinting marker generation and matching. Must be initialized after Settings have been
   # loaded, so that we can retrieve the option setting.
   getMarkerMatcher: ->
     if Settings.get("filterLinkHints") then filterHints else alphabetHints
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 47ee9e68..bb1e971f 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -41,25 +41,6 @@ textInputXPath = (->
   DomUtils.makeXPath(inputElements)
 )()
 
-# NOTE(mrmr1993): we use Settings everywhere instead of the dedicated implementation that was here.
-# Previously, only the values listed below would be loaded. If the space used by settings across all of our
-# content scripts is becoming an issue, then we can restrict the values we load to the list below.
-settings =
-  values:
-    scrollStepSize: null
-    linkHintCharacters: null
-    linkHintNumbers: null
-    filterLinkHints: null
-    hideHud: null
-    previousPatterns: null
-    nextPatterns: null
-    regexFindMode: null
-    userDefinedLinkHintCss: null
-    helpDialog_showAdvancedCommands: null
-    smoothScroll: null
-    grabBackFocus: null
-    searchEngines: null
-
 #
 # Give this frame a unique (non-zero) id.
 #
@@ -82,15 +63,15 @@ class GrabBackFocus extends Mode
       _name: "grab-back-focus-mousedown"
       mousedown: => @alwaysContinueBubbling => @exit()
 
-    activate = =>
-      return @exit() unless Settings.get "grabBackFocus"
-      @push
-        _name: "grab-back-focus-focus"
-        focus: (event) => @grabBackFocus event.target
-      # An input may already be focused. If so, grab back the focus.
-      @grabBackFocus document.activeElement if document.activeElement
-
-    if Settings.isLoaded then activate() else Settings.addEventListener "load", activate
+    Settings.use "grabBackFocus", (grabBackFocus) =>
+      if grabBackFocus
+        @push
+          _name: "grab-back-focus-focus"
+          focus: (event) => @grabBackFocus event.target
+        # An input may already be focused. If so, grab back the focus.
+        @grabBackFocus document.activeElement if document.activeElement
+      else
+        @exit()
 
   grabBackFocus: (element) ->
     return @continueBubbling unless DomUtils.isEditable element
@@ -145,8 +126,7 @@ window.initializeModes = ->
 # Complete initialization work that sould be done prior to DOMReady.
 #
 initializePreDomReady = ->
-  Settings.addEventListener "load", LinkHints.init.bind LinkHints
-  Settings.init()
+  Settings.use "theKeyHereDoesNotMatter", LinkHints.init.bind LinkHints
 
   initializeModes()
   checkIfEnabledForUrl()
@@ -224,7 +204,6 @@ window.installListeners = ->
 
 #
 # Whenever we get the focus:
-# - Reload settings (they may have changed).
 # - Tell the background page this frame's URL.
 # - Check if we should be enabled.
 #
@@ -1146,7 +1125,6 @@ window.onbeforeunload = ->
     scrollY: window.scrollY)
 
 root = exports ? window
-root.settings = settings
 root.handlerStack = handlerStack
 root.frameId = frameId
 root.windowIsFocused = windowIsFocused
diff --git a/lib/settings.coffee b/lib/settings.coffee
index 5bbc9719..87fefd95 100644
--- a/lib/settings.coffee
+++ b/lib/settings.coffee
@@ -2,8 +2,6 @@
 # * Sync.set() and Sync.clear() propagate local changes to chrome.storage.sync.
 # * Sync.handleStorageUpdate() listens for changes to chrome.storage.sync and propagates those
 #   changes to localStorage and into vimium's internal state.
-# * Sync.fetchAsync() polls chrome.storage.sync at startup, similarly propagating
-#   changes to localStorage and into vimium's internal state.
 #
 # The effect is best-effort synchronization of vimium options/settings between
 # chrome/vimium instances.
@@ -13,26 +11,19 @@
 #   they're always non-empty strings.
 #
 
-root = exports ? window
 Sync =
-
   storage: chrome.storage.sync
   doNotSync: ["settingsVersion", "previousVersion"]
 
-  # This is called in main.coffee.
-  init: ->
-    chrome.storage.onChanged.addListener (changes, area) -> Sync.handleStorageUpdate changes, area
-    @fetchAsync()
-
-  # Asynchronous fetch from synced storage, called only at startup.
-  fetchAsync: ->
+  init: (onReady) ->
+    chrome.storage.onChanged.addListener (changes, area) => @handleStorageUpdate changes, area
     @storage.get null, (items) =>
       unless chrome.runtime.lastError
         for own key, value of items
           Settings.storeAndPropagate key, value if @shouldSyncKey key
-        Settings.isLoaded = true
-        unless isPreloaded
-          listener() while listener = Settings.eventListeners.load?.pop()
+      # We call onReady() even if @storage.get() fails; otherwise, the initialization of Settings never
+      # completes.
+      onReady?()
 
   # Asynchronous message from synced storage.
   handleStorageUpdate: (changes, area) ->
@@ -53,31 +44,24 @@ Sync =
   # Should we synchronize this key?
   shouldSyncKey: (key) -> key not in @doNotSync
 
-#
-# Used by all parts of Vimium to manipulate localStorage.
-#
-
-# Select the object to use as the cache for settings.
-if Utils.isExtensionPage()
-  if Utils.isBackgroundPage()
-    settingsCache = localStorage
-    isPreloaded = true
-  else
-    settingsCache = extend {}, localStorage # Make a copy of the cached settings from localStorage
-    isPreloaded = true
-else
-  settingsCache = {}
-  isPreloaded = false
-
-root.Settings = Settings =
-  isLoaded: isPreloaded
-  cache: settingsCache
-  eventListeners: {}
+Settings =
+  isLoaded: false
+  cache: {}
+  onLoadedListeners: []
 
   init: ->
-    Sync.init()
-    if isPreloaded
-      listener() while listener = Settings.eventListeners.load?.pop()
+    # On extension pages, we use localStorage (or a copy of it) as the cache.
+    if Utils.isExtensionPage()
+      @cache = if Utils.isBackgroundPage() then localStorage else extend {}, localStorage
+      @postInit()
+
+    Sync.init => @postInit()
+
+  postInit: ->
+    wasLoaded = @isLoaded
+    @isLoaded = true
+    unless wasLoaded
+      listener() while listener = @onLoadedListeners.pop()
 
   get: (key) ->
     console.log "WARNING: Settings have not loaded yet; using the default value for #{key}." unless @isLoaded
@@ -99,8 +83,9 @@ root.Settings = Settings =
 
   has: (key) -> key of @cache
 
-  addEventListener: (eventName, callback) ->
-    (@eventListeners[eventName] ||= []).push callback
+  use: (key, callback) ->
+    callCallback = => callback @get key
+    if @isLoaded then callCallback() else @onLoadedListeners.push => callCallback
 
   # For settings which require action when their value changes, add hooks to this object, to be called from
   # options/options.coffee (when the options page is saved), and by Settings.storeAndPropagate (when an
@@ -111,7 +96,7 @@ root.Settings = Settings =
   performPostUpdateHook: (key, value) ->
     @postUpdateHooks[key]? value
 
-  # Only ever called from asynchronous synced-storage callbacks (fetchAsync and handleStorageUpdate).
+  # Only ever called from asynchronous synced-storage callbacks (on start up and handleStorageUpdate).
   storeAndPropagate: (key, value) ->
     return unless key of @defaults
     return if value and key of @cache and @cache[key] is value
@@ -200,8 +185,7 @@ root.Settings = Settings =
     settingsVersion: Utils.getCurrentVersion()
     helpDialog_showAdvancedCommands: false
 
-# Export Sync via Settings for tests.
-root.Settings.Sync = Sync
+Settings.init()
 
 # Perform migration from old settings versions, if this is the background page.
 if Utils.isBackgroundPage()
@@ -218,3 +202,9 @@ if Utils.isBackgroundPage()
     unless chrome.runtime.lastError or items.findModeRawQueryList
       rawQuery = Settings.get "findModeRawQuery"
       chrome.storage.local.set findModeRawQueryList: (if rawQuery then [ rawQuery ] else [])
+
+root = exports ? window
+root.Settings = Settings
+
+# Export Sync via Settings for tests.
+root.Settings.Sync = Sync
diff --git a/pages/options.coffee b/pages/options.coffee
index c8c21850..ddab2bf4 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -1,6 +1,5 @@
 
 $ = (id) -> document.getElementById id
-Settings.init()
 bgExclusions = chrome.extension.getBackgroundPage().Exclusions
 
 #
diff --git a/tests/dom_tests/chrome.coffee b/tests/dom_tests/chrome.coffee
index 4de85876..d4e6930d 100644
--- a/tests/dom_tests/chrome.coffee
+++ b/tests/dom_tests/chrome.coffee
@@ -28,7 +28,7 @@ root.chrome =
       get: ->
       set: ->
     sync:
-      get: ->
+      get: (_, callback) -> callback? {}
       set: ->
     onChanged:
       addListener: ->
diff --git a/tests/unit_tests/exclusion_test.coffee b/tests/unit_tests/exclusion_test.coffee
index 28c17a2f..0e4b87bc 100644
--- a/tests/unit_tests/exclusion_test.coffee
+++ b/tests/unit_tests/exclusion_test.coffee
@@ -15,7 +15,6 @@ root.Marks =
 extend(global, require "../../lib/utils.js")
 Utils.getCurrentVersion = -> '1.44'
 extend(global,require "../../lib/settings.js")
-Settings.init()
 extend(global, require "../../background_scripts/exclusions.js")
 extend(global, require "../../background_scripts/commands.js")
 extend(global, require "../../background_scripts/main.js")
diff --git a/tests/unit_tests/settings_test.coffee b/tests/unit_tests/settings_test.coffee
index ded7b5f8..a2aca6fd 100644
--- a/tests/unit_tests/settings_test.coffee
+++ b/tests/unit_tests/settings_test.coffee
@@ -14,7 +14,6 @@ context "settings",
     stub global, 'localStorage', {}
     Settings.cache = global.localStorage # Point the settings cache to the new localStorage object.
     Settings.postUpdateHooks = {} # Avoid running update hooks which include calls to outside of settings.
-    Settings.init()
 
   should "save settings in localStorage as JSONified strings", ->
     Settings.set 'dummy', ""
diff --git a/tests/unit_tests/utils_test.coffee b/tests/unit_tests/utils_test.coffee
index f9ed3636..67c3b333 100644
--- a/tests/unit_tests/utils_test.coffee
+++ b/tests/unit_tests/utils_test.coffee
@@ -3,7 +3,6 @@ extend global, require "./test_chrome_stubs.js"
 extend(global, require "../../lib/utils.js")
 Utils.getCurrentVersion = -> '1.43'
 extend(global, require "../../lib/settings.js")
-Settings.init()
 
 context "isUrl",
   should "accept valid URLs", ->