Redo wrapper for localStorage integration

I think the issues in T329299 are caused by partially autosaved
comments. We store data in multiple localStorage keys, and if some of
them are stored but others are not (due to exceeding storage quota),
our code can't handle the inconsistent state.

We already have a wrapper around localStorage that tries to cover up
these issues. Change it so that all values specific to an instance of
a reply tool are stored under one localStorage key. This ensures that
all updates consistently succeed or fail, with no partially stored
state.

One of the reasons we haven't done this is because this requires the
whole data to be serialized to JSON every time, but our experience
with VE change 4355d697aa shows that this is fast enough.

Extra changes:
* Remove storagePrefix, now redundant
* Remove use of createConflictableStorage, now redundant
* Prefix the key with 'mw' as advised by mw.storage documentation
* Use ES6 syntax for the new code (just for fun)
* Use consistent expiry (T339042)

Bug: T329299
Change-Id: I347115f7187fd7d6afd9c6f368441e262154233b

extension.json

@@ -78,7 +78,7 @@
 				"CommentItem.js",
 				"HeadingItem.js",
 				"CommentDetails.js",
-				"createMemoryStorage.js",
+				"MemoryStorage.js",
 				"lib/moment-timezone/moment-timezone-with-data-1970-2030.js",
 				{
 					"name": "parser/data.json",

modules/.eslintrc.json

@@ -1,7 +1,7 @@
 {
 	"root": true,
 	"extends": [
-		"wikimedia/client-es5",
+		"wikimedia/client",
 		"wikimedia/jquery",
 		"wikimedia/mediawiki"
 	],
@@ -11,6 +11,7 @@
 	},
 	"rules": {
 		"no-implicit-globals": "off",
+		"no-var": "off",
 		"max-len": "off"
 	},
 	"settings": {

modules/CommentController.js

@@ -16,8 +16,9 @@ var
  * @param {jQuery} $pageContainer Page container
  * @param {ThreadItem} threadItem Thread item to attach new comment to
  * @param {ThreadItemSet} threadItemSet
+ * @param {MemoryStorage} storage Storage object for autosave
  */
-function CommentController( $pageContainer, threadItem, threadItemSet ) {
+function CommentController( $pageContainer, threadItem, threadItemSet, storage ) {
 	// Mixin constructors
 	OO.EventEmitter.call( this );
 
@@ -25,6 +26,7 @@ function CommentController( $pageContainer, threadItem, threadItemSet ) {
 	this.$pageContainer = $pageContainer;
 	this.threadItem = threadItem;
 	this.threadItemSet = threadItemSet;
+	this.storage = storage;
 	this.newListItem = null;
 	this.replyWidgetPromise = null;
 	this.newComments = [];
@@ -334,7 +336,7 @@ CommentController.prototype.setupReplyWidget = function ( replyWidget, data, sup
 
 CommentController.prototype.storeEditSummary = function () {
 	if ( this.replyWidget ) {
-		this.replyWidget.storage.set( this.replyWidget.storagePrefix + '/summary', this.replyWidget.getEditSummary() );
+		this.replyWidget.storage.set( 'summary', this.replyWidget.getEditSummary() );
 	}
 };
 

modules/MemoryStorage.js

@@ -0,0 +1,75 @@
+/**
+ * MemoryStorage is a wrapper around mw.SafeStorage objects.
+ *
+ * It provides two mechanisms to improve their behavior when the underlying storage mechanism fails
+ * (e.g. quota exceeded):
+ *
+ * - Duplicating their contents in memory, so that the storage can be relied on before the page has
+ *   been reloaded.
+ * - Storing all keys in a single underlying key, to make the stores are atomic: either all values
+ *   are stored or none.
+ *
+ * This has two additional consequences which are convenient for our use case:
+ *
+ * - All keys share the same expiry time, removing the need to specify it repeatedly.
+ * - When multiple processes try to write the same keys, all keys are overwritten, so that we don't
+ *   have to worry about inconsistent data.
+ *
+ * @example
+ * var sessionStorage = new MemoryStorage( mw.storage.session, 'myprefix' );
+ * var localStorage = new MemoryStorage( mw.storage, 'myprefix' );
+ */
+class MemoryStorage {
+	/**
+	 * @param {mw.SafeStorage} backend
+	 * @param {string} key Key name to store under
+	 * @param {number} [expiry] Number of seconds after which this item can be deleted
+	 */
+	constructor( backend, key, expiry ) {
+		this.backend = backend;
+		this.key = key;
+		this.expiry = expiry;
+
+		this.data = this.backend.getObject( this.key ) || {};
+	}
+
+	get( key ) {
+		if ( Object.prototype.hasOwnProperty.call( this.data, key ) ) {
+			return this.data[ key ];
+		}
+		return null;
+	}
+
+	set( key, value ) {
+		this.data[ key ] = value;
+		this.backend.setObject( this.key, this.data, this.expiry );
+		return true;
+	}
+
+	remove( key ) {
+		delete this.data[ key ];
+		if ( Object.keys( this.data ).length === 0 ) {
+			this.backend.remove( this.key );
+		} else {
+			this.backend.setObject( this.key, this.data, this.expiry );
+		}
+		return true;
+	}
+
+	clear() {
+		this.data = {};
+		this.backend.remove( this.key );
+	}
+
+	// For compatibility with mw.SafeStorage API
+	getObject( key ) {
+		return this.get( key );
+	}
+
+	// For compatibility with mw.SafeStorage API
+	setObject( key, value ) {
+		return this.set( key, value );
+	}
+}
+
+module.exports = MemoryStorage;

modules/NewTopicController.js

@@ -8,8 +8,9 @@ var
  * @param {jQuery} $pageContainer Page container
  * @param {HeadingItem} threadItem
  * @param {ThreadItemSet} threadItemSet
+ * @param {MemoryStorage} storage Storage object for autosave
  */
-function NewTopicController( $pageContainer, threadItem, threadItemSet ) {
+function NewTopicController( $pageContainer, threadItem, threadItemSet, storage ) {
 	this.container = new OO.ui.PanelLayout( {
 		classes: [ 'ext-discussiontools-ui-newTopic' ],
 		expanded: false,
@@ -39,7 +40,7 @@ function NewTopicController( $pageContainer, threadItem, threadItemSet ) {
 	threadItem.range.endContainer = this.sectionTitleField.$element[ 0 ];
 	threadItem.range.endOffset = this.sectionTitleField.$element[ 0 ].childNodes.length;
 
-	NewTopicController.super.call( this, $pageContainer, threadItem, threadItemSet );
+	NewTopicController.super.call( this, $pageContainer, threadItem, threadItemSet, storage );
 }
 
 OO.inheritClass( NewTopicController, CommentController );
@@ -120,7 +121,7 @@ NewTopicController.prototype.setupReplyWidget = function ( replyWidget, data ) {
 	}
 	mw.hook( 'wikipage.content' ).fire( this.$notices );
 
-	var title = this.replyWidget.storage.get( this.replyWidget.storagePrefix + '/title' );
+	var title = this.replyWidget.storage.get( 'title' );
 	if ( title && !this.sectionTitle.getValue() ) {
 		// Don't overwrite if the user has already typed something in while the widget was loading.
 		// TODO This should happen immediately rather than waiting for the reply widget to load,
@@ -128,12 +129,12 @@ NewTopicController.prototype.setupReplyWidget = function ( replyWidget, data ) {
 		this.sectionTitle.setValue( title );
 		this.prevTitleText = title;
 
-		if ( this.replyWidget.storage.get( this.replyWidget.storagePrefix + '/summary' ) === null ) {
+		if ( this.replyWidget.storage.get( 'summary' ) === null ) {
 			var generatedSummary = this.generateSummary( title );
 			this.replyWidget.editSummaryInput.setValue( generatedSummary );
 		}
 	}
-	this.replyWidget.storage.set( this.replyWidget.storagePrefix + '/title', this.sectionTitle.getValue() );
+	this.replyWidget.storage.set( 'title', this.sectionTitle.getValue() );
 
 	if ( this.replyWidget.modeTabSelect ) {
 		// Start with the mode-select widget not-tabbable so focus will go from the title to the body
@@ -173,7 +174,7 @@ NewTopicController.prototype.clear = function () {
 NewTopicController.prototype.clearStorage = function () {
 	// This is going to get called as part of the teardown chain from replywidget
 	if ( this.replyWidget ) {
-		this.replyWidget.storage.remove( this.replyWidget.storagePrefix + '/title' );
+		this.replyWidget.storage.remove( 'title' );
 	}
 };
 
@@ -183,7 +184,7 @@ NewTopicController.prototype.storeEditSummary = function () {
 		var generatedSummary = this.generateSummary( this.sectionTitle.getValue() );
 		if ( currentSummary === generatedSummary ) {
 			// Do not store generated summaries (T315730)
-			this.replyWidget.storage.remove( this.replyWidget.storagePrefix + '/summary' );
+			this.replyWidget.storage.remove( 'summary' );
 			return;
 		}
 	}
@@ -289,7 +290,7 @@ NewTopicController.prototype.onSectionTitleChange = function () {
 	var prevTitleText = this.prevTitleText;
 
 	if ( prevTitleText !== titleText ) {
-		this.replyWidget.storage.set( this.replyWidget.storagePrefix + '/title', titleText );
+		this.replyWidget.storage.set( 'title', titleText );
 
 		var generatedSummary = this.generateSummary( titleText );
 		var generatedPrevSummary = this.generateSummary( prevTitleText );

modules/controller.js

@@ -5,8 +5,8 @@ var
 	pageThreads,
 	lastControllerScrollOffset,
 	featuresEnabled = mw.config.get( 'wgDiscussionToolsFeaturesEnabled' ) || {},
-	createMemoryStorage = require( './createMemoryStorage.js' ),
-	storage = createMemoryStorage( mw.storage ),
+	MemoryStorage = require( './MemoryStorage.js' ),
+	STORAGE_EXPIRY = 60 * 60 * 24 * 30,
 	Parser = require( './Parser.js' ),
 	ThreadItemSet = require( './ThreadItemSet.js' ),
 	CommentDetails = require( './CommentDetails.js' ),
@@ -355,18 +355,22 @@ function init( $container, state ) {
 	 * @param {string} [mode] Optionally force a mode, 'visual' or 'source'
 	 * @param {boolean} [hideErrors] Suppress errors, e.g. when restoring auto-save
 	 * @param {boolean} [suppressNotifications] Don't notify the user if recovering auto-save
+	 * @param {MemoryStorage} [storage] Storage object for autosave
 	 */
-	function setupController( comment, $link, mode, hideErrors, suppressNotifications ) {
+	function setupController( comment, $link, mode, hideErrors, suppressNotifications, storage ) {
 		var commentController, $addSectionLink;
 
+		if ( !storage ) {
+			storage = new MemoryStorage( mw.storage, 'mw-ext-DiscussionTools-reply/' + comment.id, STORAGE_EXPIRY );
+		}
 		if ( comment.id === utils.NEW_TOPIC_COMMENT_ID ) {
 			// eslint-disable-next-line no-jquery/no-global-selector
 			$addSectionLink = $( '#ca-addsection' ).find( 'a' );
 			// When opening new topic tool using any link, always activate the link in page tabs too
 			$link = $link.add( $addSectionLink );
-			commentController = new NewTopicController( $pageContainer, comment, pageThreads );
+			commentController = new NewTopicController( $pageContainer, comment, pageThreads, storage );
 		} else {
-			commentController = new CommentController( $pageContainer, comment, pageThreads );
+			commentController = new CommentController( $pageContainer, comment, pageThreads, storage );
 		}
 
 		activeCommentId = comment.id;
@@ -473,13 +477,14 @@ function init( $container, state ) {
 			return;
 		}
 
-		var mode, $link;
+		var mode, $link, storage;
 		for ( var i = 0; i < pageThreads.threadItems.length; i++ ) {
 			var comment = pageThreads.threadItems[ i ];
-			if ( storage.get( 'reply/' + comment.id + '/saveable' ) ) {
-				mode = storage.get( 'reply/' + comment.id + '/mode' );
+			storage = new MemoryStorage( mw.storage, 'mw-ext-DiscussionTools-reply/' + comment.id, STORAGE_EXPIRY );
+			if ( storage.get( 'saveable' ) ) {
+				mode = storage.get( 'mode' );
 				$link = $( commentNodes[ i ] );
-				setupController( comment, $link, mode, true, !state.firstLoad );
+				setupController( comment, $link, mode, true, !state.firstLoad, storage );
 				if ( OO.ui.isMobile() ) {
 					var urlFragment = mw.util.escapeIdForLink( comment.id );
 					// Force the section to expand on mobile (T338920)
@@ -488,12 +493,10 @@ function init( $container, state ) {
 				break;
 			}
 		}
-		if (
-			storage.get( 'reply/' + utils.NEW_TOPIC_COMMENT_ID + '/saveable' ) ||
-			storage.get( 'reply/' + utils.NEW_TOPIC_COMMENT_ID + '/title' )
-		) {
-			mode = storage.get( 'reply/' + utils.NEW_TOPIC_COMMENT_ID + '/mode' );
-			setupController( newTopicComment(), $( [] ), mode, true, !state.firstLoad );
+		storage = new MemoryStorage( mw.storage, 'mw-ext-DiscussionTools-reply/' + utils.NEW_TOPIC_COMMENT_ID, STORAGE_EXPIRY );
+		if ( storage.get( 'saveable' ) || storage.get( 'title' ) ) {
+			mode = storage.get( 'mode' );
+			setupController( newTopicComment(), $( [] ), mode, true, !state.firstLoad, storage );
 		} else if ( mw.config.get( 'wgDiscussionToolsStartNewTopicTool' ) ) {
 			var data = linksController.parseNewTopicLink( location.href );
 			setupController( newTopicComment( data ), $( [] ) );
@@ -765,7 +768,6 @@ module.exports = {
 	checkThreadItemOnPage: checkThreadItemOnPage,
 	getCheckboxesPromise: getCheckboxesPromise,
 	getApi: getApi,
-	storage: storage,
 	getReplyWidgetModules: getReplyWidgetModules,
 	defaultVisual: defaultVisual,
 	enable2017Wikitext: enable2017Wikitext

modules/createMemoryStorage.js

@@ -1,83 +0,0 @@
-/**
- * createMemoryStorage creates a wrapper around mw.SafeStorage objects, duplicating
- * their contents in memory, so that even if the underlying storage mechanism
- * fails (e.g. quota exceeded), the storage can be relied on before the
- * page has been reloaded.
- *
- * @example
- * var sessionStorage = createMemoryStorage( mw.storage.session );
- * var localStorage = createMemoryStorage( mw.storage );
- *
- * @param {mw.SafeStorage} storage
- * @return {MemoryStorage}
- */
-var createMemoryStorage = function ( storage ) {
-
-	/**
-	 * @class
-	 * @extends mw.SafeStorage
-	 *
-	 * @constructor
-	 * @param {Storage|undefined} store The Storage instance to wrap around
-	 */
-	function MemoryStorage( store ) {
-		this.data = {};
-
-		// Attempt to populate memory cache with existing data.
-		// Ignore any errors accessing native Storage object, as in mw.SafeStorage.
-		try {
-			for ( var i = 0, l = store.length; i < l; i++ ) {
-				var key = store.key( i );
-				this.data[ key ] = store.getItem( key );
-			}
-		} catch ( e ) {}
-
-		// Parent constructor
-		MemoryStorage.super.apply( this, arguments );
-	}
-
-	/* Inheritance */
-
-	var ParentStorage = storage.constructor;
-	OO.inheritClass( MemoryStorage, ParentStorage );
-
-	/* Methods */
-
-	/**
-	 * @inheritdoc
-	 */
-	MemoryStorage.prototype.get = function ( key ) {
-		if ( Object.prototype.hasOwnProperty.call( this.data, key ) ) {
-			return this.data[ key ];
-		} else {
-			// Parent method
-			return MemoryStorage.super.prototype.get.apply( this, arguments );
-		}
-	};
-
-	/**
-	 * @inheritdoc
-	 */
-	MemoryStorage.prototype.set = function ( key, value ) {
-		// Parent method
-		MemoryStorage.super.prototype.set.apply( this, arguments );
-
-		this.data[ key ] = value;
-		return true;
-	};
-
-	/**
-	 * @inheritdoc
-	 */
-	MemoryStorage.prototype.remove = function ( key ) {
-		// Parent method
-		MemoryStorage.super.prototype.remove.apply( this, arguments );
-
-		delete this.data[ key ];
-		return true;
-	};
-
-	return new MemoryStorage( storage.store );
-};
-
-module.exports = createMemoryStorage;

modules/dt.ui.ReplyWidget.js

@@ -43,9 +43,7 @@ function ReplyWidget( commentController, commentDetails, config ) {
 	this.pageExists = mw.config.get( 'wgRelevantArticleId', 0 ) !== 0;
 	var contextNode = utils.closestElement( threadItem.range.endContainer, [ 'dl', 'ul', 'ol' ] );
 	this.context = contextNode ? contextNode.tagName.toLowerCase() : 'dl';
-	// TODO: Should storagePrefix include pageName?
-	this.storagePrefix = 'reply/' + threadItem.id;
-	this.storage = controller.storage;
+	this.storage = commentController.storage;
 	// eslint-disable-next-line no-jquery/no-global-selector
 	this.contentDir = $( '#mw-content-text' ).css( 'direction' );
 	this.hideNewCommentsWarning = false;
@@ -390,7 +388,7 @@ ReplyWidget.prototype.clear = function ( preserveStorage ) {
 	if ( !preserveStorage ) {
 		this.clearStorage();
 	} else {
-		this.storage.set( this.storagePrefix + '/saveable', '1' );
+		this.storage.set( 'saveable', '1' );
 	}
 
 	this.emit( 'clear' );
@@ -400,12 +398,7 @@ ReplyWidget.prototype.clear = function ( preserveStorage ) {
  * Remove any storage that the widget is using
  */
 ReplyWidget.prototype.clearStorage = function () {
-	this.storage.remove( this.storagePrefix + '/mode' );
-	this.storage.remove( this.storagePrefix + '/saveable' );
-	this.storage.remove( this.storagePrefix + '/summary' );
-	this.storage.remove( this.storagePrefix + '/showAdvanced' );
-	this.storage.remove( this.storagePrefix + '/formToken' );
-
+	this.storage.clear();
 	this.emit( 'clearStorage' );
 };
 
@@ -498,7 +491,7 @@ ReplyWidget.prototype.toggleAdvanced = function ( showAdvanced ) {
 	this.advanced.toggle( !!this.showAdvanced );
 	this.advancedToggle.setIndicator( this.showAdvanced ? 'up' : 'down' );
 
-	this.storage.set( this.storagePrefix + '/showAdvanced', this.showAdvanced ? '1' : '' );
+	this.storage.set( 'showAdvanced', this.showAdvanced ? '1' : '' );
 };
 
 ReplyWidget.prototype.onEditSummaryChange = function () {
@@ -585,7 +578,7 @@ ReplyWidget.prototype.setup = function ( data ) {
 	}
 	this.saveEditMode( this.getMode() );
 
-	var summary = this.storage.get( this.storagePrefix + '/summary' ) || data.editSummary;
+	var summary = this.storage.get( 'summary' ) || data.editSummary;
 
 	if ( !summary ) {
 		if ( this.isNewTopic ) {
@@ -600,7 +593,7 @@ ReplyWidget.prototype.setup = function ( data ) {
 	}
 
 	this.toggleAdvanced(
-		!!this.storage.get( this.storagePrefix + '/showAdvanced' ) ||
+		!!this.storage.get( 'showAdvanced' ) ||
 		!!+mw.user.options.get( 'discussiontools-showadvanced' ) ||
 		!!data.showAdvanced
 	);
@@ -631,7 +624,7 @@ ReplyWidget.prototype.afterSetup = function () {
 	// Init preview and button state
 	this.onInputChange();
 	// Autosave
-	this.storage.set( this.storagePrefix + '/mode', this.getMode() );
+	this.storage.set( 'mode', this.getMode() );
 };
 
 /**
@@ -640,12 +633,12 @@ ReplyWidget.prototype.afterSetup = function () {
  * @return {string} Form token
  */
 ReplyWidget.prototype.getFormToken = function () {
-	var formToken = this.storage.get( this.storagePrefix + '/formToken' );
+	var formToken = this.storage.get( 'formToken' );
 	if ( !formToken ) {
 		// See ApiBase::PARAM_MAX_CHARS in ApiDiscussionToolsEdit.php
 		var maxLength = 16;
 		formToken = Math.random().toString( 36 ).slice( 2, maxLength + 2 );
-		this.storage.set( this.storagePrefix + '/formToken', formToken );
+		this.storage.set( 'formToken', formToken );
 	}
 	return formToken;
 };
@@ -771,7 +764,7 @@ ReplyWidget.prototype.onInputChange = function () {
 	}
 
 	this.updateButtons();
-	this.storage.set( this.storagePrefix + '/saveable', this.isEmpty() ? '' : '1' );
+	this.storage.set( 'saveable', this.isEmpty() ? '' : '1' );
 	this.preparePreview();
 };
 

modules/dt.ui.ReplyWidgetPlain.js

@@ -92,7 +92,7 @@ ReplyWidgetPlain.prototype.clear = function ( preserveStorage ) {
 	this.replyBodyWidget.setValue( '' );
 
 	if ( !preserveStorage ) {
-		this.storage.remove( this.storagePrefix + '/body' );
+		this.storage.remove( 'body' );
 	}
 
 	// Parent method
@@ -126,14 +126,14 @@ ReplyWidgetPlain.prototype.onInputChange = function () {
 	ReplyWidgetPlain.super.prototype.onInputChange.apply( this, arguments );
 
 	var wikitext = this.getValue();
-	this.storage.set( this.storagePrefix + '/body', wikitext );
+	this.storage.set( 'body', wikitext );
 };
 
 /**
  * @inheritdoc
  */
 ReplyWidgetPlain.prototype.setup = function ( data ) {
-	var autosaveValue = this.storage.get( this.storagePrefix + '/body' );
+	var autosaveValue = this.storage.get( 'body' );
 
 	data = data || {};
 

modules/dt.ui.ReplyWidgetVisual.js

@@ -96,8 +96,8 @@ ReplyWidgetVisual.prototype.setup = function ( data, suppressNotifications ) {
 	data = data || {};
 
 	var htmlOrDoc;
-	if ( this.storage.get( this.storagePrefix + '/saveable' ) ) {
-		htmlOrDoc = this.storage.get( this.storagePrefix + '/ve-dochtml' );
+	if ( this.storage.get( 'saveable' ) ) {
+		htmlOrDoc = this.storage.get( 've-dochtml' );
 		target.recovered = true;
 	} else {
 		htmlOrDoc = data.value;
@@ -115,16 +115,9 @@ ReplyWidgetVisual.prototype.setup = function ( data, suppressNotifications ) {
 			focus: [ 'emit', 'bodyFocus' ]
 		} );
 
-		var listStorage = ve.init.platform.createConflictableStorage( widget.storage );
-		// widget.storage is a MemoryStorage object. Copy over the .data cache so
-		// that listStorage reads from/writes to the same in-memory cache.
-		listStorage.data = widget.storage.data;
-
 		target.initAutosave( {
 			suppressNotifications: suppressNotifications,
-			docId: widget.storagePrefix,
-			storage: listStorage,
-			storageExpiry: 60 * 60 * 24 * 30
+			storage: widget.storage
 		} );
 		widget.afterSetup();