Refactor and eliminate Sync object.

3 files changed, 65 insertions, 99 deletions

diff --git a/lib/settings.coffee b/lib/settings.coffee
index 040c1697..ee46c0b1 100644
--- a/lib/settings.coffee
+++ b/lib/settings.coffee
@@ -1,121 +1,83 @@
-#
-# * 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.
-#
-# The effect is best-effort synchronization of vimium options/settings between
-# chrome/vimium instances.
-#
-# NOTE:
-#   Values handled within this module are ALWAYS already JSON.stringifed, so
-#   they're always non-empty strings.
-#
-
-Sync =
-  storage: chrome.storage.sync
-  doNotSync: ["settingsVersion", "previousVersion"]
-
-  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
-      # We call onReady() even if @storage.get() fails; otherwise, the initialization of Settings never
-      # completes.
-      onReady?()
-
-  # Asynchronous message from synced storage.
-  handleStorageUpdate: (changes, area) ->
-    if area == "sync"
-      for own key, change of changes
-        Settings.storeAndPropagate key, change?.newValue if @shouldSyncKey key
-
-  # Only called synchronously from within vimium, never on a callback.
-  # No need to propagate updates to the rest of vimium, that's already been done.
-  set: (key, value) ->
-    if @shouldSyncKey key
-      setting = {}; setting[key] = value
-      @storage.set setting
-
-  # Only called synchronously from within vimium, never on a callback.
-  clear: (key) ->
-    @storage.remove key if @shouldSyncKey key
-
-  # Should we synchronize this key?
-  shouldSyncKey: (key) -> key not in @doNotSync
 
 Settings =
-  isLoaded: false
+  storage: chrome.storage.sync
   cache: {}
-  onLoadedListeners: []
+  isLoaded: false
+  onLoadedCallbacks: []
 
   init: ->
-    # On extension pages, we use localStorage (or a copy of it) as the cache.
     if Utils.isExtensionPage()
+      # On extension pages, we use localStorage (or a copy of it) as the cache.
       @cache = if Utils.isBackgroundPage() then localStorage else extend {}, localStorage
-      @postInit()
+      @onLoaded()
 
-    Sync.init => @postInit()
+    @storage.get null, (items) =>
+      @propagateChangesFromChromeStorage items unless chrome.runtime.lastError
+
+      chrome.storage.onChanged.addListener (changes, area) =>
+        @propagateChangesFromChromeStorage changes if area == "sync"
+
+      @onLoaded()
 
-  postInit: ->
-    wasLoaded = @isLoaded
+  # Called after @cache has been initialized.  On extension pages, this will be called twice, but that does
+  # not matter because it's idempotent.
+  onLoaded: ->
     @isLoaded = true
-    unless wasLoaded
-      listener() while listener = @onLoadedListeners.pop()
+    callback() while callback = @onLoadedCallbacks.pop()
+
+  shouldSyncKey: (key) ->
+    (key of @defaults) and key not in [ "settingsVersion", "previousVersion" ]
+
+  propagateChangesFromChromeStorage: (changes) ->
+    @handleUpdateFromChromeStorage key, change?.newValue for own key, change of changes
+
+  handleUpdateFromChromeStorage: (key, value) ->
+    # Note: value here is either null or a JSONified string.  Therefore, even falsy settings values (like
+    # false, 0 or "") are truthy here.  Only null is falsy.
+    if @shouldSyncKey key
+      unless value and key of @cache and @cache[key] == value
+        defaultValue = @defaults[key]
+        defaultValueJSON = JSON.stringify defaultValue
+
+        if value and value != defaultValueJSON
+          # Key/value has been changed to a non-default value.
+          @cache[key] = value
+          @performPostUpdateHook key, JSON.parse value
+        else
+          # The key has been reset to its default value.
+          delete @cache[key] if key of @cache
+          @performPostUpdateHook key, defaultValue
 
   get: (key) ->
     console.log "WARNING: Settings have not loaded yet; using the default value for #{key}." unless @isLoaded
-    if key of @cache and @cache[key]? then JSON.parse(@cache[key]) else @defaults[key]
+    if key of @cache and @cache[key]? then JSON.parse @cache[key] else @defaults[key]
 
   set: (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 == @defaults[key])
-      @clear(key)
+    # Don't store the value if it is equal to the default, so we can change the defaults in the future.
+    if value == @defaults[key]
+      @clear key
     else
       jsonValue = JSON.stringify value
       @cache[key] = jsonValue
