Fix loading of plugin provided coverage layer
getDiffLayers was only called once during diff initialization, and it
did not wait for plugin to be loaded. So sometimes coverage plugins
would be too late to register their annotation layer.
We change the order of how diff loading is organized:
- Defer layer initialization until after plugins are loaded.
- Wait for plugins to be loaded in parallel with diff content being
loaded.
- Only start rendering after everything is completed.
This change might potentially have negative impact on diff rendering
latency, because we are introducing awaitPluginsLoaded() to the
critical rendering path. Allowing diff content to be fetched
in parallel may not help, because diff content is prefetched. So we
hope for the best and have to monitor the metrics.
Change-Id: Ic54fcffef620aec874c49189dbab307e683d74d8
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 7610e5e..9fb1476 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -74,6 +74,7 @@
fireServerError,
fireEvent,
} from '../../../utils/event-util';
+import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader';
const MSG_EMPTY_BLAME = 'No blame information for this diff.';
@@ -316,6 +317,21 @@
this.clear();
}
+ initLayers() {
+ return getPluginLoader()
+ .awaitPluginsLoaded()
+ .then(() => {
+ if (!this.path) throw new Error('Missing required "path" property.');
+ if (!this.changeNum) throw new Error('Missing required "changeNum".');
+ this._layers = this._getLayers(this.path, this.changeNum);
+ this._coverageRanges = [];
+ // We kick off fetching the data here, but we don't return the promise,
+ // so awaiting initLayers() will not wait for coverage data to be
+ // completely loaded.
+ this._getCoverageData();
+ });
+ }
+
/**
* @param shouldReportMetric indicate a new Diff Page. This is a
* signal to report metrics event that started on location change.
@@ -328,22 +344,26 @@
this._errorMessage = null;
const whitespaceLevel = this._getIgnoreWhitespace();
- this._layers = this._getLayers(this.path, this.changeNum);
-
if (shouldReportMetric) {
// We listen on render viewport only on DiffPage (on paramsChanged)
this._listenToViewportRender();
}
- this._coverageRanges = [];
- this._getCoverageData();
-
try {
+ // We are carefully orchestrating operations that have to wait for another
+ // and operations that can be run in parallel. Plugins may provide layers,
+ // so we have to wait on plugins being loaded before we can initialize
+ // layers and proceed to rendering. OTOH we want to fetch diffs and diff
+ // assets in parallel.
+ const layerPromise = this.initLayers();
const diff = await this._getDiff();
this._loadedWhitespaceLevel = whitespaceLevel;
this._reportDiff(diff);
await this._loadDiffAssets(diff);
+ // Only now we are awaiting layers (and plugin loading), which was kicked
+ // off above.
+ await layerPromise;
// Not waiting for coverage ranges intentionally as
// plugin loading should not block the content rendering
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index ed9a8df..c28ee80 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -54,10 +54,11 @@
element.changeNum = 123;
element.path = 'some/path';
});
- test('plugin layers requested', () => {
+ test('plugin layers requested', async () => {
element.patchRange = {};
element.change = createChange();
- element.reload();
+ stubRestApi('getDiff').returns(Promise.resolve({content: []}));
+ await element.reload();
assert(element.$.jsAPI.getDiffLayers.called);
});
});
@@ -1109,8 +1110,9 @@
element.path = 'some/path';
});
- test('gr-diff-host provides syntax highlighting layer to gr-diff', () => {
- element.reload();
+ test('gr-diff-host provides syntax highlighting layer', async () => {
+ stubRestApi('getDiff').returns(Promise.resolve({content: []}));
+ await element.reload();
assert.equal(element.$.diff.layers[0], element.$.syntaxLayer);
});
@@ -1136,10 +1138,8 @@
test('starts syntax layer processing on render event', async () => {
sinon.stub(element.$.syntaxLayer, 'process')
.returns(Promise.resolve());
- stubRestApi('getDiff').returns(
- Promise.resolve({content: []}));
- element.reload();
- await flush();
+ stubRestApi('getDiff').returns(Promise.resolve({content: []}));
+ await element.reload();
element.dispatchEvent(
new CustomEvent('render', {bubbles: true, composed: true}));
assert.isTrue(element.$.syntaxLayer.process.called);
@@ -1164,8 +1164,9 @@
element.prefs = prefs;
});
- test('gr-diff-host provides syntax highlighting layer', () => {
- element.reload();
+ test('gr-diff-host provides syntax highlighting layer', async () => {
+ stubRestApi('getDiff').returns(Promise.resolve({content: []}));
+ await element.reload();
assert.equal(element.$.diff.layers[0], element.$.syntaxLayer);
});
@@ -1238,51 +1239,40 @@
};
element.patchRange = {};
element.prefs = prefs;
+ stubRestApi('getDiff').returns(Promise.resolve(element.diff));
});
- test('getCoverageAnnotationApis should be called', done => {
- element.reload();
- flush(() => {
- assert.isTrue(element.$.jsAPI.getCoverageAnnotationApis.calledOnce);
- done();
- });
+ test('getCoverageAnnotationApis should be called', async () => {
+ await element.reload();
+ assert.isTrue(element.$.jsAPI.getCoverageAnnotationApis.calledOnce);
});
- test('coverageRangeChanged should be called', done => {
- element.reload();
- flush(() => {
- assert.equal(notifyStub.callCount, 2);
- assert.isTrue(notifyStub.calledWithExactly(
- 'some/path', 1, 2, Side.RIGHT));
- assert.isTrue(notifyStub.calledWithExactly(
- 'some/path', 3, 4, Side.RIGHT));
- done();
- });
+ test('coverageRangeChanged should be called', async () => {
+ await element.reload();
+ assert.equal(notifyStub.callCount, 2);
+ assert.isTrue(notifyStub.calledWithExactly(
+ 'some/path', 1, 2, Side.RIGHT));
+ assert.isTrue(notifyStub.calledWithExactly(
+ 'some/path', 3, 4, Side.RIGHT));
});
- test('provider is called with appropriate params', done => {
+ test('provider is called with appropriate params', async () => {
element.patchRange.basePatchNum = 1;
element.patchRange.patchNum = 3;
- element.reload();
- flush(() => {
- assert.isTrue(coverageProviderStub.calledWithExactly(
- 123, 'some/path', 1, 3, element.change));
- done();
- });
+ await element.reload();
+ assert.isTrue(coverageProviderStub.calledWithExactly(
+ 123, 'some/path', 1, 3, element.change));
});
test('provider is called with appropriate params - special patchset values',
- done => {
+ async () => {
element.patchRange.basePatchNum = 'PARENT';
element.patchRange.patchNum = 'invalid';
- element.reload();
- flush(() => {
- assert.isTrue(coverageProviderStub.calledWithExactly(
- 123, 'some/path', undefined, undefined, element.change));
- done();
- });
+ await element.reload();
+ assert.isTrue(coverageProviderStub.calledWithExactly(
+ 123, 'some/path', undefined, undefined, element.change));
});
});
