Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to storage.sync API, so we can synchronize Vimium settings across computers #1010

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 38 additions & 6 deletions background_scripts/settings.coffee
Expand Up @@ -7,17 +7,49 @@ root.Settings = Settings =
get: (key) ->
if (key of localStorage) then JSON.parse(localStorage[key]) else @defaults[key]

set: (key, value) ->
# The doNotSync argument suppresses calls to chrome.storage.sync.* while running unit tests
set: (key, value, doNotSync) ->
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like these doNotSync arguments are used in this PR. Also, if possible, I'd rather stub out the chrome.storage api in the unit tests so we don't need to parameterize the behavior for testing.

# 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
localStorage[key] = JSON.stringify(value)
# 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

clear: (key) -> delete localStorage[key]
# The doNotSync argument suppresses calls to chrome.storage.sync.* while running unit tests
clear: (key, doNotSync) ->
if @has key
root.Sync.clear key if root?.Sync?.clear
delete localStorage[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:
keyMappings: (value) ->
root.Commands.clearKeyMappingsAndSetDefaults()
root.Commands.parseCustomKeyMappings value
root.refreshCompletionKeysAfterMappingSave()

# postUpdateHooks convenience wrapper
doPostUpdateHook: (key, value) ->
if @postUpdateHooks[key]
@postUpdateHooks[key] value


# options/options.(coffee|html) only handle booleans and strings; therefore
# all defaults must be booleans or strings
defaults:
Expand Down
136 changes: 136 additions & 0 deletions background_scripts/sync.coffee
@@ -0,0 +1,136 @@

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Black line is unnecessary

#
# * Sync.set() and Sync.clear() propagate local changes to chrome.storage.
# * Sync.listener() listens for changes to chrome.storage and propagates those
# 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.
#
# 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.
#
# NOTE:
# Values handled within this module are ALWAYS already JSON.stringifed, so
# they're always non-empty strings.
#
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very helpful comments. So to confirm this means functions in this file will never receive NULL or empty string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.


console.log ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"
root = exports ? window
root.Sync = Sync =

# ##################
# constants
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These section markers aren't needed on such a small file IMO, and they're not used in the rest of the vimium code base.


debug: true
storage: chrome.storage.sync
doNotSync: [ "settingsVersion", "previousVersion" ]

init: ->
chrome.storage.onChanged.addListener (changes, area) -> Sync.listener changes, area
@pull()
@log "Sync.init()"

# asynchronous fetch from synced storage, called at startup
pull: ->
@storage.get null, (items) ->
Sync.log "pull callback: #{Sync.callbackStatus()}"
if not chrome.runtime.lastError
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to check chrome.runtime.lastError here? Can you add a comment what case this covers?

for own key, value of items
Sync.storeAndPropagate key, value

# 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)
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}"
return
# ignore, if unchanged
if localStorage[key] == value
@log "callback unchanged: #{key}"
return

# ok: accept, store and propagate update
defaultValue = root.Settings.defaults[key]
defaultValueJSON = JSON.stringify(defaultValue) # could cache this to avoid repeated recalculation

if value && value != defaultValueJSON
# key/value has been changed to non-default value at remote instance
@log "callback set: #{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}"
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)
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)
clear: (key) ->
if @syncKey key
@storage.remove key, -> Sync.logCallback "DONE clear", key
@log "clear scheduled: #{key}"

# ##################
# utilities

syncKey: (key) ->
key not in @doNotSync

# 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
log: (msg) ->
console.log "sync debug: #{msg}" if @debug
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In anticipation of the inevitable sync-related bug report, do you think we should leave these debug logs in & on for everyone? Is this the right set of log statements or is it too noisy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. They were written for me. Will keep in mind what a bug report might look like.


logCallback: (where, key) ->
@log "#{where} callback: #{key} #{@callbackStatus()}"

callbackStatus: ->
if chrome.runtime.lastError then "ERROR: #{chrome.runtime.lastError.message}" else "(OK)"

checkHaveString: (thing) ->
if typeof(thing) != "string" or not thing
@log "sync.coffee: Yikes! this should be a non-empty string: #{typeof(thing)} #{thing}"

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}"

# end of Sync object
# ##################

Sync.init()

2 changes: 2 additions & 0 deletions manifest.json
Expand Up @@ -11,6 +11,7 @@
"lib/utils.js",
"background_scripts/commands.js",
"lib/clipboard.js",
"background_scripts/sync.js",
"background_scripts/settings.js",
"background_scripts/completion.js",
"background_scripts/marks.js",
Expand All @@ -23,6 +24,7 @@
"bookmarks",
"history",
"clipboardRead",
"storage",
"<all_urls>"
],
"content_scripts": [
Expand Down
8 changes: 1 addition & 7 deletions pages/options.coffee
Expand Up @@ -8,12 +8,6 @@ editableFields = [ "scrollStepSize", "excludedUrls", "linkHintCharacters", "link

canBeEmptyFields = ["excludedUrls", "keyMappings", "userDefinedLinkHintCss"]

postSaveHooks = keyMappings: (value) ->
commands = chrome.extension.getBackgroundPage().Commands
commands.clearKeyMappingsAndSetDefaults()
commands.parseCustomKeyMappings value
chrome.extension.getBackgroundPage().refreshCompletionKeysAfterMappingSave()

document.addEventListener "DOMContentLoaded", ->
populateOptions()

Expand Down Expand Up @@ -73,7 +67,7 @@ saveOptions = ->
bgSettings.set fieldName, fieldValue
$(fieldName).value = fieldValue
$(fieldName).setAttribute "savedValue", fieldValue
postSaveHooks[fieldName] fieldValue if postSaveHooks[fieldName]
bgSettings.doPostUpdateHook fieldName, fieldValue

$("saveOptions").disabled = true

Expand Down