From fcc3cf8c1d76cfc4acfed253c8b65e03c06213cc Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Wed, 7 Feb 2018 11:06:19 -0800 Subject: [PATCH] Cancel diff rendering when diffs are collapsed or detached Formerly, if diff rendering is started, but then unloaded or navigated away from, the rendering work would needlessly continue in the background. Cancel rendering when a diff will not be used. Bug: Issue 6061 Change-Id: I695281229335e84f2d5c802c1fa971842c588330 --- .../async-foreach-behavior.html | 15 +++++++++++++-- .../async-foreach-behavior_test.html | 14 ++++++++++++++ .../change/gr-file-list/gr-file-list.js | 19 ++++++++++++++++++- .../gr-file-list/gr-file-list_test.html | 11 +++++++++++ .../app/elements/diff/gr-diff/gr-diff.js | 7 ++++++- .../elements/diff/gr-diff/gr-diff_test.html | 9 +++++++-- 6 files changed, 69 insertions(+), 6 deletions(-) diff --git a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior.html b/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior.html index 01483772bb..d6d5a864f7 100644 --- a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior.html +++ b/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior.html @@ -25,12 +25,23 @@ limitations under the License. /** * @template T * @param {!Array} array - * @param {!Function} fn + * @param {!Function} fn An iteratee function to be passed each element of + * the array in order. Must return a promise, and the following + * iteration will not begin until resolution of the promise returned by + * the previous iteration. + * + * An optional second argument to fn is a callback that will halt the + * loop if called. * @return {!Promise} */ asyncForeach(array, fn) { if (!array.length) { return Promise.resolve(); } - return fn(array[0]).then(() => this.asyncForeach(array.slice(1), fn)); + let stop = false; + const stopCallback = () => { stop = true; }; + return fn(array[0], stopCallback).then(exit => { + if (stop) { return Promise.resolve(); } + return this.asyncForeach(array.slice(1), fn); + }); }, }; })(window); diff --git a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior_test.html b/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior_test.html index ba15ad72c7..cbc7056072 100644 --- a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior_test.html +++ b/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior_test.html @@ -35,5 +35,19 @@ limitations under the License. assert.equal(fn.getCall(2).args[0], 3); }); }); + + test('halts on stop condition', () => { + const stub = sinon.stub(); + const fn = (e, stop) => { + stub(e); + stop(); + return Promise.resolve(); + }; + return Gerrit.AsyncForeachBehavior.asyncForeach([1, 2, 3], fn) + .then(() => { + assert.isTrue(stub.calledOnce); + assert.equal(stub.lastCall.args[0], 1); + }); + }); }); diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js index b42824cd98..2c686c33bb 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js @@ -137,6 +137,9 @@ value: true, computed: '_computeShowSizeBars(_userPrefs)', }, + + /** @type {Function} */ + _cancelForEachDiff: Function, }, behaviors: [ @@ -174,6 +177,10 @@ keydown: '_scopedKeydownHandler', }, + detached() { + this._cancelDiffs(); + }, + /** * Iron-a11y-keys-behavior catches keyboard events globally. Some keyboard * events must be scoped to a component level (e.g. `enter`) in order to not @@ -849,6 +856,7 @@ _clearCollapsedDiffs(collapsedDiffs) { for (const diff of collapsedDiffs) { + diff.cancel(); diff.clearDiffContent(); } }, @@ -871,7 +879,9 @@ return (new Promise(resolve => { this.fire('reload-drafts', {resolve}); })).then(() => { - return this.asyncForeach(paths, path => { + return this.asyncForeach(paths, (path, cancel) => { + this._cancelForEachDiff = cancel; + iter++; console.log('Expanding diff', iter, 'of', initialCount, ':', path); @@ -884,6 +894,7 @@ } return Promise.all(promises); }).then(() => { + this._cancelForEachDiff = null; this._nextRenderParams = null; console.log('Finished expanding', initialCount, 'diff(s)'); this.$.reporting.timeEnd(timerName); @@ -892,6 +903,12 @@ }); }, + /** Cancel the rendering work of every diff in the list */ + _cancelDiffs() { + if (this._cancelForEachDiff) { this._cancelForEachDiff(); } + this._forEachDiff(d => d.cancel()); + }, + /** * In the given NodeList of diff elements, find the diff for the given path. * @param {string} path diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html index ffd91a4c0a..d5f8d626ce 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html @@ -851,6 +851,7 @@ limitations under the License. reload() { done(); }, + cancel() {}, }]; sinon.stub(element, 'diffs', { get() { return diffs; }, @@ -858,6 +859,16 @@ limitations under the License. element.push('_expandedFilePaths', path); }); + test('_clearCollapsedDiffs', () => { + const diff = { + cancel: sinon.stub(), + clearDiffContent: sinon.stub(), + }; + element._clearCollapsedDiffs([diff]); + assert.isTrue(diff.cancel.calledOnce); + assert.isTrue(diff.clearDiffContent.calledOnce); + }); + test('filesExpanded value updates to correct enum', () => { element._files = [{__path: 'foo.bar'}, {__path: 'baz.bar'}]; flushAsynchronousOperations(); diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js index 4ad35fcd2f..00a2c70906 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js @@ -182,7 +182,7 @@ /** @return {!Promise} */ reload() { - this.$.diffBuilder.cancel(); + this.cancel(); this.clearBlame(); this._safetyBypass = null; this._showWarning = false; @@ -203,6 +203,11 @@ }); }, + /** Cancel any remaining diff builder rendering work. */ + cancel() { + this.$.diffBuilder.cancel(); + }, + /** @return {!Array} */ getCursorStops() { if (this.hidden && this.noAutoRender) { diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html index 5b3670b582..91e3b97f76 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html @@ -49,7 +49,7 @@ limitations under the License. test('reload cancels before network resolves', () => { element = fixture('basic'); - const cancelStub = sandbox.stub(element.$.diffBuilder, 'cancel'); + const cancelStub = sandbox.stub(element, 'cancel'); // Stub the network calls into requests that never resolve. sandbox.stub(element, '_getDiff', () => new Promise(() => {})); @@ -58,13 +58,18 @@ limitations under the License. assert.isTrue(cancelStub.called); }); + test('cancel', () => { + const cancelStub = sandbox.stub(element.$.diffBuilder, 'cancel'); + element.cancel(); + assert.isTrue(cancelStub.calledOnce); + }); + test('_diffLength', () => { element = fixture('basic'); const mock = document.createElement('mock-diff-response'); assert.equal(element._diffLength(mock.diffResponse), 52); }); - suite('_get{PatchNum|IsParentComment}ByLineAndContent', () => { let lineEl; let contentEl;