Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(responsiveness): remove fallback trace event pre m103 #15866

Merged
merged 1 commit into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
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;

Check warning on line 73 in core/audits/metrics/interaction-to-next-paint.js

View check run for this annotation

Codecov / codecov/patch

core/audits/metrics/interaction-to-next-paint.js#L73

Added line #L73 was not covered by tests

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 @@
* 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)

Check warning on line 88 in core/computed/metrics/responsiveness.js

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/responsiveness.js#L86-L88

Added lines #L86 - L88 were not covered by tests
// 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'}
);

Check warning on line 94 in core/computed/metrics/responsiveness.js

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/responsiveness.js#L91-L94

Added lines #L91 - L94 were not covered by tests
}

const {maxDuration, interactionType} = responsivenessEvent.args.data;
Expand Down Expand Up @@ -136,7 +132,7 @@
/**
* @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 @@
const {settings} = context;
try {
const responsivenessEvent = await Responsiveness.request({trace, settings}, context);
if (!responsivenessEvent || responsivenessEvent.name === 'FallbackTiming') return;
if (!responsivenessEvent) return;

Check warning on line 158 in core/gather/gatherers/trace-elements.js

View check run for this annotation

Codecov / codecov/patch

core/gather/gatherers/trace-elements.js#L158

Added line #L158 was not covered by tests
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 () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we verify the error is thrown here in addition to/instead of in interaction-to-next-paint-test.js? Just cuz the logic to throw the error lives in responsiveness.js.

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
Loading