Fix events emitted by gr-diff-builder-element
We had multiple issues with render-* events.
Change 334544 has added a 'render-progress' event for updating cursor
stops while the rendering is still in progress. This has now been
identified as substantially impacting rendering performance. Such events
were fired hundreds of times per second (one for each diff group).
Updating cursor stops involves querying the DOM, which forces the
browser to reflow. This should be avoided, so we are removing
'render-progress' events altogether in this change. Maybe a solution for
updating cursor stops is using a timer that fires every 500 ms or so.
But that needs some careful thought and has to be done in a separate
change.
Change 336092 tried to fire 'render-*' events only after DOM rendering
has completed, but it did not work as intended, and change 336494 tried
to fix that. But it turns out that `afterNextRender` uses
`requestAnimationFrame`, which is not invoking callbacks while tabs are
in the background. This broke Gerrit's latency metrics and triggered
SLO alerts, see b/232095724. We are replacing usages of
`afterNextRender` by `whenRendered`, which is using a `ResizeObserver`
and waits for the DOM element to get a clientHeight > 0.
Google-Bug-Id: b/232095724
Release-Notes: skip
Change-Id: I98d99ca43bf12ba901bb2defe8a4ded3bf0f587c
diff --git a/polygerrit-ui/app/api/diff.ts b/polygerrit-ui/app/api/diff.ts
index 4322c64..430e770 100644
--- a/polygerrit-ui/app/api/diff.ts
+++ b/polygerrit-ui/app/api/diff.ts
@@ -340,6 +340,7 @@
lineNum: LineNumber;
}
+// TODO: Currently unused and not fired.
export declare interface RenderProgressEventDetail {
linesRendered: number;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index d200c75..d2704ac 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -26,11 +26,7 @@
GrRangedCommentLayer,
} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
import {GrCoverageLayer} from '../gr-coverage-layer/gr-coverage-layer';
-import {
- DiffViewMode,
- RenderPreferences,
- RenderProgressEventDetail,
-} from '../../../api/diff';
+import {DiffViewMode, RenderPreferences} from '../../../api/diff';
import {createDefaultDiffPrefs, Side} from '../../../constants/constants';
import {GrDiffLine, LineNumber} from '../gr-diff/gr-diff-line';
import {
@@ -39,13 +35,9 @@
hideInContextControl,
} from '../gr-diff/gr-diff-group';
import {getLineNumber, getSideByLineEl} from '../gr-diff/gr-diff-utils';
-import {
- fireAlert,
- fire,
- HTMLElementEventDetailType,
-} from '../../../utils/event-util';
+import {fireAlert, fireEvent} from '../../../utils/event-util';
import {assertIsDefined} from '../../../utils/common-util';
-import {afterNextRender} from '../../../utils/dom-util';
+import {untilRendered} from '../../../utils/dom-util';
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
const COMMIT_MSG_PATH = '/COMMIT_MSG';
@@ -59,11 +51,6 @@
*/
'render-start': CustomEvent<{}>;
/**
- * Fired whenever a new chunk of lines has been rendered synchronously - this
- * only happens for full renders.
- */
- 'render-progress': CustomEvent<RenderProgressEventDetail>;
- /**
* Fired when the diff finishes rendering text content - both for full
* renders and for partial rerenders.
*/
@@ -163,6 +150,16 @@
// visible for testing
processor = new GrDiffProcessor();
+ /**
+ * Groups are mostly just passed on to the diff builder (this.builder). But
+ * we also keep track of them here for being able to fire a `render-content`
+ * event when .element of each group has rendered.
+ *
+ * TODO: Refactor DiffBuilderElement and DiffBuilders with a cleaner
+ * separation of responsibilities.
+ */
+ private groups: GrDiffGroup[] = [];
+
constructor() {
this.processor.consumer = this;
}
@@ -210,7 +207,7 @@
const isBinary = !!(this.isImageDiff || this.diff.binary);
- this.fireDiffEvent('render-start', {});
+ this.fireDiffEvent('render-start');
this.cancelableRenderPromise = makeCancelable(
this.processor
.process(this.diff.content, isBinary)
@@ -218,7 +215,10 @@
if (this.isImageDiff) {
(this.builder as GrDiffBuilderImage).renderDiff();
}
- afterNextRender(() => this.fireDiffEvent('render-content', {}));
+ return this.untilGroupsRendered();
+ })
+ .then(() => {
+ this.fireDiffEvent('render-content');
})
// Mocha testing does not like uncaught rejections, so we catch
// the cancels which are expected and should not throw errors in
@@ -233,6 +233,18 @@
);
}
+ // visible for testing
+ async untilGroupsRendered(groups: readonly GrDiffGroup[] = this.groups) {
+ for (const g of groups) {
+ // The LOST or FILE lines may be hidden and thus never resolve an
+ // untilRendered() promise.
+ const lineNumber = g.lines?.[0]?.beforeNumber;
+ if (g.skip || lineNumber === 'LOST' || lineNumber === 'FILE') continue;
+ assertIsDefined(g.element);
+ await untilRendered(g.element);
+ }
+ }
+
private onDiffContextExpanded = (
e: CustomEvent<DiffContextExpandedEventDetail>
) => {
@@ -241,17 +253,9 @@
this.replaceGroup(e.detail.contextGroup, e.detail.groups);
};
- private fireDiffEvent<K extends keyof HTMLElementEventMap>(
- type: K,
- detail: HTMLElementEventDetailType<K>
- ) {
+ private fireDiffEvent<K extends keyof HTMLElementEventMap>(type: K) {
assertIsDefined(this.diffElement, 'diff table');
- fire(this.diffElement, type, detail);
- }
-
- private fireDiffEventRenderProgress(detail: RenderProgressEventDetail) {
- assertIsDefined(this.diffElement, 'diff table');
- fire(this.diffElement, 'render-progress', detail);
+ fireEvent(this.diffElement, type);
}
// visible for testing
@@ -364,15 +368,12 @@
newGroups: readonly GrDiffGroup[]
) {
if (!this.builder) return;
- this.fireDiffEvent('render-start', {});
- const linesRendered = newGroups.reduce(
- (sum, group) => sum + group.lines.length,
- 0
- );
+ this.fireDiffEvent('render-start');
this.builder.replaceGroup(contextGroup, newGroups);
- afterNextRender(() => {
- this.fireDiffEvent('render-progress', {linesRendered});
- this.fireDiffEvent('render-content', {});
+ this.groups = this.groups.filter(g => g !== contextGroup);
+ this.groups.push(...newGroups);
+ this.untilGroupsRendered(newGroups).then(() => {
+ this.fireDiffEvent('render-content');
});
}
@@ -464,6 +465,7 @@
*/
clearGroups() {
if (!this.builder) return;
+ this.groups = [];
this.builder.clearGroups();
}
@@ -473,9 +475,7 @@
addGroup(group: GrDiffGroup) {
if (!this.builder) return;
this.builder.addGroups([group]);
- afterNextRender(() =>
- this.fireDiffEventRenderProgress({linesRendered: group.lines.length})
- );
+ this.groups.push(group);
}
// visible for testing
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
index 8ae08f4..d37afc0 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
@@ -10,11 +10,7 @@
createEmptyDiff,
} from '../../../test/test-data-generators';
import './gr-diff-builder-element';
-import {
- nextRender,
- queryAndAssert,
- stubBaseUrl,
-} from '../../../test/test-utils';
+import {queryAndAssert, stubBaseUrl, waitUntil} from '../../../test/test-utils';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
import {GrDiffLine, GrDiffLineType} from '../gr-diff/gr-diff-line';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
@@ -601,6 +597,7 @@
addColumnsStub = sinon.stub(builder, 'addColumns');
builder.buildSectionElement = function (group) {
const section = document.createElement('stub');
+ section.style.display = 'block';
section.textContent = group.lines.reduce(
(acc, line) => acc + line.text,
''
@@ -639,12 +636,11 @@
});
test('render-start and render-content are fired', async () => {
- await nextRender();
+ await waitUntil(() => dispatchStub.callCount >= 1);
let firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-start');
- assert.include(firedEventTypes, 'render-progress');
- await nextRender();
+ await waitUntil(() => dispatchStub.callCount >= 2);
firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-content');
});
@@ -678,7 +674,7 @@
};
element.render(keyLocations);
// Make sure all listeners are installed.
- await nextRender();
+ await element.untilGroupsRendered();
});
test('hides lines behind two context controls', () => {
@@ -742,7 +738,7 @@
assert.include(diffRows[8].textContent, 'after');
assert.include(diffRows[9].textContent, 'unchanged 11');
- await nextRender();
+ await element.untilGroupsRendered();
const firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-content');
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
index e80d86b..e00353a 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -336,10 +336,6 @@
this.preventAutoScrollOnManualScroll = true;
};
- private _boundHandleDiffRenderProgress = () => {
- this._updateStops();
- };
-
private _boundHandleDiffRenderContent = () => {
this._updateStops();
// When done rendering, turn focus on move and automatic scrolling back on
@@ -551,10 +547,6 @@
);
diff.removeEventListener('render-start', this._boundHandleDiffRenderStart);
diff.removeEventListener(
- 'render-progress',
- this._boundHandleDiffRenderProgress
- );
- diff.removeEventListener(
'render-content',
this._boundHandleDiffRenderContent
);
@@ -570,10 +562,6 @@
this.boundHandleDiffLoadingChanged
);
diff.addEventListener('render-start', this._boundHandleDiffRenderStart);
- diff.addEventListener(
- 'render-progress',
- this._boundHandleDiffRenderProgress
- );
diff.addEventListener('render-content', this._boundHandleDiffRenderContent);
diff.addEventListener('line-selected', this._boundHandleDiffLineSelected);
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index 6f71c65..616a36f 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -614,6 +614,7 @@
const result = [];
let lastChunkEndOffset = 0;
for (const {offset, keyLocation} of chunkEnds) {
+ if (lastChunkEndOffset === offset) continue;
result.push({
lines: lines.slice(lastChunkEndOffset, offset),
keyLocation,
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
index b8262e0..e015d49 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
@@ -17,6 +17,7 @@
import {BLANK_LINE, GrDiffLine, GrDiffLineType} from './gr-diff-line';
import {LineRange, Side} from '../../../api/diff';
import {LineNumber} from './gr-diff-line';
+import {assertIsDefined, check} from '../../../utils/common-util';
export enum GrDiffGroupType {
/** Unchanged context. */
@@ -275,7 +276,9 @@
},
};
} else {
- for (const line of options.lines ?? []) {
+ assertIsDefined(options.lines);
+ check(options.lines.length > 0, 'diff group must have lines');
+ for (const line of options.lines) {
this.addLine(line);
}
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts
index 43a56d1..bbf03ae 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts
@@ -46,6 +46,16 @@
]);
});
+ test('group must have lines', () => {
+ try {
+ new GrDiffGroup({type: GrDiffGroupType.BOTH});
+ } catch (e) {
+ // expected
+ return;
+ }
+ assert.fail('a standard diff group cannot be empty');
+ });
+
test('group/header line pairs', () => {
const l1 = new GrDiffLine(GrDiffLineType.BOTH, 64, 128);
const l2 = new GrDiffLine(GrDiffLineType.BOTH, 65, 129);
@@ -232,11 +242,6 @@
assert.isTrue(group.isTotal());
});
- test('not total for empty', () => {
- const group = new GrDiffGroup({type: GrDiffGroupType.BOTH});
- assert.isFalse(group.isTotal());
- });
-
test('not total for non-delta', () => {
const lines = [];
for (let idx = 0; idx < 10; idx++) {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index 0eed83d..1422223 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -613,8 +613,7 @@
threadEl.setAttribute('slot', 'right-1');
const content = [
{
- a: [],
- b: [],
+ a: ['asdf'],
},
{
ab: Array(13).fill('text'),
@@ -624,7 +623,7 @@
await waitForEventOnce(element, 'render');
element.appendChild(threadEl);
- await flush();
+ await waitForEventOnce(element, 'render');
const hint = queryAndAssert<GrRangedCommentHint>(
element,
@@ -651,21 +650,18 @@
firstHint.setAttribute('slot', 'right-1');
const content = [
{
- a: [],
- b: [],
+ a: ['asdf'],
},
{
ab: Array(13).fill('text'),
},
];
setupSampleDiff({content});
+ await waitForEventOnce(element, 'render');
element.appendChild(firstHint);
- await flush();
- element._handleRenderContent();
- await flush();
element.appendChild(threadEl);
- await flush();
+ await waitForEventOnce(element, 'render');
assert.equal(
element.querySelectorAll('gr-ranged-comment-hint').length,
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index ea7865e..985bec1 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -27,7 +27,7 @@
import {ShortcutsService} from '../services/shortcuts/shortcuts-service';
import {queryAndAssert, query} from '../utils/common-util';
import {FlagsService} from '../services/flags/flags';
-import {afterNextRender, Key, Modifier} from '../utils/dom-util';
+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';
@@ -224,10 +224,6 @@
return waitUntil(() => stub.called, `${name} was not called`);
}
-export async function nextRender() {
- return new Promise(resolve => afterNextRender(resolve));
-}
-
/**
* Subscribes to the observable and resolves once it emits a matching value.
* Usage:
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts
index f2e0994..be6aa06 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -491,8 +491,18 @@
return false;
}
+/** Returns a promise that waits for the element's height to become > 0. */
+export function untilRendered(el: HTMLElement) {
+ return new Promise(resolve => {
+ whenRendered(el, resolve);
+ });
+}
+
/** Executes the given callback when the element's height is > 0. */
-export function whenRendered(el: HTMLElement, callback: () => void) {
+export function whenRendered(
+ el: HTMLElement,
+ callback: (value?: unknown) => void
+) {
if (el.clientHeight > 0) {
callback();
return;
@@ -505,14 +515,3 @@
});
obs.observe(el);
}
-
-/**
- * Mimics a Polymer utility. `requestAnimationFrame` is called before the next
- * browser paint. An additional `setTimeout` ensures that the paint has
- * actually happened.
- */
-export function afterNextRender(callback: (value?: unknown) => void) {
- requestAnimationFrame(() => {
- setTimeout(callback);
- });
-}
