Code review of Sync().

2 files changed, 78 insertions, 98 deletions

diff --git a/background_scripts/settings.coffee b/background_scripts/settings.coffee
index 64fce308..063287db 100644
--- a/background_scripts/settings.coffee
+++ b/background_scripts/settings.coffee
@@ -7,47 +7,36 @@ root.Settings = Settings =
   get: (key) ->
     if (key of localStorage) then JSON.parse(localStorage[key]) else @defaults[key]
 
-  # The doNotSync argument suppresses calls to chrome.storage.sync.* while running unit tests
   set: (key, value, doNotSync) ->
     # don't store the value if it is equal to the default, so we can change the defaults in the future
-    # warning: this test is always false for settings with numeric default values (such as scrollStepSize)
-    if ( value == @defaults[key] )
-      return @clear(key,doNotSync)
-    # don't update the key/value if it's unchanged; thereby suppressing unnecessary calls to chrome.storage
-    valueJSON = JSON.stringify value
-    if localStorage[key] == valueJSON
-      return localStorage[key]
-    # we have a new value: so update chrome.storage and localStorage
-    root.Sync.set key, valueJSON if root?.Sync?.set
-    localStorage[key] = valueJSON
+    if (value == @defaults[key])
+      @clear(key)
+    else
+      jsonified = JSON.stringify value
+      localStorage[key] = jsonified
+      root.Sync.set key, jsonified
 
-  # The doNotSync argument suppresses calls to chrome.storage.sync.* while running unit tests
-  clear: (key, doNotSync) ->
+  clear: (key) ->
     if @has key
-      root.Sync.clear key if root?.Sync?.clear
       delete localStorage[key]
+    root.Sync.clear key
 
   has: (key) -> key of localStorage
 
-  # the postUpdateHooks handler below is 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)
-  # 
-  # NOTE:
-  # this has been refactored and renamed from options.coffee(postSaveHooks):
-  #   - refactored because it is now also called from sync.coffee
-  #   - renamed because it is no longer associated only with "Save" operations
+  # 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)
   #
   postUpdateHooks:
     keyMappings: (value) ->
       root.Commands.clearKeyMappingsAndSetDefaults()
       root.Commands.parseCustomKeyMappings value
       root.refreshCompletionKeysAfterMappingSave()
-  
+
   # postUpdateHooks convenience wrapper
   doPostUpdateHook: (key, value) ->
     if @postUpdateHooks[key]
-      @postUpdateHooks[key] value 
+      @postUpdateHooks[key] value
 
 
   # options/options.(coffee|html) only handle booleans and strings; therefore
diff --git a/background_scripts/sync.coffee b/background_scripts/sync.coffee
index d3a18b85..7cb77518 100644
--- a/background_scripts/sync.coffee
+++ b/background_scripts/sync.coffee
@@ -1,30 +1,25 @@
 
 #
-# * Sync.set() and Sync.clear() propagate local changes to chrome.storage.
-# * Sync.listener() listens for changes to chrome.storage and propagates those
+# * Sync.set() and Sync.clear() propagate local changes to chrome.storage.sync.
+# * Sync.listener() 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
 #   changes to localStorage and into vimium's internal state.
-# * Sync.pull() polls chrome.storage at startup, similarly propagating changes
-#   to localStorage and into vimium's internal state.
 #
-# Changes are propagated into vimium's state using the same mechanism that is
-# used when options are changed on the options page.
+# Changes are propagated into vimium's state using the same mechanism
+# (Settings.doPostUpdateHook) that is used when options are changed on
+# the options page.
 #
 # The effect is best-effort synchronization of vimium options/settings between
-# chrome/vimium instances, whenever:
-#   - chrome is logged in to the user's Google account, and
-#   - chrome synchronization is enabled.
+# chrome/vimium instances.
 #
 # NOTE:
 #   Values handled within this module are ALWAYS already JSON.stringifed, so
 #   they're always non-empty strings.
 #
 
-console.log ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"
 root = exports ? window
-root.Sync = Sync = 
-
-  # ##################
-  # constants
+root.Sync = Sync =
 
   debug: true
   storage: chrome.storage.sync
@@ -33,104 +28,100 @@ root.Sync = Sync =
   init: ->
     chrome.storage.onChanged.addListener (changes, area) -> Sync.listener changes, area
     @pull()
-    @log "Sync.init()"
 
-  # asynchronous fetch from synced storage, called at startup
+  # Asynchronous fetch from synced storage, called only at startup.
   pull: ->
     @storage.get null, (items) ->
-      Sync.log "pull callback: #{Sync.callbackStatus()}"
-      if not chrome.runtime.lastError
+      if chrome.runtime.lastError is undefined
         for own key, value of items
-          Sync.storeAndPropagate key, value
+          @storeAndPropagate key, value
+      else
+        @log "chrome sync callback for Sync.pull() indicates error"
+        @log chrome.runtime.lastError
 
-  # asynchronous message from synced storage
+  # Asynchronous message from synced storage.
   listener: (changes, area) ->
-    @log "listener: #{area}"
     for own key, change of changes
       @storeAndPropagate key, change.newValue
   
