Let the new syntax highlighting layer properly deal with cancellation
Release-Notes: skip
Google-Bug-Id: b/219684547
Change-Id: I7230475686d6c24801d851f9733ffa53bf35e36d
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 9d2a819..85f8ce0 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
@@ -98,10 +98,8 @@
import {distinctUntilChanged, map} from 'rxjs/operators';
import {deepEqual} from '../../../utils/deep-util';
import {Category} from '../../../api/checks';
-import {
- GrSyntaxLayerWorker,
- CODE_MAX_LINES,
-} from '../../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker';
+import {GrSyntaxLayerWorker} from '../../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker';
+import {CODE_MAX_LINES} from '../../../services/highlight/highlight-service';
const EMPTY_BLAME = 'No blame information for this diff.';
diff --git a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts
index 59ce396..82dfb35 100644
--- a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts
+++ b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts
@@ -10,6 +10,7 @@
import {Side} from '../../../constants/constants';
import {getAppContext} from '../../../services/app-context';
import {SyntaxLayerLine} from '../../../types/syntax-worker-api';
+import {CancelablePromise, util} from '../../../scripts/util';
const LANGUAGE_MAP = new Map<string, string>([
['application/dart', 'dart'],
@@ -125,29 +126,35 @@
'variable',
]);
-/**
- * Safe guard for not killing the browser.
- */
-export const CODE_MAX_LINES = 20 * 1000;
-
-/**
- * Safe guard for not killing the browser. Maximum in number of chars.
- */
-const CODE_MAX_LENGTH = 25 * CODE_MAX_LINES;
-
export class GrSyntaxLayerWorker implements DiffLayer {
diff?: DiffInfo;
enabled = true;
- private leftRanges: SyntaxLayerLine[] = [];
+ // private, but visible for testing
+ leftRanges: SyntaxLayerLine[] = [];
- private rightRanges: SyntaxLayerLine[] = [];
+ // private, but visible for testing
+ rightRanges: SyntaxLayerLine[] = [];
+
+ /**
+ * We are keeping a reference around to the async computation, such that we
+ * can cancel it, if needed.
+ */
+ private leftPromise?: CancelablePromise<SyntaxLayerLine[]>;
+
+ /**
+ * We are keeping a reference around to the async computation, such that we
+ * can cancel it, if needed.
+ */
+ private rightPromise?: CancelablePromise<SyntaxLayerLine[]>;
private listeners: DiffLayerListener[] = [];
private readonly highlightService = getAppContext().highlightService;
+ private readonly reportingService = getAppContext().reportingService;
+
setEnabled(enabled: boolean) {
this.enabled = enabled;
}
@@ -220,6 +227,10 @@
this.diff = diff;
this.leftRanges = [];
this.rightRanges = [];
+ if (this.leftPromise) this.leftPromise.cancel();
+ if (this.rightPromise) this.rightPromise.cancel();
+ this.leftPromise = undefined;
+ this.rightPromise = undefined;
if (!this.enabled || !this.diff) return;
const leftLanguage = this._getLanguage(this.diff.meta_a);
@@ -245,17 +256,26 @@
leftContent = leftContent.trimEnd();
rightContent = rightContent.trimEnd();
- const leftPromise = this.highlight(leftLanguage, leftContent);
- const rightPromise = this.highlight(rightLanguage, rightContent);
- this.leftRanges = await leftPromise;
- this.rightRanges = await rightPromise;
- this.notify();
+ try {
+ this.leftPromise = this.highlight(leftLanguage, leftContent);
+ this.rightPromise = this.highlight(rightLanguage, rightContent);
+ this.leftRanges = await this.leftPromise;
+ this.rightRanges = await this.rightPromise;
+ this.notify();
+ } catch (err) {
+ if (!err.isCanceled) this.reportingService.error(err);
+ // One source of "error" can promise cancelation.
+ this.leftRanges = [];
+ this.rightRanges = [];
+ }
}
- async highlight(language?: string, code?: string) {
- if (!language || !code) return [];
- if (code.length > CODE_MAX_LENGTH) return [];
- return this.highlightService.highlight(language, code);
+ highlight(
+ language?: string,
+ code?: string
+ ): CancelablePromise<SyntaxLayerLine[]> {
+ const hlPromise = this.highlightService.highlight(language, code);
+ return util.makeCancelable(hlPromise);
}
notify() {
diff --git a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts
index c9e70ad..ee82d5d 100644
--- a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts
@@ -5,6 +5,7 @@
*/
import {DiffInfo, GrDiffLineType, Side} from '../../../api/diff';
import '../../../test/common-test-setup-karma';
+import {mockPromise, stubHighlightService} from '../../../test/test-utils';
import {SyntaxLayerLine} from '../../../types/syntax-worker-api';
import {GrDiffLine} from '../gr-diff/gr-diff-line';
import {GrSyntaxLayerWorker} from './gr-syntax-layer-worker';
@@ -75,40 +76,78 @@
setup(() => {
layer = new GrSyntaxLayerWorker();
- listener = sinon.stub();
- layer.addListener(listener);
- sinon.stub(layer, 'highlight').callsFake((lang?: string) => {
- if (lang === 'lang-left') return Promise.resolve(leftRanges);
- if (lang === 'lang-right') return Promise.resolve(rightRanges);
- return Promise.resolve([]);
+ });
+
+ test('cancel processing', async () => {
+ const mockPromise1 = mockPromise<SyntaxLayerLine[]>();
+ const mockPromise2 = mockPromise<SyntaxLayerLine[]>();
+ const mockPromise3 = mockPromise<SyntaxLayerLine[]>();
+ const mockPromise4 = mockPromise<SyntaxLayerLine[]>();
+ const stub = stubHighlightService('highlight');
+ stub.onCall(0).returns(mockPromise1);
+ stub.onCall(1).returns(mockPromise2);
+ stub.onCall(2).returns(mockPromise3);
+ stub.onCall(3).returns(mockPromise4);
+
+ const processPromise1 = layer.process(diff);
+ // Calling the process() a second time means that the promises created
+ // during the first call are cancelled.
+ const processPromise2 = layer.process(diff);
+ // We can await the outer promise even before the inner promises resolve,
+ // because cancelling rejects the inner promises.
+ await processPromise1;
+ // It does not matter actually, whether these two inner promises are
+ // resolved or not.
+ mockPromise1.resolve(leftRanges);
+ mockPromise2.resolve(rightRanges);
+ // Both ranges must still be empty, because the promise of the first call
+ // must have been cancelled and the returned ranges ignored.
+ assert.isEmpty(layer.leftRanges);
+ assert.isEmpty(layer.rightRanges);
+ // Lets' resolve and await the promises of the second as normal.
+ mockPromise3.resolve(leftRanges);
+ mockPromise4.resolve(rightRanges);
+ await processPromise2;
+ assert.equal(layer.leftRanges, leftRanges);
+ });
+
+ suite('annotate and listen', () => {
+ setup(() => {
+ listener = sinon.stub();
+ layer.addListener(listener);
+ stubHighlightService('highlight').callsFake((lang?: string) => {
+ if (lang === 'lang-left') return Promise.resolve(leftRanges);
+ if (lang === 'lang-right') return Promise.resolve(rightRanges);
+ return Promise.resolve([]);
+ });
});
- });
- test('process and annotate line 2 LEFT', async () => {
- await layer.process(diff);
- const el = annotate(Side.LEFT, 1, 'import it;');
- assert.equal(
- el.innerHTML,
- '<hl class="gr-diff gr-syntax gr-syntax-literal">import</hl> it;'
- );
- assert.equal(listener.callCount, 2);
- assert.equal(listener.getCall(0).args[0], 1);
- assert.equal(listener.getCall(0).args[1], 1);
- assert.equal(listener.getCall(0).args[2], Side.LEFT);
- assert.equal(listener.getCall(1).args[0], 3);
- assert.equal(listener.getCall(1).args[1], 3);
- assert.equal(listener.getCall(1).args[2], Side.RIGHT);
- });
+ test('process and annotate line 2 LEFT', async () => {
+ await layer.process(diff);
+ const el = annotate(Side.LEFT, 1, 'import it;');
+ assert.equal(
+ el.innerHTML,
+ '<hl class="gr-diff gr-syntax gr-syntax-literal">import</hl> it;'
+ );
+ assert.equal(listener.callCount, 2);
+ assert.equal(listener.getCall(0).args[0], 1);
+ assert.equal(listener.getCall(0).args[1], 1);
+ assert.equal(listener.getCall(0).args[2], Side.LEFT);
+ assert.equal(listener.getCall(1).args[0], 3);
+ assert.equal(listener.getCall(1).args[1], 3);
+ assert.equal(listener.getCall(1).args[2], Side.RIGHT);
+ });
- test('process and annotate line 3 RIGHT', async () => {
- await layer.process(diff);
- const el = annotate(Side.RIGHT, 3, ' public static final {');
- assert.equal(
- el.innerHTML,
- ' <hl class="gr-diff gr-syntax gr-syntax-literal">public</hl> ' +
- '<hl class="gr-diff gr-syntax gr-syntax-keyword">static</hl> ' +
- '<hl class="gr-diff gr-syntax gr-syntax-name">final</hl> {'
- );
- assert.equal(listener.callCount, 2);
+ test('process and annotate line 3 RIGHT', async () => {
+ await layer.process(diff);
+ const el = annotate(Side.RIGHT, 3, ' public static final {');
+ assert.equal(
+ el.innerHTML,
+ ' <hl class="gr-diff gr-syntax gr-syntax-literal">public</hl> ' +
+ '<hl class="gr-diff gr-syntax gr-syntax-keyword">static</hl> ' +
+ '<hl class="gr-diff gr-syntax gr-syntax-name">final</hl> {'
+ );
+ assert.equal(listener.callCount, 2);
+ });
});
});
diff --git a/polygerrit-ui/app/services/highlight/highlight-service.ts b/polygerrit-ui/app/services/highlight/highlight-service.ts
index 3dd3cf1..80da260 100644
--- a/polygerrit-ui/app/services/highlight/highlight-service.ts
+++ b/polygerrit-ui/app/services/highlight/highlight-service.ts
@@ -30,6 +30,16 @@
const WORKER_POOL_SIZE = 3;
/**
+ * Safe guard for not killing the browser.
+ */
+export const CODE_MAX_LINES = 20 * 1000;
+
+/**
+ * Safe guard for not killing the browser. Maximum in number of chars.
+ */
+const CODE_MAX_LENGTH = 25 * CODE_MAX_LINES;
+
+/**
* Service for syntax highlighting. Maintains some HighlightJS workers doing
* their job in the background.
*/
@@ -115,7 +125,12 @@
if (resolver) resolver(result.ranges ?? []);
}
- async highlight(language: string, code: string): Promise<SyntaxLayerLine[]> {
+ async highlight(
+ language?: string,
+ code?: string
+ ): Promise<SyntaxLayerLine[]> {
+ if (!language || !code) return [];
+ if (code.length > CODE_MAX_LENGTH) return [];
const worker = await this.requestWorker();
const message: SyntaxWorkerRequest = {
type: SyntaxWorkerMessageType.REQUEST,
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index f0c8a84..0d9cf3c 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -30,6 +30,7 @@
import {Key, Modifier} from '../utils/dom-util';
import {Observable} from 'rxjs';
import {filter, take, timeout} from 'rxjs/operators';
+import {HighlightService} from '../services/highlight/highlight-service';
export {query, queryAll, queryAndAssert} from '../utils/common-util';
export interface MockPromise<T> extends Promise<T> {
@@ -121,6 +122,12 @@
return sinon.stub(getAppContext().shortcutsService, method);
}
+export function stubHighlightService<K extends keyof HighlightService>(
+ method: K
+) {
+ return sinon.stub(getAppContext().highlightService, method);
+}
+
export function stubStorage<K extends keyof StorageService>(method: K) {
return sinon.stub(getAppContext().storageService, method);
}
