Response to @philc's comments regarding sync.

6 files changed, 97 insertions, 129 deletions

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index dc853803..5147aec2 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -597,5 +597,5 @@ 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 synchrized storage.
+# Start pulling changes from synchronized storage.
 Sync.init()
diff --git a/background_scripts/settings.coffee b/background_scripts/settings.coffee
index f75c1db3..73a7a04b 100644
--- a/background_scripts/settings.coffee
+++ b/background_scripts/settings.coffee
@@ -23,10 +23,9 @@ root.Settings = Settings =
 
   has: (key) -> key of localStorage
 
-  # postUpdateHooks are called each time an option changes:
-  #   either from options/options.coffee          (when the options page is saved)
-  #       or from background_scripts/sync.coffee  (when an update propagates from chrome.storage)
-  #
+  # for settings which require action when their value changes, add hooks here
+  # called from options/options.coffee (when the options page is saved), and from background_scripts/sync.coffee (when
+  # an update propagates from chrome.storage.sync).
   postUpdateHooks:
     keyMappings: (value) ->
       root.Commands.clearKeyMappingsAndSetDefaults()
@@ -34,10 +33,8 @@ root.Settings = Settings =
       root.refreshCompletionKeysAfterMappingSave()
 
   # postUpdateHooks convenience wrapper
-  doPostUpdateHook: (key, value) ->
-    if @postUpdateHooks[key]
-      @postUpdateHooks[key] value
-
+  performPostUpdateHook: (key, value) ->
+    @postUpdateHooks[key] value if @postUpdateHooks[key]
 
   # options/options.(coffee|html) only handle booleans and strings; therefore
   # all defaults must be booleans or strings
diff --git a/background_scripts/sync.coffee b/background_scripts/sync.coffee
index 1c6c7fb6..56c74b81 100644
--- a/background_scripts/sync.coffee
+++ b/background_scripts/sync.coffee
@@ -1,12 +1,12 @@
 #
 # * Sync.set() and Sync.clear() propagate local changes to chrome.storage.sync.
-# * Sync.listener() listens for changes to chrome.storage.sync and propagates those
+# * Sync.handleStorageUpdate() listens for changes to chrome.storage.sync and propagates those
 #   changes to localStorage and into vimium's internal state.
-# * Sync.pull() polls chrome.storage.sync at startup, similarly propagating
+# * Sync.fetchAsync() polls chrome.storage.sync at startup, similarly propagating
 #   changes to localStorage and into vimium's internal state.
 #
 # Changes are propagated into vimium's state using the same mechanism
-# (Settings.doPostUpdateHook) that is used when options are changed on
+# (Settings.performPostUpdateHook) that is used when options are changed on
 # the options page.
 #
 # The effect is best-effort synchronization of vimium options/settings between
@@ -26,40 +26,38 @@ root.Sync = Sync =
   # However, if users have problems, they are unlikely to notice and make sense of console logs on
   # background pages.  So disable it, by default.
   # For genuine errors, we call console.log directly.
-  debug: false
+  debug: true
   storage: chrome.storage.sync
   doNotSync: [ "settingsVersion", "previousVersion" ]
 
-  # This is called in main().
+  # This is called in main.coffee.
   init: ->
-    chrome.storage.onChanged.addListener (changes, area) -> Sync.listener changes, area
-    @pull()
+    chrome.storage.onChanged.addListener (changes, area) -> Sync.handleStorageUpdate changes, area
+    @fetchAsync()
 
   # Asynchronous fetch from synced storage, called only at startup.
-  pull: ->
+  fetchAsync: ->
     @storage.get null, (items) =>
       # Chrome sets chrome.runtime.lastError if there is an error.
       if chrome.runtime.lastError is undefined
         for own key, value of items
-          @log "pull: #{key} <- #{value}"
+          @log "fetchAsync: #{key} <- #{value}"
           @storeAndPropagate key, value
       else
