opendev/gerrit
Code Issues Proposed changes

Lift gr-syntax-layer into gr-diff-host

Browse Source 
This change decouples gr-diff-builder and gr-syntax-layer. gr-syntax-
layer will now live in gr-diff-host.
As a result, from the perspective of gr-diff/gr-diff-builder, plugin
layers and the syntax layer will now be part of the same list.

(This change fixes a time reporting regression in https://gerrit-review.googlesource.com/c/gerrit/+/239972.)

A future potential refactor could also lift gr-coverage-layer instances
in a similar fashion.

Change-Id: I47e91fdac00e17460aab9ca57e4569ade63c2eeb
This commit is contained in:
Mayank Kumar
2019-10-03 18:04:08 -07:00
parent 897a9c218a
commit 41b0f951fc
9 changed files with 245 additions and 147 deletions
Download Patch File Download Diff File Expand all files Collapse all files

75
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
View File

@@ -20,7 +20,6 @@ limitations under the License.
<link rel="import" href="../gr-coverage-layer/gr-coverage-layer.html">
<link rel="import" href="../gr-diff-processor/gr-diff-processor.html">
<link rel="import" href="../gr-ranged-comment-layer/gr-ranged-comment-layer.html">
<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
<dom-module id="gr-diff-builder">
<template>
@@ -30,9 +29,6 @@ limitations under the License.
<gr-ranged-comment-layer
id="rangeLayer"
comment-ranges="[[commentRanges]]"></gr-ranged-comment-layer>
<gr-syntax-layer
id="syntaxLayer"
diff="[[diff]]"></gr-syntax-layer>
<gr-coverage-layer
id="coverageLayerLeft"
coverage-ranges="[[_leftCoverageRanges]]"
@@ -64,13 +60,6 @@ limitations under the License.
UNIFIED: 'UNIFIED_DIFF',
};
// If any line of the diff is more than the character limit, then disable
// syntax highlighting for the entire file.
const SYNTAX_MAX_LINE_LENGTH = 500;
// Disable syntax highlighting if the overall diff is too large.
const SYNTAX_MAX_DIFF_LENGTH = 20000;
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
Polymer({
@@ -84,18 +73,11 @@ limitations under the License.
*/
/**
* Fired when the diff finishes rendering text content and starts
* syntax highlighting.
* Fired when the diff finishes rendering text content.
*
* @event render-content
*/
/**
* Fired when the diff finishes syntax highlighting.
*
* @event render-syntax
*/
properties: {
diff: Object,
diffPath: String,
@@ -138,7 +120,7 @@ limitations under the License.
* @type {?Object}
*/
_cancelableRenderPromise: Object,
pluginLayers: {
layers: {
type: Array,
value: [],
},
@@ -171,11 +153,10 @@ limitations under the License.
// attached before plugins are installed.
this._setupAnnotationLayers();
this.$.syntaxLayer.enabled = prefs.syntax_highlighting;
this._showTabs = !!prefs.show_tabs;
this._showTrailingWhitespace = !!prefs.show_whitespace_errors;
// Stop the processor and syntax layer (if they're running).
// Stop the processor if it's running.
this.cancel();
this._builder = this._getDiffBuilder(this.diff, prefs);
@@ -198,16 +179,6 @@ limitations under the License.
}
this.dispatchEvent(new CustomEvent('render-content',
{bubbles: true, composed: true}));
if (this._diffTooLargeForSyntax()) {
this.$.syntaxLayer.enabled = false;
}
return this.$.syntaxLayer.process();
})
.then(() => {
this.dispatchEvent(new CustomEvent(
'render-syntax', {bubbles: true, composed: true}));
}));
return this._cancelableRenderPromise
.finally(() => { this._cancelableRenderPromise = null; })
@@ -220,7 +191,6 @@ limitations under the License.
_setupAnnotationLayers() {
const layers = [
this._createTrailingWhitespaceLayer(),
this.$.syntaxLayer,
this._createIntralineLayer(),
this._createTabIndicatorLayer(),
this.$.rangeLayer,
@@ -228,8 +198,8 @@ limitations under the License.
this.$.coverageLayerRight,
];
if (this.pluginLayers) {
layers.push(...this.pluginLayers);
if (this.layers) {
layers.push(...this.layers);
}
this._layers = layers;
},
@@ -307,7 +277,6 @@ limitations under the License.
cancel() {
this.$.processor.cancel();
this.$.syntaxLayer.cancel();
if (this._cancelableRenderPromise) {
this._cancelableRenderPromise.cancel();
this._cancelableRenderPromise = null;
@@ -446,44 +415,10 @@ limitations under the License.
};
},
/**
* @return {boolean} whether any of the lines in _groups are longer
* than SYNTAX_MAX_LINE_LENGTH.
*/
_anyLineTooLong() {
return this._groups.reduce((acc, group) => {
return acc || group.lines.reduce((acc, line) => {
return acc || line.text.length >= SYNTAX_MAX_LINE_LENGTH;
}, false);
}, false);
},
_diffTooLargeForSyntax() {
return this._anyLineTooLong() ||
this.getDiffLength() > SYNTAX_MAX_DIFF_LENGTH;
},
setBlame(blame) {
if (!this._builder || !blame) { return; }
this._builder.setBlame(blame);
},
/**
* Get the approximate length of the diff as the sum of the maximum
* length of the chunks.
* @return {number}
*/
getDiffLength() {
return this.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);
},
});
})();
</script>

