Cleanup of diff comment draft saving work - Refactors the _editDraft property of gr-diff-comment to _messageText. - Renames the gr-storage element as instantiated in gr-diff-comment from "localStorage" to "storage". - The gr-storage diff comment functions are refactored to include "comment" in their names and to accept a location object rather than 4 separate arguments. - Added a comment describing the use of a promise in the _loadLocalDraft method of gr-diff-comment. - Throttled the invocation of the _cleanupDrafts method of gr-storage to avoid potentially expensive crawls over all localStorage entries. - Various other smaller fixes. Bug: Issue 3787 Change-Id: Idf96051c0d56d6ce8b15f55ca2680363bd1ca805
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html index fab0425..6e7a68a 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -129,7 +129,7 @@ class="editMessage" disabled="{{disabled}}" rows="4" - bind-value="{{_editDraft}}" + bind-value="{{_messageText}}" on-keydown="_handleTextareaKeydown"></iron-autogrow-textarea> <gr-linked-text class="message" pre @@ -141,7 +141,7 @@ <gr-button class="action done" on-tap="_handleDone">Done</gr-button> <gr-button class="action edit" on-tap="_handleEdit">Edit</gr-button> <gr-button class="action save" on-tap="_handleSave" - disabled$="[[_computeSaveDisabled(_editDraft)]]">Save</gr-button> + disabled$="[[_computeSaveDisabled(_messageText)]]">Save</gr-button> <gr-button class="action cancel" on-tap="_handleCancel" hidden>Cancel</gr-button> <div class="danger"> <gr-button class="action discard" on-tap="_handleDiscard">Discard</gr-button> @@ -149,7 +149,7 @@ </div> </div> <gr-rest-api-interface id="restAPI"></gr-rest-api-interface> - <gr-storage id="localStorage"></gr-storage> + <gr-storage id="storage"></gr-storage> </template> <script src="gr-diff-comment.js"></script> </dom-module>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js index c28d0ac..f7b5f01 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -69,25 +69,29 @@ projectConfig: Object, _xhrPromise: Object, // Used for testing. - _editDraft: { + _messageText: { type: String, - observer: '_editDraftChanged', + observer: '_messageTextChanged', }, }, ready: function() { this._loadLocalDraft().then(function(loadedLocal) { - this._editDraft = (this.comment && this.comment.message) || ''; - this.editing = !this._editDraft.length || loadedLocal; + this._messageText = (this.comment && this.comment.message) || ''; + this.editing = !this._messageText.length || loadedLocal; }.bind(this)); }, save: function() { - this.comment.message = this._editDraft; + this.comment.message = this._messageText; this.disabled = true; - this.$.localStorage.eraseDraft(this.changeNum, this.patchNum, - this.comment.path, this.comment.line); + this.$.storage.eraseDraftComment({ + changeNum: this.changeNum, + patchNum: this.patchNum, + path: this.comment.path, + line: this.comment.line, + }); this._xhrPromise = this._saveDraft(this.comment).then(function(response) { this.disabled = false; @@ -147,23 +151,27 @@ } }, - _editDraftChanged: function(newValue, oldValue) { + _messageTextChanged: function(newValue, oldValue) { if (this.comment && this.comment.id) { return; } this.debounce('store', function() { - var message = this._editDraft; + var message = this._messageText; - // If the draft has been modified to be empty, then erase the storage - // entry. - if ((!this._editDraft || !this._editDraft.length) && oldValue) { - this.$.localStorage.eraseDraft(this.changeNum, this.patchNum, - this.comment.path, this.comment.line); - return; + var commentLocation = { + changeNum: this.changeNum, + patchNum: this.patchNum, + path: this.comment.path, + line: this.comment.line, + }; + + if ((!this._messageText || !this._messageText.length) && oldValue) { + // If the draft has been modified to be empty, then erase the storage + // entry. + this.$.storage.eraseDraftComment(commentLocation); + } else { + this.$.storage.setDraftComment(commentLocation, message); } - - this.$.localStorage.setDraft(this.changeNum, this.patchNum, - this.comment.path, this.comment.line, message); - }.bind(this), STORAGE_DEBOUNCE_INTERVAL); + }, STORAGE_DEBOUNCE_INTERVAL); }, _handleLinkTap: function(e) { @@ -195,7 +203,7 @@ _handleEdit: function(e) { this._preventDefaultAndBlur(e); - this._editDraft = this.comment.message; + this._messageText = this.comment.message; this.editing = true; }, @@ -210,7 +218,7 @@ this.fire('comment-discard'); return; } - this._editDraft = this.comment.message; + this._messageText = this.comment.message; this.editing = false; }, @@ -252,6 +260,8 @@ }, _loadLocalDraft: function() { + // Use an async promise to avoid blocking render on potentially slow + // localStorage calls. return new Promise(function(resolve) { this.async(function() { // Only apply local drafts to comments that haven't been saved @@ -261,8 +271,12 @@ return; } - var draft = this.$.localStorage.getDraft(this.changeNum, - this.patchNum, this.comment.path, this.comment.line); + var draft = this.$.storage.getDraftComment({ + changeNum: this.changeNum, + patchNum: this.patchNum, + path: this.comment.path, + line: this.comment.line, + }); if (draft) { this.comment.message = draft.message;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html index a333e14..8ef222e 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -183,11 +183,11 @@ MockInteractions.tap(element.$$('.edit')); assert.isTrue(element.editing); - element._editDraft = ''; + element._messageText = ''; // Save should be disabled on an empty message. var disabled = element.$$('.save').hasAttribute('disabled'); assert.isTrue(disabled, 'save button should be disabled.'); - element._editDraft = ' '; + element._messageText = ' '; disabled = element.$$('.save').hasAttribute('disabled'); assert.isTrue(disabled, 'save button should be disabled.'); @@ -206,7 +206,7 @@ test('draft saving/editing', function(done) { element.draft = true; MockInteractions.tap(element.$$('.edit')); - element._editDraft = 'good news, everyone!'; + element._messageText = 'good news, everyone!'; MockInteractions.tap(element.$$('.save')); assert.isTrue(element.disabled, 'Element should be disabled when creating draft.'); @@ -218,7 +218,7 @@ assert.isFalse(element.editing); }).then(function() { MockInteractions.tap(element.$$('.edit')); - element._editDraft = 'You’ll be delivering a package to Chapek 9, a ' + + element._messageText = 'You’ll be delivering a package to Chapek 9, a ' + 'world where humans are killed on sight.'; MockInteractions.tap(element.$$('.save')); assert.isTrue(element.disabled,
diff --git a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html index e877aec..74bfcdf 100644 --- a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html +++ b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html
@@ -15,6 +15,5 @@ --> <link rel="import" href="../../../bower_components/polymer/polymer.html"> <dom-module id="gr-storage"> - <template></template> <script src="gr-storage.js"></script> </dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js index 79012d4..ef3a6c0 100644 --- a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js +++ b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
@@ -17,10 +17,14 @@ // Date cutoff is one day: var DRAFT_MAX_AGE = 24*60*60*1000; + // Clean up old entries no more frequently than one day. + var CLEANUP_THROTTLE_INTERVAL = 24*60*60*1000; + Polymer({ is: 'gr-storage', properties: { + _lastCleanup: Number, _storage: { type: Object, value: function() { @@ -29,27 +33,34 @@ }, }, - getDraft: function(changeNum, patchNum, path, line) { + getDraftComment: function(location) { this._cleanupDrafts(); - return this._getObject( - this._getDraftKey(changeNum, patchNum, path, line)); + return this._getObject(this._getDraftKey(location)); }, - setDraft: function(changeNum, patchNum, path, line, message) { - var key = this._getDraftKey(changeNum, patchNum, path, line); + setDraftComment: function(location, message) { + var key = this._getDraftKey(location); this._setObject(key, {message: message, updated: Date.now()}); }, - eraseDraft: function(changeNum, patchNum, path, line) { - var key = this._getDraftKey(changeNum, patchNum, path, line); + eraseDraftComment: function(location) { + var key = this._getDraftKey(location); this._storage.removeItem(key); }, - _getDraftKey: function(changeNum, patchNum, path, line) { - return ['draft', changeNum, patchNum, path, line].join(':'); + _getDraftKey: function(location) { + return ['draft', location.changeNum, location.patchNum, location.path, + location.line].join(':'); }, _cleanupDrafts: function() { + // Throttle cleanup to the throttle interval. + if (this._lastCleanup && + Date.now() - this._lastCleanup < CLEANUP_THROTTLE_INTERVAL) { + return; + } + this._lastCleanup = Date.now(); + var draft; for (var key in this._storage) { if (key.indexOf('draft:') === 0) {
diff --git a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html index d21a6c4..4e17cb6 100644 --- a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html +++ b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html
@@ -51,23 +51,29 @@ var patchNum = 5; var path = 'my_source_file.js'; var line = 123; + var location = { + changeNum: changeNum, + patchNum: patchNum, + path: path, + line: line, + }; // The key is in the expected format. - var key = element._getDraftKey(changeNum, patchNum, path, line); + var key = element._getDraftKey(location); assert.equal(key, ['draft', changeNum, patchNum, path, line].join(':')); // There should be no draft initially. - var draft = element.getDraft(changeNum, patchNum, path, line); + var draft = element.getDraftComment(location); assert.isNotOk(draft); // Setting the draft stores it under the expected key. - element.setDraft(changeNum, patchNum, path, line, 'my comment'); + element.setDraftComment(location, 'my comment'); assert.isOk(storage.getItem(key)); assert.equal(JSON.parse(storage.getItem(key)).message, 'my comment'); assert.isOk(JSON.parse(storage.getItem(key)).updated); // Erasing the draft removes the key. - element.eraseDraft(changeNum, patchNum, path, line); + element.eraseDraftComment(location); assert.isNotOk(storage.getItem(key)); cleanupStorage(); @@ -78,7 +84,16 @@ var patchNum = 5; var path = 'my_source_file.js'; var line = 123; - var key = element._getDraftKey(changeNum, patchNum, path, line); + var location = { + changeNum: changeNum, + patchNum: patchNum, + path: path, + line: line, + }; + var key = element._getDraftKey(location); + + // Make sure that the call to cleanup doesn't get throttled. + element._lastCleanup = 0; var cleanupSpy = sinon.spy(element, '_cleanupDrafts'); @@ -89,7 +104,7 @@ })); // Getting the draft should cause it to be removed. - var draft = element.getDraft(changeNum, patchNum, path, line); + var draft = element.getDraftComment(location); assert.isTrue(cleanupSpy.called); assert.isNotOk(draft);