diff --git a/app/ui.js b/app/ui.js index 2218d241..592dfcf7 100644 --- a/app/ui.js +++ b/app/ui.js @@ -721,21 +721,17 @@ var UI = { if (val === null) { val = WebUtil.readSetting(name, defVal); } - UI.updateSetting(name, val); + WebUtil.setSetting(name, val); + UI.updateSetting(name); return val; }, // Update cookie and form control setting. If value is not set, then // updates from control to current cookie setting. - updateSetting: function(name, value) { - - // Save the cookie for this session - if (typeof value !== 'undefined') { - WebUtil.writeSetting(name, value); - } + updateSetting: function(name) { // Update the settings control - value = UI.getSetting(name); + var value = UI.getSetting(name); var ctrl = document.getElementById('noVNC_setting_' + name); if (ctrl.type === 'checkbox') { diff --git a/app/webutil.js b/app/webutil.js index 249a1382..1087b330 100644 --- a/app/webutil.js +++ b/app/webutil.js @@ -117,21 +117,25 @@ export function initSettings (callback /*, ...callbackArgs */) { } }); } else { - // No-op + settings = {}; if (callback) { callback.apply(this, callbackArgs); } } }; +// Update the settings cache, but do not write to permanent storage +export function setSetting (name, value) { + settings[name] = value; +}; + // No days means only for this browser session export function writeSetting (name, value) { "use strict"; + if (settings[name] === value) return; + settings[name] = value; if (window.chrome && window.chrome.storage) { - if (settings[name] !== value) { - settings[name] = value; - window.chrome.storage.sync.set(settings); - } + window.chrome.storage.sync.set(settings); } else { localStorage.setItem(name, value); } @@ -140,10 +144,11 @@ export function writeSetting (name, value) { export function readSetting (name, defaultValue) { "use strict"; var value; - if (window.chrome && window.chrome.storage) { + if ((name in settings) || (window.chrome && window.chrome.storage)) { value = settings[name]; } else { value = localStorage.getItem(name); + settings[name] = value; } if (typeof value === "undefined") { value = null; @@ -157,9 +162,14 @@ export function readSetting (name, defaultValue) { export function eraseSetting (name) { "use strict"; + // Deleting here means that next time the setting is read when using local + // storage, it will be pulled from local storage again. + // If the setting in local storage is changed (e.g. in another tab) + // between this delete and the next read, it could lead to an unexpected + // value change. + delete settings[name]; if (window.chrome && window.chrome.storage) { window.chrome.storage.sync.remove(name); - delete settings[name]; } else { localStorage.removeItem(name); } diff --git a/karma.conf.js b/karma.conf.js index 10bf372a..5b9da9f1 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -62,6 +62,7 @@ module.exports = function(config) { { pattern: 'vendor/sinon.js', included: false }, { pattern: 'node_modules/sinon-chai/lib/sinon-chai.js', included: false }, { pattern: 'app/localization.js', included: false }, + { pattern: 'app/webutil.js', included: false }, { pattern: 'core/**/*.js', included: false }, { pattern: 'vendor/pako/**/*.js', included: false }, { pattern: 'tests/test.*.js', included: false }, @@ -92,6 +93,7 @@ module.exports = function(config) { // available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor preprocessors: { 'app/localization.js': ['babel'], + 'app/webutil.js': ['babel'], 'core/**/*.js': ['babel'], 'tests/test.*.js': ['babel'], 'tests/fake.*.js': ['babel'], diff --git a/tests/test.webutil.js b/tests/test.webutil.js new file mode 100644 index 00000000..b3c59649 --- /dev/null +++ b/tests/test.webutil.js @@ -0,0 +1,182 @@ +/* jshint expr: true */ + +var assert = chai.assert; +var expect = chai.expect; + +import * as WebUtil from '../app/webutil.js'; + +import sinon from '../vendor/sinon.js'; + +describe('WebUtil', function() { + "use strict"; + + describe('settings', function () { + + // on Firefox, localStorage methods cannot be replaced + // localStorage is (currently) mockable on Chrome + // test to see if localStorage is mockable + var mockTest = sinon.spy(window.localStorage, 'setItem'); + var canMock = window.localStorage.setItem.getCall instanceof Function; + mockTest.restore(); + if (!canMock) { + console.warn('localStorage cannot be mocked'); + } + (canMock ? describe : describe.skip)('localStorage', function() { + var chrome = window.chrome; + before(function() { + chrome = window.chrome; + window.chrome = null; + }); + after(function() { + window.chrome = chrome; + }); + + var lsSandbox = sinon.createSandbox(); + + beforeEach(function() { + lsSandbox.stub(window.localStorage, 'setItem'); + lsSandbox.stub(window.localStorage, 'getItem'); + lsSandbox.stub(window.localStorage, 'removeItem'); + WebUtil.initSettings(); + }); + afterEach(function() { + lsSandbox.restore(); + }); + + describe('writeSetting', function() { + it('should save the setting value to local storage', function() { + WebUtil.writeSetting('test', 'value'); + expect(window.localStorage.setItem).to.have.been.calledWithExactly('test', 'value'); + expect(WebUtil.readSetting('test')).to.equal('value'); + }); + }); + + describe('setSetting', function() { + it('should update the setting but not save to local storage', function() { + WebUtil.setSetting('test', 'value'); + expect(window.localStorage.setItem).to.not.have.been.called; + expect(WebUtil.readSetting('test')).to.equal('value'); + }); + }); + + describe('readSetting', function() { + it('should read the setting value from local storage', function() { + localStorage.getItem.returns('value'); + expect(WebUtil.readSetting('test')).to.equal('value'); + }); + + it('should return the default value when not in local storage', function() { + expect(WebUtil.readSetting('test', 'default')).to.equal('default'); + }); + + it('should return the cached value even if local storage changed', function() { + localStorage.getItem.returns('value'); + expect(WebUtil.readSetting('test')).to.equal('value'); + localStorage.getItem.returns('something else'); + expect(WebUtil.readSetting('test')).to.equal('value'); + }); + + it('should cache the value even if it is not initially in local storage', function() { + expect(WebUtil.readSetting('test')).to.be.null; + localStorage.getItem.returns('value'); + expect(WebUtil.readSetting('test')).to.be.null; + }); + + it('should return the default value always if the first read was not in local storage', function() { + expect(WebUtil.readSetting('test', 'default')).to.equal('default'); + localStorage.getItem.returns('value'); + expect(WebUtil.readSetting('test', 'another default')).to.equal('another default'); + }); + + it('should return the last local written value', function() { + localStorage.getItem.returns('value'); + expect(WebUtil.readSetting('test')).to.equal('value'); + WebUtil.writeSetting('test', 'something else'); + expect(WebUtil.readSetting('test')).to.equal('something else'); + }); + }); + + // this doesn't appear to be used anywhere + describe('eraseSetting', function() { + it('should remove the setting from local storage', function() { + WebUtil.eraseSetting('test'); + expect(window.localStorage.removeItem).to.have.been.calledWithExactly('test'); + }); + }); + }); + + describe('chrome.storage', function() { + var chrome = window.chrome; + var settings = {}; + before(function() { + chrome = window.chrome; + window.chrome = { + storage: { + sync: { + get: function(cb){ cb(settings); }, + set: function(){}, + remove: function() {} + } + } + }; + }); + after(function() { + window.chrome = chrome; + }); + + var csSandbox = sinon.createSandbox(); + + beforeEach(function() { + settings = {}; + csSandbox.spy(window.chrome.storage.sync, 'set'); + csSandbox.spy(window.chrome.storage.sync, 'remove'); + WebUtil.initSettings(); + }); + afterEach(function() { + csSandbox.restore(); + }); + + describe('writeSetting', function() { + it('should save the setting value to chrome storage', function() { + WebUtil.writeSetting('test', 'value'); + expect(window.chrome.storage.sync.set).to.have.been.calledWithExactly(sinon.match({ test: 'value' })); + expect(WebUtil.readSetting('test')).to.equal('value'); + }); + }); + + describe('setSetting', function() { + it('should update the setting but not save to chrome storage', function() { + WebUtil.setSetting('test', 'value'); + expect(window.chrome.storage.sync.set).to.not.have.been.called; + expect(WebUtil.readSetting('test')).to.equal('value'); + }); + }); + + describe('readSetting', function() { + it('should read the setting value from chrome storage', function() { + settings.test = 'value'; + expect(WebUtil.readSetting('test')).to.equal('value'); + }); + + it('should return the default value when not in chrome storage', function() { + expect(WebUtil.readSetting('test', 'default')).to.equal('default'); + }); + + it('should return the last local written value', function() { + settings.test = 'value'; + expect(WebUtil.readSetting('test')).to.equal('value'); + WebUtil.writeSetting('test', 'something else'); + expect(WebUtil.readSetting('test')).to.equal('something else'); + }); + }); + + // this doesn't appear to be used anywhere + describe('eraseSetting', function() { + it('should remove the setting from chrome storage', function() { + WebUtil.eraseSetting('test'); + expect(window.chrome.storage.sync.remove).to.have.been.calledWithExactly('test'); + }); + }); + }); + }); +});