-        console.log "callback for Sync.pull() indicates error"
+        console.log "callback for Sync.fetchAsync() indicates error"
         console.log chrome.runtime.lastError
 
   # Asynchronous message from synced storage.
-  listener: (changes, area) ->
+  handleStorageUpdate: (changes, area) ->
     for own key, change of changes
-      @log "listener: #{key} <- #{change.newValue}"
+      @log "handleStorageUpdate: #{key} <- #{change.newValue}"
       @storeAndPropagate key, change?.newValue
   
-  # Only ever called from asynchronous synced-storage callbacks (pull and listener).
+  # Only ever called from asynchronous synced-storage callbacks (fetchAsync and handleStorageUpdate).
   storeAndPropagate: (key, value) ->
     return if not key of Settings.defaults
-    return if not @isSyncKey key
+    return if not @shouldSyncKey key
     return if value and key of localStorage and localStorage[key] is value
-
-    # Ok: store and propagate this update.
     defaultValue = Settings.defaults[key]
     defaultValueJSON = JSON.stringify(defaultValue)
 
@@ -67,20 +65,22 @@ root.Sync = Sync =
       # Key/value has been changed to non-default value at remote instance.
       @log "storeAndPropagate update: #{key}=#{value}"
       localStorage[key] = value
-      Settings.doPostUpdateHook key, JSON.parse(value)
+      Settings.performPostUpdateHook key, JSON.parse(value)
     else
       # Key has been reset to default value at remote instance.
       @log "storeAndPropagate clear: #{key}"
       if key of localStorage
         delete localStorage[key]
-      Settings.doPostUpdateHook key, defaultValue
+      Settings.performPostUpdateHook key, defaultValue
 
   # 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 @isSyncKey key
+    if @shouldSyncKey key
       @log "set scheduled: #{key}=#{value}"
-      @storage.set @mkKeyValue(key,value), =>
+      key_value = {}
+      key_value[key] = value
+      @storage.set key_value, =>
         # Chrome sets chrome.runtime.lastError if there is an error.
         if chrome.runtime.lastError
           console.log "callback for Sync.set() indicates error: #{key} <- #{value}"
@@ -88,7 +88,7 @@ root.Sync = Sync =
 
   # Only called synchronously from within vimium, never on a callback.
   clear: (key) ->
-    if @isSyncKey key
+    if @shouldSyncKey key
       @log "clear scheduled: #{key}"
       @storage.remove key, =>
         # Chrome sets chrome.runtime.lastError if there is an error.
@@ -97,15 +97,9 @@ root.Sync = Sync =
           console.log chrome.runtime.lastError
 
   # Should we synchronize this key?
-  isSyncKey: (key) ->
+  shouldSyncKey: (key) ->
     key not in @doNotSync
 
-  # There has to be a more elegant way to do this!
-  mkKeyValue: (key, value) ->
-    obj = {}
-    obj[key] = value
-    obj
-
   log: (msg) ->
     console.log "Sync: #{msg}" if @debug
 
diff --git a/pages/options.coffee b/pages/options.coffee
index 0aeb8e2d..34696f68 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -67,7 +67,7 @@ saveOptions = ->
       bgSettings.set fieldName, fieldValue
     $(fieldName).value = fieldValue
     $(fieldName).setAttribute "savedValue", fieldValue
-    bgSettings.doPostUpdateHook fieldName, fieldValue
+    bgSettings.performPostUpdateHook fieldName, fieldValue
 
   $("saveOptions").disabled = true
 
diff --git a/tests/unit_tests/settings_test.coffee b/tests/unit_tests/settings_test.coffee
index dd2815f0..25bb3628 100644
--- a/tests/unit_tests/settings_test.coffee
+++ b/tests/unit_tests/settings_test.coffee
@@ -13,7 +13,7 @@ context "settings",
   setup ->
     stub global, 'localStorage', {}
 
