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(fr): convert trace-elements gatherer #12442

Merged
merged 9 commits into from
May 6, 2021
Merged
Show file tree
Hide file tree
Changes from 7 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
3 changes: 3 additions & 0 deletions lighthouse-core/fraggle-rock/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const artifacts = {
RobotsTxt: '',
Stacks: '',
TapTargets: '',
TraceElements: '',
ViewportDimensions: '',
WebAppManifest: '',
devtoolsLogs: '',
Expand Down Expand Up @@ -68,6 +69,7 @@ const defaultConfig = {
{id: artifacts.RobotsTxt, gatherer: 'seo/robots-txt'},
{id: artifacts.Stacks, gatherer: 'stacks'},
{id: artifacts.TapTargets, gatherer: 'seo/tap-targets'},
{id: artifacts.TraceElements, gatherer: 'trace-elements'},
{id: artifacts.ViewportDimensions, gatherer: 'viewport-dimensions'},
{id: artifacts.WebAppManifest, gatherer: 'web-app-manifest'},
/* eslint-enable max-len */
Expand Down Expand Up @@ -103,6 +105,7 @@ const defaultConfig = {
artifacts.RobotsTxt,
artifacts.Stacks,
artifacts.TapTargets,
artifacts.TraceElements,
artifacts.ViewportDimensions,
artifacts.WebAppManifest,

Expand Down
119 changes: 67 additions & 52 deletions lighthouse-core/gather/gatherers/trace-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@
* We take the backend nodeId from the trace and use it to find the corresponding element in the DOM.
*/

const Gatherer = require('./gatherer.js');
const dom = require('../driver/dom.js');
const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const {resolveNodeIdToObjectId} = require('../driver/dom.js');
const pageFunctions = require('../../lib/page-functions.js');
const TraceProcessor = require('../../lib/tracehouse/trace-processor.js');
const RectHelpers = require('../../lib/rect-helpers.js');
const Sentry = require('../../lib/sentry.js');
const Trace = require('./trace.js');

/** @typedef {{nodeId: number, score?: number, animations?: {name?: string, failureReasonsMask?: number, unsupportedProperties?: string[]}[]}} TraceElementData */

Expand All @@ -37,7 +38,16 @@ function getNodeDetailsData() {
}
/* c8 ignore stop */

class TraceElements extends Gatherer {
class TraceElements extends FRGatherer {
/** @type {LH.Gatherer.GathererMeta<'Trace'>} */
meta = {
supportedModes: ['timespan', 'navigation'],
dependencies: {Trace: Trace.symbol},
}

/** @type {Map<string, string>} */
animationIdToName = new Map();

/**
* @param {LH.TraceEvent | undefined} event
* @return {number | undefined}
Expand Down Expand Up @@ -88,34 +98,6 @@ class TraceElements extends Gatherer {
return RectHelpers.addRectTopAndBottom(rectArgs);
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {string} animationId
* @return {Promise<string | undefined>}
*/
static async resolveAnimationName(passContext, animationId) {
const driver = passContext.driver;
try {
const result = await driver.sendCommand('Animation.resolveAnimation', {animationId});
const objectId = result.remoteObject.objectId;
if (!objectId) return undefined;
const response = await driver.sendCommand('Runtime.getProperties', {
objectId,
});
const nameProperty = response.result.find((property) => property.name === 'animationName');
const animationName = nameProperty && nameProperty.value && nameProperty.value.value;
if (animationName === '') return undefined;
return animationName;
} catch (err) {
// Animation name is not mission critical information and can be evicted, so don't throw fatally if we can't find it.
Sentry.captureException(err, {
tags: {gatherer: TraceElements.name},
level: 'error',
});
return undefined;
}
}

/**
* This function finds the top (up to 5) elements that contribute to the CLS score of the page.
* Each layout shift event has a 'score' which is the amount added to the CLS as a result of the given shift(s).
Expand Down Expand Up @@ -186,11 +168,10 @@ class TraceElements extends Gatherer {

/**
* Find the node ids of elements which are animated using the Animation trace events.
* @param {LH.Gatherer.PassContext} passContext
* @param {Array<LH.TraceEvent>} mainThreadEvents
* @return {Promise<Array<TraceElementData>>}
*/
static async getAnimatedElements(passContext, mainThreadEvents) {
async getAnimatedElements(mainThreadEvents) {
/** @type {Map<string, {begin: LH.TraceEvent | undefined, status: LH.TraceEvent | undefined}>} */
const animationPairs = new Map();
for (const event of mainThreadEvents) {
Expand All @@ -214,10 +195,10 @@ class TraceElements extends Gatherer {
/** @type {Map<number, Set<{animationId: string, failureReasonsMask?: number, unsupportedProperties?: string[]}>>} */
const elementAnimations = new Map();
for (const {begin, status} of animationPairs.values()) {
const nodeId = this.getNodeIDFromTraceEvent(begin);
const animationId = this.getAnimationIDFromTraceEvent(begin);
const failureReasonsMask = this.getFailureReasonsFromTraceEvent(status);
const unsupportedProperties = this.getUnsupportedPropertiesFromTraceEvent(status);
const nodeId = TraceElements.getNodeIDFromTraceEvent(begin);
const animationId = TraceElements.getAnimationIDFromTraceEvent(begin);
const failureReasonsMask = TraceElements.getFailureReasonsFromTraceEvent(status);
const unsupportedProperties = TraceElements.getUnsupportedPropertiesFromTraceEvent(status);
if (!nodeId || !animationId) continue;
const animationIds = elementAnimations.get(nodeId) || new Set();
animationIds.add({animationId, failureReasonsMask, unsupportedProperties});
Expand All @@ -229,7 +210,7 @@ class TraceElements extends Gatherer {
for (const [nodeId, animationIds] of elementAnimations) {
const animations = [];
for (const {animationId, failureReasonsMask, unsupportedProperties} of animationIds) {
const animationName = await this.resolveAnimationName(passContext, animationId);
const animationName = await this.animationIdToName.get(animationId);
adamraine marked this conversation as resolved.
Show resolved Hide resolved
animations.push({name: animationName, failureReasonsMask, unsupportedProperties});
}
animatedElementData.push({nodeId, animations});
Expand All @@ -238,30 +219,47 @@ class TraceElements extends Gatherer {
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.FRTransitionalContext} context
*/
async beforePass(passContext) {
await passContext.driver.sendCommand('Animation.enable');
async startInstrumentation(context) {
await context.driver.defaultSession.sendCommand('Animation.enable');

/** @param {LH.Crdp.Animation.AnimationStartedEvent} args */
this.onAnimationCreated = ({animation: {id, name}}) => {
name && this.animationIdToName.set(id, name);
adamraine marked this conversation as resolved.
Show resolved Hide resolved
};

context.driver.defaultSession.on('Animation.animationStarted', this.onAnimationCreated);
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @param {LH.Gatherer.FRTransitionalContext} context
*/
async stopInstrumentation(context) {
if (this.onAnimationCreated) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could avoid this if by moving onAnimationCreated to class property next to the map, but understand if you'd rather keep the impl inside startInstrumentation :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not a private function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Needed access to this, I wound up defining it in a new constructor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See JSUsage gatherer constructor for how to use this in class callback functions.

context.driver.defaultSession.off('Animation.animationStarted', this.onAnimationCreated);
}
await context.driver.defaultSession.sendCommand('Animation.disable');
}

/**
* @param {LH.Gatherer.FRTransitionalContext} context
* @param {LH.Trace|undefined} trace
* @return {Promise<LH.Artifacts['TraceElements']>}
*/
async afterPass(passContext, loadData) {
const driver = passContext.driver;
if (!loadData.trace) {
async _getArtifact(context, trace) {
const session = context.driver.defaultSession;
if (!trace) {
throw new Error('Trace is missing!');
}

const {largestContentfulPaintEvt, mainThreadEvents} =
TraceProcessor.computeTraceOfTab(loadData.trace);
TraceProcessor.computeTraceOfTab(trace);

const lcpNodeId = TraceElements.getNodeIDFromTraceEvent(largestContentfulPaintEvt);
const clsNodeData = TraceElements.getTopLayoutShiftElements(mainThreadEvents);
const animatedElementData =
await TraceElements.getAnimatedElements(passContext, mainThreadEvents);
await this.getAnimatedElements(mainThreadEvents);

/** @type {Map<string, TraceElementData[]>} */
const backendNodeDataMap = new Map([
Expand All @@ -276,9 +274,9 @@ class TraceElements extends Gatherer {
const backendNodeId = backendNodeData[i].nodeId;
let response;
try {
const objectId = await dom.resolveNodeIdToObjectId(driver.defaultSession, backendNodeId);
const objectId = await resolveNodeIdToObjectId(session, backendNodeId);
if (!objectId) continue;
response = await driver.sendCommand('Runtime.callFunctionOn', {
response = await session.sendCommand('Runtime.callFunctionOn', {
objectId,
functionDeclaration: `function () {
${getNodeDetailsData.toString()};
Expand Down Expand Up @@ -308,10 +306,27 @@ class TraceElements extends Gatherer {
}
}

await driver.sendCommand('Animation.disable');

return traceElements;
}

/**
* @param {LH.Gatherer.FRTransitionalContext<'Trace'>} context
* @return {Promise<LH.Artifacts.TraceElement[]>}
*/
async getArtifact(context) {
return this._getArtifact(context, context.dependencies.Trace);
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @return {Promise<LH.Artifacts.TraceElement[]>}
*/
async afterPass(passContext, loadData) {
const context = {...passContext, dependencies: {}};
await this.stopInstrumentation(context);
return this._getArtifact(context, loadData.trace);
}
}

module.exports = TraceElements;
2 changes: 1 addition & 1 deletion lighthouse-core/test/fraggle-rock/api-test-pptr.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ describe('Fraggle Rock API', () => {
const {lhr} = result;
const {auditResults, failedAudits, erroredAudits} = getAuditsBreakdown(lhr);
// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
expect(auditResults.length).toMatchInlineSnapshot(`103`);
expect(auditResults.length).toMatchInlineSnapshot(`105`);
Copy link
Collaborator

@patrickhulce patrickhulce May 5, 2021

Choose a reason for hiding this comment

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

hm, didn't bump the timespan one? oh right trace isn't supported in timespan yet 👍

expect(erroredAudits).toHaveLength(0);

const failedAuditIds = failedAudits.map(audit => audit.id);
Expand Down
Loading