54
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
View File

@@ -590,38 +590,38 @@ limitations under the License.
});
});
suite('layers from plugins', () => {
suite('layers', () => {
let element;
let initialLayersCount;
let withPluginLayerCount;
let withLayerCount;
setup(() => {
const pluginLayers = [];
const layers = [];
element = fixture('basic');
element.pluginLayers = pluginLayers;
element.layers = layers;
element._showTrailingWhitespace = true;
element._setupAnnotationLayers();
initialLayersCount = element._layers.length;
});
test('no plugin layers', () => {
test('no layers', () => {
element._setupAnnotationLayers();
assert.equal(element._layers.length, initialLayersCount);
});
suite('with plugin layers', () => {
const pluginLayers = [{}, {}];
suite('with layers', () => {
const layers = [{}, {}];
setup(() => {
element = fixture('basic');
element.pluginLayers = pluginLayers;
element.layers = layers;
element._showTrailingWhitespace = true;
element._setupAnnotationLayers();
withPluginLayerCount = element._layers.length;
withLayerCount = element._layers.length;
});
test('with plugin layers', () => {
test('with layers', () => {
element._setupAnnotationLayers();
assert.equal(element._layers.length, withPluginLayerCount);
assert.equal(initialLayersCount + pluginLayers.length,
withPluginLayerCount);
assert.equal(element._layers.length, withLayerCount);
assert.equal(initialLayersCount + layers.length,
withLayerCount);
});
});
});
@@ -733,7 +733,6 @@ limitations under the License.
element.viewMode = 'SIDE_BY_SIDE';
processStub = sandbox.stub(element.$.processor, 'process')
.returns(Promise.resolve());
sandbox.stub(element, '_anyLineTooLong').returns(true);
keyLocations = {left: {}, right: {}};
prefs = {
line_length: 10,
@@ -862,37 +861,14 @@ limitations under the License.
.map(c => { return c.args[0].type; });
assert.include(firedEventTypes, 'render-start');
assert.include(firedEventTypes, 'render-content');
assert.include(firedEventTypes, 'render-syntax');
done();
});
});
test('rendering normal-sized diff does not disable syntax', () => {
assert.isTrue(element.$.syntaxLayer.enabled);
});
test('rendering large diff disables syntax', done => {
// Before it renders, set the first diff line to 500 '*' characters.
element.diff.content[0].a = [new Array(501).join('*')];
const prefs = {
line_length: 10,
show_tabs: true,
tab_size: 4,
context: -1,
syntax_highlighting: true,
};
element.render(keyLocations, prefs).then(() => {
assert.isFalse(element.$.syntaxLayer.enabled);
done();
});
});
test('cancel', () => {
const processorCancelStub = sandbox.stub(element.$.processor, 'cancel');
const syntaxCancelStub = sandbox.stub(element.$.syntaxLayer, 'cancel');
element.cancel();
assert.isTrue(processorCancelStub.called);
assert.isTrue(syntaxCancelStub.called);
});
});
@@ -921,10 +897,6 @@ limitations under the License.
});
});
test('getDiffLength', () => {
assert.equal(element.getDiffLength(diff), 52);
});
test('getContentByLine', () => {
let actual;

10
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
View File

@@ -23,6 +23,7 @@ limitations under the License.
<link rel="import" href="../../shared/gr-comment-thread/gr-comment-thread.html">
<link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
<link rel="import" href="../gr-diff/gr-diff.html">
<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
<dom-module id="gr-diff-host">
<template>
@@ -49,8 +50,13 @@ limitations under the License.
revision-image=[[_revisionImage]]
coverage-ranges="[[_coverageRanges]]"
blame="[[_blame]]"
plugin-layers="[[pluginLayers]]"
diff="[[diff]]"></gr-diff>
layers="[[_layers]]"
diff="[[diff]]">
</gr-diff>
<gr-syntax-layer
id="syntaxLayer"
enabled="[[_syntaxHighlightingEnabled]]"
diff="[[diff]]"></gr-syntax-layer>
<gr-js-api-interface id="jsAPI"></gr-js-api-interface>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-reporting id="reporting" category="diff"></gr-reporting>

63
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
View File

@@ -35,6 +35,13 @@
SYNTAX: 'Diff Syntax Render',
};
// Disable syntax highlighting if the overall diff is too large.
const SYNTAX_MAX_DIFF_LENGTH = 20000;
// If any line of the diff is more than the character limit, then disable
// syntax highlighting for the entire file.
const SYNTAX_MAX_LINE_LENGTH = 500;
const WHITESPACE_IGNORE_NONE = 'IGNORE_NONE';
/**
@@ -210,7 +217,13 @@
computed: '_computeParentIndex(patchRange.*)',
},
pluginLayers: {
_syntaxHighlightingEnabled: {
type: Boolean,
computed:
'_isSyntaxHighlightingEnabled(prefs.syntax_highlighting, _diff)',
},
_layers: {
type: Array,
value: [],
},
@@ -235,7 +248,6 @@
'render-start': '_handleRenderStart',
'render-content': '_handleRenderContent',
'render-syntax': '_handleRenderSyntax',
'normalize-range': '_handleNormalizeRange',
},
@@ -263,13 +275,13 @@
this._errorMessage = null;
const whitespaceLevel = this._getIgnoreWhitespace();
const pluginLayers = [];
const layers = [this.$.syntaxLayer];
// Get layers from plugins (if any).
for (const pluginLayer of this.$.jsAPI.getDiffLayers(
this.diffPath, this.changeNum, this.patchNum)) {
pluginLayers.push(pluginLayer);
layers.push(pluginLayer);
}
this.push('pluginLayers', ...pluginLayers);
this._layers = layers;
this._coverageRanges = [];
const {changeNum, path, patchRange: {basePatchNum, patchNum}} = this;
@@ -312,8 +324,20 @@
}
this.filesWeblinks = this._getFilesWeblinks(diff);
return new Promise(resolve => {
const callback = () => {
resolve();
const callback = event => {
const needsSyntaxHighlighting = event.detail
&& event.detail.contentRendered;
if (needsSyntaxHighlighting) {
this.$.reporting.time(TimingLabel.SYNTAX);
this.$.syntaxLayer.process().then(() => {
this.$.reporting.timeEnd(TimingLabel.SYNTAX);
this.$.reporting.timeEnd(TimingLabel.TOTAL);
resolve();
});
} else {
this.$.reporting.timeEnd(TimingLabel.TOTAL);
resolve();
}
this.removeEventListener('render', callback);
};
this.addEventListener('render', callback);
@@ -856,6 +880,25 @@
item => item.__draftID === comment.__draftID);
},
_isSyntaxHighlightingEnabled(preference, diff) {
if (!preference) return false;
return !this._anyLineTooLong(diff) &&
this.$.diff.getDiffLength(diff) <= SYNTAX_MAX_DIFF_LENGTH;
},
/**
* @return {boolean} whether any of the lines in diff are longer
* than SYNTAX_MAX_LINE_LENGTH.
*/
_anyLineTooLong(diff) {
return diff.content.some(section => {
const lines = section.ab ?
section.ab :
(section.a || []).concat(section.b || []);
return lines.some(line => line.length >= SYNTAX_MAX_LINE_LENGTH);
});
},
_handleRenderStart() {
this.$.reporting.time(TimingLabel.TOTAL);
this.$.reporting.time(TimingLabel.CONTENT);
@@ -863,12 +906,6 @@
_handleRenderContent() {
this.$.reporting.timeEnd(TimingLabel.CONTENT);
this.$.reporting.time(TimingLabel.SYNTAX);
},
_handleRenderSyntax() {
this.$.reporting.timeEnd(TimingLabel.SYNTAX);
this.$.reporting.timeEnd(TimingLabel.TOTAL);
},
_handleNormalizeRange(event) {

135
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
View File

@@ -60,7 +60,7 @@ limitations under the License.
suite('plugin layers', () => {
const pluginLayers = [{}, {}];
const pluginLayers = [{annotate: () => {}}, {annotate: () => {}}];
setup(() => {
stub('gr-js-api-interface', {
getDiffLayers() { return pluginLayers; },
@@ -302,24 +302,81 @@ limitations under the License.
done();
});
test('ends content and starts syntax timer on render-content', done => {
test('ends content timer on render-content', () => {
element.dispatchEvent(
new CustomEvent('render-content', {bubbles: true, composed: true}));
assert.isTrue(element.$.reporting.time.calledWithExactly(
'Diff Syntax Render'));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Content Render'));
done();
});
test('ends total and syntax timer on render-syntax', done => {
element.dispatchEvent(
new CustomEvent('render-syntax', {bubbles: true, composed: true}));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Total Render'));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Syntax Render'));
done();
test('ends total and syntax timer after syntax layer processing', done => {
let notifySyntaxProcessed;
sandbox.stub(element.$.syntaxLayer, 'process').returns(new Promise(
resolve => {
notifySyntaxProcessed = resolve;
}));
sandbox.stub(element.$.restAPI, 'getDiff').returns(
Promise.resolve({content: []}));
element.patchRange = {};
element.$.restAPI.getDiffPreferences().then(prefs => {
element.prefs = prefs;
return element.reload();
});
// Multiple cascading microtasks are scheduled.
setTimeout(() => {
notifySyntaxProcessed();
// Assert after the notification task is processed.
Promise.resolve().then(() => {
assert.isTrue(element.$.reporting.timeEnd.calledThrice);
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Total Render'));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Syntax Render'));
done();
});
});
});
test('ends total timer w/ no syntax layer processing', done => {
sandbox.stub(element.$.restAPI, 'getDiff').returns(
Promise.resolve({content: []}));
element.patchRange = {};
element.reload();
// Multiple cascading microtasks are scheduled.
setTimeout(() => {
assert.isTrue(element.$.reporting.timeEnd.calledOnce);
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Total Render'));
done();
});
});
test('completes reload promise after syntax layer processing', done => {
let notifySyntaxProcessed;
sandbox.stub(element.$.syntaxLayer, 'process').returns(new Promise(
resolve => {
notifySyntaxProcessed = resolve;
}));
sandbox.stub(element.$.restAPI, 'getDiff').returns(
Promise.resolve({content: []}));
element.patchRange = {};
let reloadComplete = false;
element.$.restAPI.getDiffPreferences().then(prefs => {
element.prefs = prefs;
return element.reload();
}).then(() => {
reloadComplete = true;
});
// Multiple cascading microtasks are scheduled.
setTimeout(() => {
assert.isFalse(reloadComplete);
notifySyntaxProcessed();
// Assert after the notification task is processed.
setTimeout(() => {
assert.isTrue(reloadComplete);
done();
});
});
});
});
@@ -1285,5 +1342,57 @@ limitations under the License.
assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
Gerrit.DiffSide.RIGHT), [r]);
});
suite('syntax layer', () => {
setup(() => {
const prefs = {
line_length: 10,
show_tabs: true,
tab_size: 4,
context: -1,
syntax_highlighting: true,
};
element.prefs = prefs;
});
test('gr-diff-host provides syntax highlighting layer to gr-diff', () => {
element.patchRange = {};
element.reload();
assert.equal(element.$.diff.layers[0], element.$.syntaxLayer);
});
test('rendering normal-sized diff does not disable syntax', () => {
element._diff = {
content: [{
a: ['foo'],
}],
};
assert.isTrue(element.$.syntaxLayer.enabled);
});
test('rendering large diff disables syntax', () => {
// Before it renders, set the first diff line to 500 '*' characters.
element._diff = {
content: [{
a: [new Array(501).join('*')],
}],
};
assert.isFalse(element.$.syntaxLayer.enabled);
});
test('starts syntax layer processing on render event', done => {
sandbox.stub(element.$.syntaxLayer, 'process').returns(Promise.resolve());
sandbox.stub(element.$.restAPI, 'getDiff').returns(
Promise.resolve({content: []}));
element.patchRange = {};
element.reload();
setTimeout(() => {
element.dispatchEvent(
new CustomEvent('render', {bubbles: true, composed: true}));
assert.isTrue(element.$.syntaxLayer.process.called);
done();
});
});
});
});
</script>