-  should "all settings stored in localStorage must be JSONified strings", ->
+  should "save settings in localStorage as JSONified strings", ->
     Settings.set 'dummy', ""
     assert.equal localStorage.dummy, '""'
 
@@ -36,58 +36,39 @@ context "settings",
     Settings.clear 'scrollStepSize'
     assert.equal Settings.get('scrollStepSize'), 60
 
-  should "remote changes take effect locally, non-default value", ->
+  should "propagate non-default value via synced storage listener", ->
     Settings.set 'scrollStepSize', 20
     assert.equal Settings.get('scrollStepSize'), 20
-    Sync.listener { scrollStepSize: { newValue: "40" } }
+    Sync.handleStorageUpdate { scrollStepSize: { newValue: "40" } }
     assert.equal Settings.get('scrollStepSize'), 40
 
-  should "remote changes take effect locally, default value", ->
+  should "propagate default value via synced storage listener", ->
     Settings.set 'scrollStepSize', 20
     assert.equal Settings.get('scrollStepSize'), 20
-    Sync.listener { scrollStepSize: { newValue: "60" } }
+    Sync.handleStorageUpdate { scrollStepSize: { newValue: "60" } }
     assert.isFalse Settings.has 'scrollStepSize'
 
-  should "remote changes are propagated, non-default value", ->
-    # Prime Sync.
-    Settings.set 'scrollStepSize', 20
-    assert.equal Settings.get('scrollStepSize'), 20
-    # Set a bogus value in localStorage, bypassing Settings and Sync.
-    localStorage['scrollStepSize'] = JSON.stringify(10)
-    assert.equal Settings.get('scrollStepSize'), 10
-    # Pull Sync's version of scrollStepSize, this should reset it to the correct value (20).
-    Sync.pull()
+  should "propagate non-default values from synced storage", ->
+    chrome.storage.sync.set { scrollStepSize: JSON.stringify(20) }
+    Sync.fetchAsync()
     assert.equal Settings.get('scrollStepSize'), 20
 
-  should "remote changes are propagated, default value", ->
-    # Prime Sync with a default value.
+  should "propagate default values from synced storage", ->
+    Settings.set 'scrollStepSize', 20
     chrome.storage.sync.set { scrollStepSize: JSON.stringify(60) }
-    assert.isFalse Settings.has 'scrollStepSize'
-    # Set a bogus value in localStorage, bypassing Settings and Sync.
-    localStorage['scrollStepSize'] = JSON.stringify(10)
-    assert.equal Settings.get('scrollStepSize'), 10
-    # Pull Sync's version of scrollStepSize, this should delete scrollStepSize in localStorage, because it's a default value.
-    Sync.pull()
+    Sync.fetchAsync()
     assert.isFalse Settings.has 'scrollStepSize'
 
-  should "remote setting cleared", ->
-    # Prime localStorage.
+  should "clear a setting from synced storage", ->
     Settings.set 'scrollStepSize', 20
-    assert.equal Settings.get('scrollStepSize'), 20
-    # Prime Sync with a non-default value.
-    chrome.storage.sync.set { scrollStepSize: JSON.stringify(40) }
     chrome.storage.sync.remove 'scrollStepSize'
     assert.isFalse Settings.has 'scrollStepSize'
 
   should "trigger a postUpdateHook", ->
     message = "Hello World"
-    # Install a bogus update hook for an existing setting.
     Settings.postUpdateHooks['scrollStepSize'] = (value) -> Sync.message = value
     chrome.storage.sync.set { scrollStepSize: JSON.stringify(message) }
-    # Was the update hook triggered?
     assert.equal message, Sync.message
 
-  should "sync a key which is not a setting", ->
-    # There is nothing to test, here.  It's purpose is just to ensure that, should additional settings be
-    # added in future, then the Sync won't cause unexpected crashes.
+  should "sync a key which is not a known setting (without crashing)", ->
     chrome.storage.sync.set { notASetting: JSON.stringify("notAUsefullValue") }
