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.

Sign up for GitHub

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

Jump to bottom

core(fr): convert trace-elements gatherer #12442

Merged
merged 9 commits into from May 6, 2021
Merged

core(fr): convert trace-elements gatherer #12442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
abbff10
core(fr): convert trace-elements gatherer
adamraine May 4, 2021
9070191
rn context
adamraine May 4, 2021
ffe39dc
fix tests
adamraine May 4, 2021
10cf229
convert old tests
adamraine May 4, 2021
e30f536
i cant decide
adamraine May 4, 2021
f73fc4b
add to config
adamraine May 4, 2021
8cd5172
use instrumentation
adamraine May 5, 2021
4db9ee9
comments
adamraine May 5, 2021
4935e46
bind
adamraine May 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions lighthouse-core/fraggle-rock/config/default-config.js
View file
Open in desktop
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
121 changes: 69 additions & 52 deletions lighthouse-core/gather/gatherers/trace-elements.js
View file
Open in desktop
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,26 @@ 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();

constructor() {
super();
this._onAnimationStarted = this._onAnimationStarted.bind(this);
}

/** @param {LH.Crdp.Animation.AnimationStartedEvent} args */
_onAnimationStarted({animation: {id, name}}) {
if (name) this.animationIdToName.set(id, name);
}
Comment on lines +51 to +59
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
constructor() {
super();
this._onAnimationStarted = this._onAnimationStarted.bind(this);
}
/** @param {LH.Crdp.Animation.AnimationStartedEvent} args */
_onAnimationStarted({animation: {id, name}}) {
if (name) this.animationIdToName.set(id, name);
}
/** @param {LH.Crdp.Animation.AnimationStartedEvent} args */
_onAnimationStarted = ({animation: {id, name}}) => {
if (name) this.animationIdToName.set(id, name);
}

FWIW, this was my suggestion if you're going to separate already :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't do it, need access to this

Copy link
Collaborator

Choose a reason for hiding this comment

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

that shouldn't be a problem

$ cat > foo.js <<EOF
class Foo {
  x = 1
  foo = () => this.x
}

console.log(new Foo().foo())
const detached = new Foo().foo
console.log(detached()) // this works too :)
EOF
$ node foo.js
1
1

Copy link
Member Author

@adamraine adamraine May 5, 2021 •
edited

Choose a reason for hiding this comment

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

Unfortunately eslint disagrees. I guess it technically works though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's because they don't support class fields eslint/eslint#14343


/**
* @param {LH.TraceEvent | undefined} event
* @return {number | undefined}
Expand Down Expand Up @@ -88,34 +108,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 +178,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 +205,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 +220,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 = this.animationIdToName.get(animationId);
animations.push({name: animationName, failureReasonsMask, unsupportedProperties});
}
animatedElementData.push({nodeId, animations});
Expand All @@ -238,30 +229,39 @@ 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');
context.driver.defaultSession.on('Animation.animationStarted', this._onAnimationStarted);
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @param {LH.Gatherer.FRTransitionalContext} context
*/
async stopInstrumentation(context) {
context.driver.defaultSession.off('Animation.animationStarted', this._onAnimationStarted);
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 +276,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 +308,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
View file
Open in desktop
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

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