From e0750f9b2c1495473fdcbb2f22dd97961803bc81 Mon Sep 17 00:00:00 2001 From: Andrew Webster Date: Thu, 25 Jan 2018 15:23:08 -0500 Subject: [PATCH 1/2] Use localstorage only to initialize settings map This only reads from localstorage in order to initialize the settings map. After initializaton, reads will return the value from the map. When writing a value, the settings map and the local storage are updated, unless the setting is a default value or derived from the query string. This has a few advantages: 1. Saved settings will not be overridden by settings specified in the query string. This means a setting could be temporarily changed using the query string, but once removed from the query string, the setting would return back to what the user selected. 2. Default values will not be saved. If a user has always used the default value for a setting, then they can move to a new version with different defaults without clearing localstorage. 3. Changes made to localstorage in a session running in a different window will not affect the settings in the current window (until the page is refreshed). Regarding eraseSetting: It is possible that another tab could change the value, leading to an unexpected value change in the tab that deletes. However, this function is currently unused, so this will be evaluted if and when it used. --- app/ui.js | 2 +- app/webutil.js | 24 ++++-- karma.conf.js | 2 + tests/test.webutil.js | 182 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 202 insertions(+), 8 deletions(-) create mode 100644 tests/test.webutil.js diff --git a/app/ui.js b/app/ui.js index 2218d241..7c754cec 100644 --- a/app/ui.js +++ b/app/ui.js @@ -731,7 +731,7 @@ var UI = { // Save the cookie for this session if (typeof value !== 'undefined') { - WebUtil.writeSetting(name, value); + WebUtil.setSetting(name, value); } // Update the settings control 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'); + }); + }); + }); + }); +}); From 8ad8f15cf6ea3a5417ee471d168a585f11f225bc Mon Sep 17 00:00:00 2001 From: Andrew Webster Date: Thu, 1 Feb 2018 11:46:38 -0500 Subject: [PATCH 2/2] Move writeSetting from updateSetting to initSetting initSetting was the only place that supplied a 'value' to updateSetting. So move it to clean up updateSetting. --- app/ui.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/ui.js b/app/ui.js index 7c754cec..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.setSetting(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') {