From 765aa40cc1da52efb269775d9c96a8742f0a4bd2 Mon Sep 17 00:00:00 2001 From: jdlrobson Date: Fri, 13 May 2016 13:15:36 -0700 Subject: [PATCH] Replace use of jStorage with mw.storage Use the standardised MediaWiki storage system for the simple use case of disabling/enabling Popups. The jStorage library is 12kb uncompressed and cannot be justified for this usage (and if it becomes the standardised library mw.storage will begin using it) This means that browsers with full localStorage or no localStorage support will not be able to disable Popups. Given the current ecosystem of the web - localStorage is widely supported - and the fact that grade A browsers enjoy localStorage support - this is not a problem. See https://github.com/wikimedia/mediawiki/blob/REL1_27/resources/src/startup.js#L59 Changes: * Stop using jStorage * Cleanup and migrate previous values in jStorage with plan for removing this in a month. Bug: T136241 Change-Id: I6dac2911e84d6cb20731be34add01576cf407c46 --- extension.json | 3 +- resources/ext.popups.core.js | 8 ++--- resources/ext.popups.experiment.js | 10 +++--- resources/ext.popups.settings.js | 2 +- resources/ext.popups.targets/desktopTarget.js | 31 ++++++++++++++++--- tests/qunit/ext.popups.core.test.js | 22 ++++++------- tests/qunit/ext.popups.experiment.test.js | 10 +++--- tests/qunit/ext.popups.settings.test.js | 8 ++--- 8 files changed, 58 insertions(+), 36 deletions(-) diff --git a/extension.json b/extension.json index d19e1fa7a..db88d9bf5 100644 --- a/extension.json +++ b/extension.json @@ -82,7 +82,7 @@ "resources/ext.popups.targets/desktopTarget.js" ], "dependencies": [ - "jquery.jStorage", + "mediawiki.storage", "jquery.client", "ext.popups.core", "ext.popups.renderer.desktopRenderer", @@ -104,7 +104,6 @@ ], "dependencies": [ "ext.popups.core", - "jquery.jStorage", "mediawiki.user", "mediawiki.storage", "mediawiki.experiments" diff --git a/resources/ext.popups.core.js b/resources/ext.popups.core.js index 4b2aef6af..d96c25e19 100644 --- a/resources/ext.popups.core.js +++ b/resources/ext.popups.core.js @@ -214,16 +214,16 @@ }; /** - * Save the popups enabled state via $.jStorage + * Save the popups enabled state via device storage * * @param {boolean} isEnabled */ mw.popups.saveEnabledState = function ( isEnabled ) { - $.jStorage.set( popupsEnabledStorageKey, isEnabled.toString() ); + mw.storage.set( popupsEnabledStorageKey, isEnabled ? '1' : '0' ); }; /** - * Retrieve the popups enabled state via $.jStorage or 'wgPopupsExperiment' + * Retrieve the popups enabled state via device storage or 'wgPopupsExperiment' * config variable. * If the experiment isn't running, then continue to enable Popups * by default during initialisation. In this case the return value @@ -233,7 +233,7 @@ */ mw.popups.getEnabledState = function () { if ( !mw.config.get( 'wgPopupsExperiment', false ) ) { - return $.jStorage.get( popupsEnabledStorageKey ) !== 'false'; + return mw.storage.get( popupsEnabledStorageKey ) !== '0'; } else { return mw.popups.experiment.isUserInCondition(); } diff --git a/resources/ext.popups.experiment.js b/resources/ext.popups.experiment.js index 4812b1ba3..809fb02dd 100644 --- a/resources/ext.popups.experiment.js +++ b/resources/ext.popups.experiment.js @@ -1,4 +1,4 @@ -( function ( mw, $ ) { +( function ( mw ) { /** * @ignore @@ -24,9 +24,9 @@ * @ignore */ function hasUserEnabledFeature() { - var value = $.jStorage.get( 'mwe-popups-enabled' ); + var value = mw.storage.get( 'mwe-popups-enabled' ); - return Boolean( value ) && value !== 'false'; + return Boolean( value ) && value !== '0'; } /** @@ -37,7 +37,7 @@ * @ignore */ function hasUserDisabledFeature() { - return $.jStorage.get( 'mwe-popups-enabled' ) === 'false'; + return mw.storage.get( 'mwe-popups-enabled' ) === '0'; } /** @@ -87,4 +87,4 @@ return mw.experiments.getBucket( config, getToken() ) !== 'control'; }; -}( mediaWiki, jQuery ) ); +}( mediaWiki ) ); diff --git a/resources/ext.popups.settings.js b/resources/ext.popups.settings.js index 50e373164..85ebea33a 100644 --- a/resources/ext.popups.settings.js +++ b/resources/ext.popups.settings.js @@ -72,7 +72,7 @@ }; /** - * Save the setting to localStorage (jStorage) and close the dialog + * Save the setting to the device and close the dialog * * @method save */ diff --git a/resources/ext.popups.targets/desktopTarget.js b/resources/ext.popups.targets/desktopTarget.js index 86ac43e19..9b06f9b3b 100644 --- a/resources/ext.popups.targets/desktopTarget.js +++ b/resources/ext.popups.targets/desktopTarget.js @@ -154,11 +154,34 @@ } } ); + function initPopups() { + mw.popups.checkScroll(); + mw.popups.createSVGMask(); + mw.popups.createPopupElement(); + } + $( function () { - if ( mw.popups.enabled ) { - mw.popups.checkScroll(); - mw.popups.createSVGMask(); - mw.popups.createPopupElement(); + var jStorage = mw.storage.get( 'jStorage' ); + + // FIXME: Can be removed a month from jStorage removal. Not perfect but should work in most cases. + if ( jStorage && jStorage.indexOf( 'popups-enabled' ) > 0 ) { + // slower loading popups when in transition mode + mw.loader.using( 'jquery.jStorage' ).done( function () { + var val = $.jStorage.get( 'mwe-popups-enabled' ); + // delete jStorage key + $.jStorage.deleteKey( 'mwe-popups-enabled' ); + if ( val === 'false' ) { + // copy over + mw.storage.set( 'mwe-popups-enabled', '0' ); + } else { + if ( val ) { + mw.storage.set( 'mwe-popups-enabled', '1' ); + } + initPopups(); + } + } ); + } else if ( mw.popups.enabled ) { + initPopups(); } mw.track( 'ext.popups.schemaPopups', { diff --git a/tests/qunit/ext.popups.core.test.js b/tests/qunit/ext.popups.core.test.js index 31a80d214..235aef6dc 100644 --- a/tests/qunit/ext.popups.core.test.js +++ b/tests/qunit/ext.popups.core.test.js @@ -286,22 +286,22 @@ QUnit.test( 'saveEnabledState', function ( assert ) { var storageKey = 'mwe-popups-enabled', - jStorageStub = this.sandbox.stub( $.jStorage, 'set' ) + deviceStorageStub = this.sandbox.stub( mw.storage, 'set' ) .withArgs( storageKey ); QUnit.expect( 2 ); mw.popups.saveEnabledState( true ); assert.equal( - jStorageStub.firstCall.args[ 1 ], - 'true', + deviceStorageStub.firstCall.args[ 1 ], + '1', 'Popups enabled state has been saved as "true".' ); mw.popups.saveEnabledState( false ); assert.equal( - jStorageStub.secondCall.args[ 1 ], - 'false', + deviceStorageStub.secondCall.args[ 1 ], + '0', 'Popups enabled state has been saved as "false".' ); } ); @@ -310,7 +310,7 @@ var storageKey = 'mwe-popups-enabled', mwConfigStub = this.sandbox.stub( mw.config, 'get' ) .withArgs( 'wgPopupsExperiment' ), - jStorageStub = this.sandbox.stub( $.jStorage, 'get' ) + deviceStorageStub = this.sandbox.stub( mw.storage, 'get' ) .withArgs( storageKey ), experimentStub = this.sandbox.stub( mw.popups.experiment, 'isUserInCondition' ); @@ -318,7 +318,7 @@ QUnit.expect( 7 ); mwConfigStub.returns( null ); - jStorageStub.returns( null ); + deviceStorageStub.returns( null ); assert.equal( mw.popups.getEnabledState(), true, @@ -327,7 +327,7 @@ ); mwConfigStub.returns( null ); - jStorageStub.returns( 'true' ); + deviceStorageStub.returns( '1' ); assert.equal( mw.popups.getEnabledState(), true, @@ -336,7 +336,7 @@ ); mwConfigStub.returns( null ); - jStorageStub.returns( 'false' ); + deviceStorageStub.returns( '0' ); assert.equal( mw.popups.getEnabledState(), false, @@ -345,7 +345,7 @@ ); mwConfigStub.returns( false ); - jStorageStub.returns( 'true' ); + deviceStorageStub.returns( '1' ); assert.equal( mw.popups.getEnabledState(), true, @@ -354,7 +354,7 @@ ); mwConfigStub.returns( false ); - jStorageStub.returns( 'false' ); + deviceStorageStub.returns( '0' ); assert.equal( mw.popups.getEnabledState(), false, diff --git a/tests/qunit/ext.popups.experiment.test.js b/tests/qunit/ext.popups.experiment.test.js index bdee981b3..225cde7ba 100644 --- a/tests/qunit/ext.popups.experiment.test.js +++ b/tests/qunit/ext.popups.experiment.test.js @@ -1,4 +1,4 @@ -( function ( mw, $ ) { +( function ( mw ) { QUnit.module( 'ext.popups.experiment', QUnit.newMwEnvironment( { config: { @@ -13,7 +13,7 @@ wgPopupsExperimentIsBetaFeatureEnabled: null }, setup: function () { - $.jStorage.deleteKey( 'mwe-popups-enabled' ); + mw.storage.remove( 'mwe-popups-enabled' ); }, teardown: function () { mw.storage.remove( 'PopupsExperimentID' ); @@ -80,7 +80,7 @@ } ); QUnit.test( '#isUserInCondition: user has enabled the feature', 1, function ( assert ) { - $.jStorage.set( 'mwe-popups-enabled', 'true' ); + mw.storage.set( 'mwe-popups-enabled', '1' ); assert.strictEqual( mw.popups.experiment.isUserInCondition(), @@ -93,7 +93,7 @@ // This should be read as follows: the user has enabled the beta feature but has since // disabled the feature via its settings. mw.config.set( 'wgPopupsExperimentIsBetaFeatureEnabled', true ); - $.jStorage.set( 'mwe-popups-enabled', 'false' ); + mw.storage.set( 'mwe-popups-enabled', '0' ); assert.strictEqual( mw.popups.experiment.isUserInCondition(), @@ -102,4 +102,4 @@ ); } ); -}( mediaWiki, jQuery ) ); +}( mediaWiki ) ); diff --git a/tests/qunit/ext.popups.settings.test.js b/tests/qunit/ext.popups.settings.test.js index c53c9c325..d9d55e3e5 100644 --- a/tests/qunit/ext.popups.settings.test.js +++ b/tests/qunit/ext.popups.settings.test.js @@ -29,16 +29,16 @@ radioButtonValue = 'simple'; mw.popups.settings.save(); assert.equal( - $.jStorage.get( 'mwe-popups-enabled' ), - 'true', + mw.storage.get( 'mwe-popups-enabled' ), + '1', 'Popups are enabled when the `simple` radio button is checked.' ); radioButtonValue = 'off'; mw.popups.settings.save(); assert.equal( - $.jStorage.get( 'mwe-popups-enabled' ), - 'false', + mw.storage.get( 'mwe-popups-enabled' ), + '0', 'Popups are disabled when the `off` radio button is checked.' );