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 6 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
74 changes: 49 additions & 25 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,13 @@ 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},
}

/**
* @param {LH.TraceEvent | undefined} event
* @return {number | undefined}
Expand Down Expand Up @@ -89,17 +96,17 @@ class TraceElements extends Gatherer {
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.FRTransitionalContext} context
* @param {string} animationId
* @return {Promise<string | undefined>}
*/
static async resolveAnimationName(passContext, animationId) {
const driver = passContext.driver;
static async resolveAnimationName(context, animationId) {
const session = context.driver.defaultSession;
try {
const result = await driver.sendCommand('Animation.resolveAnimation', {animationId});
const result = await session.sendCommand('Animation.resolveAnimation', {animationId});
const objectId = result.remoteObject.objectId;
if (!objectId) return undefined;
const response = await driver.sendCommand('Runtime.getProperties', {
const response = await session.sendCommand('Runtime.getProperties', {
objectId,
});
const nameProperty = response.result.find((property) => property.name === 'animationName');
Expand All @@ -112,7 +119,6 @@ class TraceElements extends Gatherer {
tags: {gatherer: TraceElements.name},
level: 'error',
});
return undefined;
}
}

Expand Down Expand Up @@ -186,11 +192,11 @@ class TraceElements extends Gatherer {

/**
* Find the node ids of elements which are animated using the Animation trace events.
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.FRTransitionalContext} context
* @param {Array<LH.TraceEvent>} mainThreadEvents
* @return {Promise<Array<TraceElementData>>}
*/
static async getAnimatedElements(passContext, mainThreadEvents) {
static async getAnimatedElements(context, mainThreadEvents) {
/** @type {Map<string, {begin: LH.TraceEvent | undefined, status: LH.TraceEvent | undefined}>} */
const animationPairs = new Map();
for (const event of mainThreadEvents) {
Expand Down Expand Up @@ -229,7 +235,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.resolveAnimationName(context, animationId);
animations.push({name: animationName, failureReasonsMask, unsupportedProperties});
}
animatedElementData.push({nodeId, animations});
Expand All @@ -238,30 +244,30 @@ 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.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @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 TraceElements.getAnimatedElements(context, mainThreadEvents);

/** @type {Map<string, TraceElementData[]>} */
const backendNodeDataMap = new Map([
Expand All @@ -276,9 +282,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 +314,28 @@ class TraceElements extends Gatherer {
}
}

await driver.sendCommand('Animation.disable');
session.sendCommand('Animation.disable');
Copy link
Collaborator

Choose a reason for hiding this comment

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

should these be in stopInstrumentation? does it matter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is tricky, I'm assuming that Animation domain needs to remain enabled for Animation.resolveAnimation to work?

if not, then yeah definitely move to stopInstrumentation

Copy link
Collaborator

Choose a reason for hiding this comment

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

why was the await removed though? I seem to recall bad things happening in LR if there are unhandled promise rejections

Copy link
Collaborator

Choose a reason for hiding this comment

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

alternate option: collect animation id -> name mapping by listening for Animation.animationCreated and fetching at the end of stopInstrumentation

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this and yeah, Animation domain needs to be enabled. When I moved it back I forgot the await though haha.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what do you think about the alternate option @adamraine? it's more code and possible more animation resolution at the benefit of more idiomatic "instrumentation" followed by the artifact computation.

I could see us going either way, just interested in your thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're still going to have some "instrumentation" in getArtifact when we call Runtime.callFunctionOn. Some less destructive options I came up with:

  • Remove the call to Animation.disable entirely
  • Call Animation.disable in stopInstrumentation, then reenable it for getArtifact.

I'll give your suggestion a shot though to see how destructive it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I went a little deep down this rabbit hole. I had trouble resolving the animation name from animationCreated events, however I found that we can just get the animation name directly from animationStarted events. Kinda wish I had thought of this when I first added animated elements to TraceElements 😆


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: {}};
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
33 changes: 30 additions & 3 deletions lighthouse-core/test/gather/gatherers/trace-elements-test.js
View file
Open in desktop
Expand Up @@ -74,6 +74,14 @@ function makeLCPTraceEvent(nodeId) {
};
}

it('startInstrumentation', async () => {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
const sendCommand = jest.fn();
await new TraceElementsGatherer().startInstrumentation({
driver: {defaultSession: {sendCommand}},
});
expect(sendCommand).toHaveBeenCalledWith('Animation.enable');
});

describe('Trace Elements gatherer - GetTopLayoutShiftElements', () => {
/**
* @param {Array<{nodeId: number, score: number}>} shiftScores
Expand Down Expand Up @@ -529,7 +537,7 @@ describe('Trace Elements gatherer - Animated Elements', () => {
trace.traceEvents.push(makeLCPTraceEvent(6));

const gatherer = new TraceElementsGatherer();
const result = await gatherer.afterPass({driver}, {trace});
const result = await gatherer._getArtifact({driver}, trace);

expect(result).toEqual([
{
Expand Down Expand Up @@ -632,7 +640,7 @@ describe('Trace Elements gatherer - Animated Elements', () => {
const driver = new Driver(connectionStub);
const gatherer = new TraceElementsGatherer();

const result = await gatherer.afterPass({driver}, {trace: animationTrace});
const result = await gatherer._getArtifact({driver}, animationTrace);

expect(result).toEqual([
{
Expand Down Expand Up @@ -724,7 +732,7 @@ describe('Trace Elements gatherer - Animated Elements', () => {
trace.traceEvents.push(makeLCPTraceEvent(7));

const gatherer = new TraceElementsGatherer();
const result = await gatherer.afterPass({driver}, {trace});
const result = await gatherer._getArtifact({driver}, trace);

expect(result).toEqual([
{
Expand All @@ -741,3 +749,22 @@ describe('Trace Elements gatherer - Animated Elements', () => {
]);
});
});

describe('FR compat', () => {
it('uses loadData in legacy mode', async () => {
const trace = ['1', '2'];
const gatherer = new TraceElementsGatherer();
gatherer._getArtifact = jest.fn();
await gatherer.afterPass({}, {trace});
expect(gatherer._getArtifact).toHaveBeenCalledWith({dependencies: {}}, trace);
});

it('uses dependency in legacy mode', async () => {
const trace = ['1', '2'];
const gatherer = new TraceElementsGatherer();
gatherer._getArtifact = jest.fn();
const context = {dependencies: {Trace: trace}};
await gatherer.getArtifact(context);
expect(gatherer._getArtifact).toHaveBeenCalledWith(context, trace);
});
});
1 change: 0 additions & 1 deletion types/artifacts.d.ts
View file
Open in desktop
Expand Up @@ -37,7 +37,6 @@ declare global {
| 'ServiceWorker'
| 'SourceMaps'
| 'TagsBlockingFirstPaint'
| 'TraceElements'
| keyof FRBaseArtifacts
>;

Expand Down