diff --git a/tests/unit_tests/test_chrome_stubs.coffee b/tests/unit_tests/test_chrome_stubs.coffee
index dd8dccf8..e9c48f31 100644
--- a/tests/unit_tests/test_chrome_stubs.coffee
+++ b/tests/unit_tests/test_chrome_stubs.coffee
@@ -1,65 +1,61 @@
-global.chrome ||= {}
-global.runtime ||= {}
-global.chrome.storage ||= {}
 
 #
 # This is a stub for chrome.strorage.sync for testing.
 # It does what chrome.storage.sync should do (roughly), but does so synchronously.
 #
 
-global.chrome.storage.onChanged ||=
-  addListener: (func) -> @func = func
-
-  # Fake a callback from chrome.storage.sync.
-  call: (key,value) ->
-    chrome.runtime = { lastError: undefined }
-    if @func
-      @func( @mkKeyValue(key,value), 'synced storage stub' )
-
-  callEmpty: (key) ->
-    chrome.runtime = { lastError: undefined }
-    if @func
-      items = {}
-      items[key] = {}
-      @func( items, 'synced storage stub' )
-
-  mkKeyValue: (key, value) ->
-    obj = {}
-    obj[key] = { newValue: value }
-    obj
-
-global.chrome.storage.sync ||=
-  store: {}
-
-  set: (items,callback) ->
-    chrome.runtime = { lastError: undefined }
-    for own key, value of items
-      @store[key] = value
-    if callback
-      callback()
-    # Now, generate (supposedly asynchronous) notifications for listeners.
-    for own key, value of items
-      global.chrome.storage.onChanged.call(key,value)
-
-  get: (keys,callback) ->
-    chrome.runtime = { lastError: undefined }
-    if keys == null
-      keys = []
-      for own key, value of @store
-        keys.push key
-    items = {}
-    for key in keys
-      items[key] = @store[key]
-    # Now, generate (supposedly asynchronous) callback
-    if callback
-      callback items
-
-  remove: (key,callback) ->
-    chrome.runtime = { lastError: undefined }
-    if key of @store
-      delete @store[key]
-    if callback
-      callback()
-    # Now, generate (supposedly asynchronous) notification for listeners.
-    global.chrome.storage.onChanged.callEmpty(key)
-
+global.chrome =
+  runtime: {}
+
+  storage:
+
+    # chrome.storage.onChanged
+    onChanged:
+      addListener: (func) -> @func = func
+
+      # Fake a callback from chrome.storage.sync.
+      call: (key, value) ->
+        chrome.runtime = { lastError: undefined }
+        key_value = {}
+        key_value[key] = { newValue: value }
+        @func(key_value,'synced storage stub') if @func
+
+      callEmpty: (key) ->
+        chrome.runtime = { lastError: undefined }
+        if @func
+          items = {}
+          items[key] = {}
+          @func(items,'synced storage stub')
+
+    # chrome.storage.sync
+    sync:
+      store: {}
+
+      set: (items, callback) ->
+        chrome.runtime = { lastError: undefined }
+        for own key, value of items
+          @store[key] = value
+        callback() if callback
+        # Now, generate (supposedly asynchronous) notifications for listeners.
+        for own key, value of items
+          global.chrome.storage.onChanged.call(key,value)
+
+      get: (keys, callback) ->
+        chrome.runtime = { lastError: undefined }
+        if keys == null
+          keys = []
+          for own key, value of @store
+            keys.push key
+        items = {}
+        for key in keys
+          items[key] = @store[key]
+        # Now, generate (supposedly asynchronous) callback
+        callback items if callback
+
+      remove: (key, callback) ->
+        chrome. runtime = { lastError: undefined }
+        if key of @store
+          delete @store[key]
+        callback() if callback
+        # Now, generate (supposedly asynchronous) notification for listeners.
+        global.chrome.storage.onChanged.callEmpty(key)