Merge branch 'settings' of https://github.com/andrwwbstr/noVNC

4 files changed, 205 insertions, 15 deletions

diff --git a/app/ui.js b/app/ui.js
index 2218d24..592dfcf 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 249a138..1087b33 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 10bf372..5b9da9f 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 0000000..b3c5964
--- /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');
+                });
+            });
+        });
+    });
+});