2
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
View File

@@ -378,7 +378,7 @@ limitations under the License.
line-wrapping="[[lineWrapping]]"
is-image-diff="[[isImageDiff]]"
base-image="[[baseImage]]"
plugin-layers="[[pluginLayers]]"
layers="[[layers]]"
revision-image="[[revisionImage]]">
<table
id="diffTable"

28
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
View File

@@ -271,7 +271,7 @@
/** Set by Polymer. */
isAttached: Boolean,
pluginLayers: Array,
layers: Array,
},
behaviors: [
@@ -724,7 +724,7 @@
_diffChanged(newValue) {
if (newValue) {
this._diffLength = this.$.diffBuilder.getDiffLength();
this._diffLength = this.getDiffLength(newValue);
this._debounceRenderDiffTable();
}
},
@@ -766,7 +766,11 @@
this.$.diffBuilder.render(keyLocations, this._getBypassPrefs())
.then(() => {
this.dispatchEvent(
new CustomEvent('render', {bubbles: true, composed: true}));
new CustomEvent('render', {
bubbles: true,
composed: true,
detail: {contentRendered: true},
}));
});
},
@@ -955,5 +959,23 @@
if (loading || !warning) { return 'newlineWarning hidden'; }
return 'newlineWarning';
},
/**
* Get the approximate length of the diff as the sum of the maximum
* length of the chunks.
* @param {Object} diff object
* @return {number}
*/
getDiffLength(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);
},
});
})();

