From 75234c78cf13be08608ccd2b6bcecf2757b199ff Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Fri, 18 Apr 2014 23:09:39 +0100 Subject: Code review of Sync(). --- background_scripts/settings.coffee | 37 ++++------ background_scripts/sync.coffee | 139 +++++++++++++++++-------------------- 2 files changed, 78 insertions(+), 98 deletions(-) (limited to 'background_scripts') 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() -- cgit v1.2.3