Skip to content

Commit

Permalink
core(responsiveness): remove fallback trace event pre m103 (#15866)
Browse files Browse the repository at this point in the history
  • Loading branch information
connorjclark authored Mar 14, 2024
1 parent 2200be5 commit 827c87b
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 58 deletions.
4 changes: 1 addition & 3 deletions core/audits/metrics/interaction-to-next-paint.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,7 @@ class InteractionToNextPaint extends Audit {
return {score: null, notApplicable: true};
}

// TODO: remove workaround once 103.0.5052.0 is sufficiently released.
const timing = interactionEvent.name === 'FallbackTiming' ?
interactionEvent.duration : interactionEvent.args.data.duration;
const timing = interactionEvent.args.data.duration;

return {
score: Audit.computeLogNormalScore({p10: context.options.p10, median: context.options.median},
Expand Down
8 changes: 0 additions & 8 deletions core/audits/work-during-interaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {taskGroups} from '../lib/tracehouse/task-groups.js';
import {TraceProcessor} from '../lib/tracehouse/trace-processor.js';
import {getExecutionTimingsByURL} from '../lib/tracehouse/task-summary.js';
import InteractionToNextPaint from './metrics/interaction-to-next-paint.js';
import {LighthouseError} from '../lib/lh-error.js';

/** @typedef {import('../computed/metrics/responsiveness.js').EventTimingEvent} EventTimingEvent */
/** @typedef {import('../lib/tracehouse/main-thread-tasks.js').TaskNode} TaskNode */
Expand Down Expand Up @@ -243,13 +242,6 @@ class WorkDuringInteraction extends Audit {
metricSavings: {INP: 0},
};
}
// TODO: remove workaround once 103.0.5052.0 is sufficiently released.
if (interactionEvent.name === 'FallbackTiming') {
throw new LighthouseError(
LighthouseError.errors.UNSUPPORTED_OLD_CHROME,
{featureName: 'detailed EventTiming trace events'}
);
}

const auditDetailsItems = [];

Expand Down
24 changes: 10 additions & 14 deletions core/computed/metrics/responsiveness.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,10 @@
* @property {number} interactionId
*/
/** @typedef {LH.Trace.AsyncEvent & {name: 'EventTiming', args: {data: EventTimingData}}} EventTimingEvent */
/**
* A fallback EventTiming placeholder, used if updated EventTiming events are not available.
* TODO: Remove once 103.0.5052.0 is sufficiently released.
* @typedef {{name: 'FallbackTiming', duration: number}} FallbackTimingEvent
*/

import {ProcessedTrace} from '../processed-trace.js';
import {makeComputedArtifact} from '../computed-artifact.js';
import {LighthouseError} from '../../lib/lh-error.js';

const KEYBOARD_EVENTS = new Set(['keydown', 'keypress', 'keyup']);
const CLICK_TAP_DRAG_EVENTS = new Set([
Expand Down Expand Up @@ -79,23 +75,23 @@ class Responsiveness {
* one interaction had this duration by returning the first found.
* @param {ResponsivenessEvent} responsivenessEvent
* @param {LH.Trace} trace
* @return {EventTimingEvent|FallbackTimingEvent}
* @return {EventTimingEvent}
*/
static findInteractionEvent(responsivenessEvent, {traceEvents}) {
const candidates = traceEvents.filter(/** @return {evt is EventTimingEvent} */ evt => {
// Examine only beginning/instant EventTiming events.
return evt.name === 'EventTiming' && evt.ph !== 'e';
});

// If trace is from < m103, the timestamps cannot be trusted, so we craft a fallback
// <m103 traces (bad) had a args.frame
// If trace is from < m103, the timestamps cannot be trusted
// <m103 traces (bad) had a args.frame (we used to provide a fallback trace event, but not
// any more)
// m103+ traces (good) have a args.data.frame (https://crrev.com/c/3632661)
// TODO(compat): remove FallbackTiming handling when we don't care about <m103
if (candidates.length && candidates.every(candidate => !candidate.args.data?.frame)) {
return {
name: 'FallbackTiming',
duration: responsivenessEvent.args.data.maxDuration,
};
throw new LighthouseError(
LighthouseError.errors.UNSUPPORTED_OLD_CHROME,
{featureName: 'detailed EventTiming trace events'}
);
}

const {maxDuration, interactionType} = responsivenessEvent.args.data;
Expand Down Expand Up @@ -136,7 +132,7 @@ class Responsiveness {
/**
* @param {{trace: LH.Trace, settings: LH.Audit.Context['settings']}} data
* @param {LH.Artifacts.ComputedContext} context
* @return {Promise<EventTimingEvent|FallbackTimingEvent|null>}
* @return {Promise<EventTimingEvent|null>}
*/
static async compute_(data, context) {
const {settings, trace} = data;
Expand Down
2 changes: 1 addition & 1 deletion core/gather/gatherers/trace-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class TraceElements extends BaseGatherer {
const {settings} = context;
try {
const responsivenessEvent = await Responsiveness.request({trace, settings}, context);
if (!responsivenessEvent || responsivenessEvent.name === 'FallbackTiming') return;
if (!responsivenessEvent) return;
return {nodeId: responsivenessEvent.args.data.nodeId};
} catch {
// Don't let responsiveness errors sink the rest of the gatherer.
Expand Down
13 changes: 3 additions & 10 deletions core/test/audits/metrics/interaction-to-next-paint-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('Interaction to Next Paint', () => {
});
});

it('falls back Responsiveness timing if no m103 EventTiming events', async () => {
it('throw error if no m103 EventTiming events', async () => {
const {artifacts, context} = getTestData();
const clonedTrace = JSON.parse(JSON.stringify(artifacts.traces.defaultPass));
for (let i = 0; i < clonedTrace.traceEvents.length; i++) {
Expand All @@ -47,15 +47,8 @@ describe('Interaction to Next Paint', () => {
}
artifacts.traces.defaultPass = clonedTrace;

const result = await InteractionToNextPaint.audit(artifacts, context);
// Conveniently, the matching responsiveness event has slightly different
// duration than the matching interaction event so can be tested against.
expect(result).toEqual({
score: 0.67,
numericValue: 364,
numericUnit: 'millisecond',
displayValue: expect.toBeDisplayString('360 ms'),
});
const promise = InteractionToNextPaint.audit(artifacts, context);
await expect(promise).rejects.toThrow('UNSUPPORTED_OLD_CHROME');
});

it('is not applicable if using simulated throttling', async () => {
Expand Down
22 changes: 0 additions & 22 deletions core/test/computed/metrics/responsiveness-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,28 +237,6 @@ describe('Metric: Responsiveness', () => {
.rejects.toThrow(`unexpected responsiveness interactionType 'brainWave'`);
});

it('returns a fallback timing event if provided with the old trace event format', async () => {
const interactionEvents = [{
ts: 500,
maxDuration: 200,
}];
const trace = makeTrace(interactionEvents);
for (const event of trace.traceEvents) {
if (event.name !== 'EventTiming') continue;
event.args.data = {};
}

const metricInputData = {
trace,
settings: {throttlingMethod: 'provided'},
};
const event = await Responsiveness.request(metricInputData, {computedCache: new Map()});
expect(event).toEqual({
name: 'FallbackTiming',
duration: 200,
});
});

it('only finds interaction events from the same frame as the responsiveness event', async () => {
const maxDuration = 200;
const interactionEvents = [{
Expand Down

0 comments on commit 827c87b

Please sign in to comment.