-      Sync.set key, jsonValue
+      if @shouldSyncKey key
+        setting = {}; setting[key] = jsonValue
+        @storage.set setting
 
   clear: (key) ->
-    if @has key
-      delete @cache[key]
-    Sync.clear key
+    delete @cache[key] if @has key
+    @storage.remove key if @shouldSyncKey key
 
   has: (key) -> key of @cache
 
   use: (key, callback) ->
-    callCallback = => callback @get key
-    if @isLoaded then callCallback() else @onLoadedListeners.push => callCallback
+    invokeCallback = => callback @get key
+    if @isLoaded then invokeCallback() else @onLoadedCallbacks.push invokeCallback
 
-  # 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
-  # update propagates from chrome.storage.sync).
+  # For settings which require action when their value changes, add hooks to this object.
   postUpdateHooks: {}
+  performPostUpdateHook: (key, value) -> @postUpdateHooks[key]? value
 
-  # postUpdateHooks convenience wrapper
-  performPostUpdateHook: (key, value) ->
-    @postUpdateHooks[key]? value
-
-  # 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
-    defaultValue = @defaults[key]
-    defaultValueJSON = JSON.stringify(defaultValue)
-
-    if value and value != defaultValueJSON
-      # Key/value has been changed to non-default value at remote instance.
-      @cache[key] = value
-      @performPostUpdateHook key, JSON.parse(value)
-    else
-      # Key has been reset to default value at remote instance.
-      if key of @cache
-        delete @cache[key]
-      @performPostUpdateHook key, defaultValue
-
-  # options.coffee and options.html only handle booleans and strings; therefore all defaults must be booleans
-  # or strings
+  # Default values for all settings.
   defaults:
     scrollStepSize: 60
     smoothScroll: true
@@ -147,7 +109,7 @@ Settings =
     exclusionRules:
       [
         # Disable Vimium on Gmail.
-        { pattern: "http*://mail.google.com/*", passKeys: "" }
+        { pattern: "https?://mail.google.com/*", passKeys: "" }
       ]
 
     # NOTE: If a page contains both a single angle-bracket link and a double angle-bracket link, then in
@@ -206,6 +168,3 @@ if Utils.isBackgroundPage()
 
 root = exports ? window
 root.Settings = Settings
-
-# Export Sync via Settings for tests.
-root.Settings.Sync = Sync
diff --git a/tests/unit_tests/settings_test.coffee b/tests/unit_tests/settings_test.coffee
index a2aca6fd..08145190 100644
--- a/tests/unit_tests/settings_test.coffee
+++ b/tests/unit_tests/settings_test.coffee
@@ -38,16 +38,23 @@ context "settings",
     Settings.clear 'scrollStepSize'
     assert.equal Settings.get('scrollStepSize'), 60
 
+context "synced settings",
+
+  setup ->
+    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.
+
   should "propagate non-default value via synced storage listener", ->
     Settings.set 'scrollStepSize', 20
     assert.equal Settings.get('scrollStepSize'), 20
-    Settings.Sync.handleStorageUpdate { scrollStepSize: { newValue: "40" } }
+    Settings.propagateChangesFromChromeStorage { scrollStepSize: { newValue: "40" } }
     assert.equal Settings.get('scrollStepSize'), 40
 
   should "propagate default value via synced storage listener", ->
     Settings.set 'scrollStepSize', 20
     assert.equal Settings.get('scrollStepSize'), 20
-    Settings.Sync.handleStorageUpdate { scrollStepSize: { newValue: "60" } }
+    Settings.propagateChangesFromChromeStorage { scrollStepSize: { newValue: "60" } }
     assert.isFalse Settings.has 'scrollStepSize'
 
   should "propagate non-default values from synced storage", ->
diff --git a/tests/unit_tests/test_chrome_stubs.coffee b/tests/unit_tests/test_chrome_stubs.coffee
index 16f0e144..fe2fc298 100644
--- a/tests/unit_tests/test_chrome_stubs.coffee
+++ b/tests/unit_tests/test_chrome_stubs.coffee
@@ -70,14 +70,14 @@ exports.chrome =
         chrome.runtime.lastError = undefined
         key_value = {}
         key_value[key] = { newValue: value }
-        @func(key_value,'synced storage stub') if @func
+        @func(key_value,'sync') if @func
 
       callEmpty: (key) ->
         chrome.runtime.lastError = undefined
         if @func
           items = {}
           items[key] = {}
-          @func(items,'synced storage stub')
+          @func(items,'sync')
 
     session:
       MAX_SESSION_RESULTS: 25