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.

diff --git a/app/ui.js b/app/ui.js
index 2218d241b249f876e9c35e89924538554c0270de..7c754cecdef9684c6ed7216083ae5e813209ef26 100644 (file)
--- 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 249a1382fe87fb150562835314bcfbe633864510..1087b330a22ebad7c540e19b471cce6573738677 100644 (file)
--- 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 10bf372ac7de77375846213f50198fa60c6da49b..5b9da9f1533ab0641fb38735330e4a93e7ec05af 100644 (file)
--- 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 (file)
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');
+                });
+            });
+        });
+    });
+});