-  # only ever called from asynchronous synced-storage callbacks (pull and listener)
+  # Only ever called from asynchronous synced-storage callbacks (pull and listener).
   storeAndPropagate: (key, value) ->
-    # must be JSON.stringifed or undefined
-    @checkHaveStringOrUndefined value
-    # ignore, if not accepting this key
-    if not @syncKey key
-       @log "callback ignoring: #{key}"
+    # Value must be JSON.stringifed or undefined.
+    if not @checkHaveStringOrUndefined value
+      return
+    # Ignore, we're not accepting this key.
+    if not @isSyncKey key
+       @log "ignoring: #{key}"
        return
-    # ignore, if unchanged
-    if localStorage[key] == value
-       @log "callback unchanged: #{key}"
+    # Ignore, it's unchanged
+    if localStorage[key] is value
+       @log "unchanged: #{key}"
        return
 
-    # ok: accept, store and propagate update
+    # Ok: accept, store and propagate this update.
     defaultValue = root.Settings.defaults[key]
-    defaultValueJSON = JSON.stringify(defaultValue) # could cache this to avoid repeated recalculation
+    defaultValueJSON = JSON.stringify(defaultValue)
 
     if value && value != defaultValueJSON
-      # key/value has been changed to non-default value at remote instance
-      @log "callback set: #{key}=#{value}"
+      # Key/value has been changed to non-default value at remote instance.
+      @log "update: #{key}=#{value}"
       localStorage[key] = value
       root.Settings.doPostUpdateHook key, JSON.parse(value)
     else
-      # key has been reset to default value at remote instance
-      @log "callback clear: #{key}=#{value}"
+      # Key has been reset to default value at remote instance.
+      @log "clear: #{key}"
       delete localStorage[key]
       root.Settings.doPostUpdateHook key, defaultValue
 
-  # only called synchronously from within vimium, never on a callback
-  # no need to propagate updates into the rest of vimium (because that will already have been handled externally)
+  # Only called synchronously from within vimium, never on a callback.
+  # No need to propagate updates into the rest of vimium.
   set: (key, value) ->
-    # value must be JSON.stringifed
-    @checkHaveString value
-    if value
-      if @syncKey key
-        @storage.set @mkKeyValue(key,value), -> Sync.logCallback "DONE set", key
-        @log "set scheduled: #{key}=#{value}"
-    else
-      # unreachable? (because value is a JSON string)
-      @log "UNREACHABLE in Sync.set(): #{key}"
-      @clear key
-
-  # only called synchronously from within vimium, never on a callback
-  # no need to propagate updates into the rest of  vimium (because that will already have been handled by externally)
+    # value has already been JSON.stringifed
+    if not @checkHaveString value
+      return
+    #
+    if @isSyncKey key
+      @storage.set @mkKeyValue(key,value), ->
+        if chrome.runtime.lastError
+          @log "chrome sync callback for Sync.set() indicates error: " + key
+          @log chrome.runtime.lastError
+      @log "set scheduled: #{key}=#{value}"
+
+  # Only called synchronously from within vimium, never on a callback.
   clear: (key) ->
-    if @syncKey key
-      @storage.remove key, -> Sync.logCallback "DONE clear", key
-      @log "clear scheduled: #{key}"
-
-  # ##################
-  # utilities 
-
-  syncKey: (key) ->
+    if @isSyncKey key
+      @storage.remove key, ->
+        if chrome.runtime.lastError
+          @log "chrome sync callback for Sync.clear() indicates error: " + key
+          @log chrome.runtime.lastError
+
+  # Should we synchronize this key?
+  isSyncKey: (key) ->
     key not in @doNotSync
 
-  # there has to be a more elegant way to do this!
+  # There has to be a more elegant way to do this!
   mkKeyValue: (key, value) ->
     obj = {}
     obj[key] = value
     obj
 
-  # debugging messages
-  # disable these by setting root.Sync.debug to anything falsy
+  # Debugging messages.
+  # Disable debugginf by setting root.Sync.debug to anything falsy.
+  # Enabled for the time being (18/4/14) -- smblott.
   log: (msg) ->
-    console.log "sync debug: #{msg}" if @debug
-
-  logCallback: (where, key) ->
-    @log "#{where} callback: #{key} #{@callbackStatus()}"
-
-  callbackStatus: ->
-    if chrome.runtime.lastError then "ERROR: #{chrome.runtime.lastError.message}" else "(OK)"
+    console.log "Sync: #{msg}" if @debug
 
   checkHaveString: (thing) ->
     if typeof(thing) != "string" or not thing
-      @log "sync.coffee: Yikes! this should be a non-empty string: #{typeof(thing)} #{thing}"
+      @log "Sync: Yikes! this should be a non-empty string: #{typeof(thing)} #{thing}"
+      return false
+    return true
 
   checkHaveStringOrUndefined: (thing) ->
     if ( typeof(thing) != "string" and typeof(thing) != "undefined" ) or ( typeof(thing) == "string" and not thing )
-      @log "sync.coffee: Yikes! this should be a non-empty string or undefined: #{typeof(thing)} #{thing}"
+      @log "Sync: Yikes! this should be a non-empty string or undefined: #{typeof(thing)} #{thing}"
+      return false
+    return true
   
-  # end of Sync object
-  # ##################
-
 Sync.init()