19
polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
View File

@@ -767,7 +767,7 @@ limitations under the License.
new CustomEvent('render', {bubbles: true, composed: true}));
});
const mock = document.createElement('mock-diff-response');
sandbox.stub(element.$.diffBuilder, 'getDiffLength').returns(10000);
sandbox.stub(element, 'getDiffLength').returns(10000);
element.diff = mock.diffResponse;
element.noRenderOnPrefsChange = true;
});
@@ -1101,6 +1101,23 @@ limitations under the License.
));
});
});
test('getDiffLength', () => {
const diff = document.createElement('mock-diff-response').diffResponse;
assert.equal(element.getDiffLength(diff), 52);
});
test('`render` event has contentRendered field in detail', done => {
element = fixture('basic');
element.prefs = {};
renderStub = sandbox.stub(element.$.diffBuilder, 'render')
.returns(Promise.resolve());
element.addEventListener('render', event => {
assert.isTrue(event.detail.contentRendered);
done();
});
element._renderDiffTable();
});
});
a11ySuite('basic');

6
polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
View File

@@ -225,7 +225,7 @@
process() {
// Cancel any still running process() calls, because they append to the
// same _baseRanges and _revisionRanges fields.
this.cancel();
this._cancel();
// Discard existing ranges.
this._baseRanges = [];
@@ -295,7 +295,7 @@
/**
* Cancel any asynchronous syntax processing jobs.
*/
cancel() {
_cancel() {
if (this._processHandle != null) {
this.cancelAsync(this._processHandle);
this._processHandle = null;
@@ -306,7 +306,7 @@
},
_diffChanged() {
this.cancel();
this._cancel();
this._baseRanges = [];
this._revisionRanges = [];
},