Confirmation for rendering large diffs with whole file
If the user views a very large diff while "Whole file" is enabled in
their preferences, rendering may lock up their browser. With this
change, instead of rendering, users are warned and allowed to bypass the
warning (risking browser lock up) or render with a context that is less
than whole file.
Bug: Issue 6402
Change-Id: I6e97c06598fb5f6900c925127ab6a99693b8aa7f
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 5738b40..5319ace 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -198,6 +198,18 @@
font-size: var(--font-size, 12px);
padding: 0.5em 0 0.5em 4em;
}
+ #sizeWarning {
+ display: none;
+ margin: 1em auto;
+ max-width: 60em;
+ text-align: center;
+ }
+ #sizeWarning gr-button {
+ margin: 1em;
+ }
+ #sizeWarning.warn {
+ display: block;
+ }
</style>
<style include="gr-theme-default"></style>
<div id="diffHeader" hidden$="[[_computeDiffHeaderHidden(_diffHeaderItems)]]">
@@ -232,6 +244,18 @@
</gr-diff-highlight>
</gr-diff-selection>
</div>
+ <div id="sizeWarning" class$="[[_computeWarningClass(_showWarning)]]">
+ <p>
+ Prevented render because "Whole file" is enabled and this diff is very
+ large (about [[_diffLength(_diff)]] lines).
+ </p>
+ <gr-button on-tap="_handleLimitedBypass">
+ Render with limited context
+ </gr-button>
+ <gr-button on-tap="_handleFullBypass">
+ Render anyway (may be slow)
+ </gr-button>
+ </div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
</template>
<script src="gr-diff-line.js"></script>
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 386ac08..8103cb3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -24,6 +24,10 @@
RIGHT: 'right',
};
+ const LARGE_DIFF_THRESHOLD_LINES = 10000;
+ const FULL_CONTEXT = -1;
+ const LIMITED_CONTEXT = 10;
+
Polymer({
is: 'gr-diff',
@@ -100,6 +104,19 @@
_comments: Object,
_baseImage: Object,
_revisionImage: Object,
+
+ /**
+ * Whether the safety check for large diffs when whole-file is set has
+ * been bypassed. If the value is null, then the safety has not been
+ * bypassed. If the value is a number, then that number represents the
+ * context preference to use when rendering the bypassed diff.
+ */
+ _safetyBypass: {
+ type: Number,
+ value: null,
+ },
+
+ _showWarning: Boolean,
},
listeners: {
@@ -124,6 +141,8 @@
reload() {
this.$.diffBuilder.cancel();
+ this._safetyBypass = null;
+ this._showWarning = false;
this._clearDiffContent();
const promises = [];
@@ -447,7 +466,25 @@
},
_renderDiffTable() {
- return this.$.diffBuilder.render(this._comments, this.prefs);
+ if (this.prefs.context === -1 &&
+ this._diffLength(this._diff) >= LARGE_DIFF_THRESHOLD_LINES &&
+ this._safetyBypass === null) {
+ this._showWarning = true;
+ return Promise.resolve();
+ }
+
+ this._showWarning = false;
+ return this.$.diffBuilder.render(this._comments, this._getBypassPrefs());
+ },
+
+ /**
+ * Get the preferences object including the safety bypass context (if any).
+ */
+ _getBypassPrefs() {
+ if (this._safetyBypass !== null) {
+ return Object.assign({}, this.prefs, {context: this._safetyBypass});
+ }
+ return this.prefs;
},
_clearDiffContent() {
@@ -600,5 +637,38 @@
_computeDiffHeaderHidden(items) {
return items.length === 0;
},
+
+ /**
+ * The number of lines in the diff. For delta chunks that are different
+ * sizes on the left and the right, the longer side is used.
+ * @param {!Object} diff
+ * @return {Number}
+ */
+ _diffLength(diff) {
+ return diff.content.reduce((sum, sec) => {
+ if (sec.hasOwnProperty('ab')) {
+ return sum + sec.ab.length;
+ } else {
+ return sum + Math.max(
+ sec.hasOwnProperty('a') ? sec.a.length : 0,
+ sec.hasOwnProperty('b') ? sec.b.length : 0
+ );
+ }
+ }, 0);
+ },
+
+ _handleFullBypass() {
+ this._safetyBypass = FULL_CONTEXT;
+ this._renderDiffTable();
+ },
+
+ _handleLimitedBypass() {
+ this._safetyBypass = LIMITED_CONTEXT;
+ this._renderDiffTable();
+ },
+
+ _computeWarningClass(showWarning) {
+ return showWarning ? 'warn' : '';
+ },
});
})();
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 e7b81be..921d285 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
@@ -60,6 +60,12 @@
assert.isTrue(cancelStub.called);
});
+ test('_diffLength', () => {
+ element = fixture('basic');
+ const mock = document.createElement('mock-diff-response');
+ assert.equal(element._diffLength(mock.diffResponse), 52);
+ });
+
suite('not logged in', () => {
setup(() => {
stub('gr-rest-api-interface', {
@@ -991,6 +997,48 @@
assert.equal(element._diffHeaderItems.length, 0);
});
});
+
+ suite('safety and bypass', () => {
+ let renderStub;
+
+ setup(() => {
+ element = fixture('basic');
+ renderStub = sandbox.stub(element.$.diffBuilder, 'render',
+ () => Promise.resolve());
+ const mock = document.createElement('mock-diff-response');
+ element._diff = mock.diffResponse;
+ element._comments = {left: [], right: []};
+ element.noRenderOnPrefsChange = true;
+ });
+
+ test('lage render w/ context = 10', () => {
+ element.prefs = {context: 10};
+ sandbox.stub(element, '_diffLength', () => 10000);
+ return element._renderDiffTable().then(() => {
+ assert.isTrue(renderStub.called);
+ assert.isFalse(element._showWarning);
+ });
+ });
+
+ test('lage render w/ whole file and bypass', () => {
+ element.prefs = {context: -1};
+ element._safetyBypass = 10;
+ sandbox.stub(element, '_diffLength', () => 10000);
+ return element._renderDiffTable().then(() => {
+ assert.isTrue(renderStub.called);
+ assert.isFalse(element._showWarning);
+ });
+ });
+
+ test('lage render w/ whole file and no bypass', () => {
+ element.prefs = {context: -1};
+ sandbox.stub(element, '_diffLength', () => 10000);
+ return element._renderDiffTable().then(() => {
+ assert.isFalse(renderStub.called);
+ assert.isTrue(element._showWarning);
+ });
+ });
+ });
});
a11ySuite('basic');
