From abbff102d1a7ad5e64ace40cd4792a763d12c068 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 4 May 2021 18:32:56 -0400 Subject: [PATCH 1/9] core(fr): convert trace-elements gatherer --- .../gather/gatherers/trace-elements.js | 67 +++++++++++++------ .../gather/gatherers/trace-elements-test.js | 36 ++++++++++ 2 files changed, 82 insertions(+), 21 deletions(-) diff --git a/lighthouse-core/gather/gatherers/trace-elements.js b/lighthouse-core/gather/gatherers/trace-elements.js index a41fcbc936fd..2984f1607027 100644 --- a/lighthouse-core/gather/gatherers/trace-elements.js +++ b/lighthouse-core/gather/gatherers/trace-elements.js @@ -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 */ @@ -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} @@ -89,17 +96,17 @@ class TraceElements extends Gatherer { } /** - * @param {LH.Gatherer.PassContext} passContext + * @param {LH.Gatherer.FRTransitionalContext} passContext * @param {string} animationId * @return {Promise} */ static async resolveAnimationName(passContext, animationId) { - const driver = passContext.driver; + const session = passContext.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'); @@ -112,7 +119,6 @@ class TraceElements extends Gatherer { tags: {gatherer: TraceElements.name}, level: 'error', }); - return undefined; } } @@ -186,7 +192,7 @@ 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} passContext * @param {Array} mainThreadEvents * @return {Promise>} */ @@ -238,25 +244,25 @@ class TraceElements extends Gatherer { } /** - * @param {LH.Gatherer.PassContext} passContext + * @param {LH.Gatherer.FRTransitionalContext} passContext */ - async beforePass(passContext) { - await passContext.driver.sendCommand('Animation.enable'); + async startInstrumentation(passContext) { + await passContext.driver.defaultSession.sendCommand('Animation.enable'); } /** - * @param {LH.Gatherer.PassContext} passContext - * @param {LH.Gatherer.LoadData} loadData + * @param {LH.Gatherer.FRTransitionalContext} passContext + * @param {LH.Trace|undefined} trace * @return {Promise} */ - async afterPass(passContext, loadData) { - const driver = passContext.driver; - if (!loadData.trace) { + async _getArtifact(passContext, trace) { + const session = passContext.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); @@ -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()}; @@ -308,10 +314,29 @@ class TraceElements extends Gatherer { } } - await driver.sendCommand('Animation.disable'); + session.sendCommand('Animation.disable'); return traceElements; } + + /** + * @param {LH.Gatherer.FRTransitionalContext<'Trace'>} passContext + * @return {Promise} + */ + async getArtifact(passContext) { + return this._getArtifact(passContext, passContext.dependencies.Trace); + } + + /** + * @param {LH.Gatherer.PassContext} passContext + * @param {LH.Gatherer.LoadData} loadData + * @return {Promise} + */ + async afterPass(passContext, loadData) { + const context = {...passContext, dependencies: {}}; + await this.stopInstrumentation(context); + return this._getArtifact(context, loadData.trace); + } } module.exports = TraceElements; diff --git a/lighthouse-core/test/gather/gatherers/trace-elements-test.js b/lighthouse-core/test/gather/gatherers/trace-elements-test.js index d65a8c137a4b..ccd892d88be0 100644 --- a/lighthouse-core/test/gather/gatherers/trace-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/trace-elements-test.js @@ -74,6 +74,14 @@ function makeLCPTraceEvent(nodeId) { }; } +it('startInstrumentation', async () => { + 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 @@ -741,3 +749,31 @@ describe('Trace Elements gatherer - Animated Elements', () => { ]); }); }); + +describe('FR compat', () => { + /** @type {TraceElementsGatherer} */ + let gatherer; + + beforeEach(() => { + gatherer = new TraceElementsGatherer(); + gatherer._getArtifact = jest.fn(); + gatherer.stopInstrumentation = jest.fn(); + }); + + it('uses loadData in legacy mode', async () => { + const trace = ['1', '2']; + await gatherer.afterPass({}, {trace}); + expect(gatherer._getArtifact).toHaveBeenCalledWith({dependencies: {}}, trace); + expect(gatherer.stopInstrumentation).toHaveBeenCalledWith({dependencies: {}}); + }); + + it('uses dependency in legacy mode', async () => { + const trace = ['1', '2']; + const context = { + dependencies: {Trace: trace}, + }; + await gatherer.getArtifact(context); + expect(gatherer._getArtifact).toHaveBeenCalledWith(context, trace); + expect(gatherer.stopInstrumentation).not.toHaveBeenCalled(); + }); +}); From 9070191ea91bdd43c2c7ab3ff7d4316a2275ba6d Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 4 May 2021 18:34:59 -0400 Subject: [PATCH 2/9] rn context --- .../gather/gatherers/trace-elements.js | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/lighthouse-core/gather/gatherers/trace-elements.js b/lighthouse-core/gather/gatherers/trace-elements.js index 2984f1607027..2edf1a7bc048 100644 --- a/lighthouse-core/gather/gatherers/trace-elements.js +++ b/lighthouse-core/gather/gatherers/trace-elements.js @@ -96,12 +96,12 @@ class TraceElements extends FRGatherer { } /** - * @param {LH.Gatherer.FRTransitionalContext} passContext + * @param {LH.Gatherer.FRTransitionalContext} context * @param {string} animationId * @return {Promise} */ - static async resolveAnimationName(passContext, animationId) { - const session = passContext.driver.defaultSession; + static async resolveAnimationName(context, animationId) { + const session = context.driver.defaultSession; try { const result = await session.sendCommand('Animation.resolveAnimation', {animationId}); const objectId = result.remoteObject.objectId; @@ -192,11 +192,11 @@ class TraceElements extends FRGatherer { /** * Find the node ids of elements which are animated using the Animation trace events. - * @param {LH.Gatherer.FRTransitionalContext} passContext + * @param {LH.Gatherer.FRTransitionalContext} context * @param {Array} mainThreadEvents * @return {Promise>} */ - static async getAnimatedElements(passContext, mainThreadEvents) { + static async getAnimatedElements(context, mainThreadEvents) { /** @type {Map} */ const animationPairs = new Map(); for (const event of mainThreadEvents) { @@ -235,7 +235,7 @@ class TraceElements extends FRGatherer { 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}); @@ -244,19 +244,19 @@ class TraceElements extends FRGatherer { } /** - * @param {LH.Gatherer.FRTransitionalContext} passContext + * @param {LH.Gatherer.FRTransitionalContext} context */ - async startInstrumentation(passContext) { - await passContext.driver.defaultSession.sendCommand('Animation.enable'); + async startInstrumentation(context) { + await context.driver.defaultSession.sendCommand('Animation.enable'); } /** - * @param {LH.Gatherer.FRTransitionalContext} passContext + * @param {LH.Gatherer.FRTransitionalContext} context * @param {LH.Trace|undefined} trace * @return {Promise} */ - async _getArtifact(passContext, trace) { - const session = passContext.driver.defaultSession; + async _getArtifact(context, trace) { + const session = context.driver.defaultSession; if (!trace) { throw new Error('Trace is missing!'); } @@ -267,7 +267,7 @@ class TraceElements extends FRGatherer { const lcpNodeId = TraceElements.getNodeIDFromTraceEvent(largestContentfulPaintEvt); const clsNodeData = TraceElements.getTopLayoutShiftElements(mainThreadEvents); const animatedElementData = - await TraceElements.getAnimatedElements(passContext, mainThreadEvents); + await TraceElements.getAnimatedElements(context, mainThreadEvents); /** @type {Map} */ const backendNodeDataMap = new Map([ @@ -320,11 +320,11 @@ class TraceElements extends FRGatherer { } /** - * @param {LH.Gatherer.FRTransitionalContext<'Trace'>} passContext + * @param {LH.Gatherer.FRTransitionalContext<'Trace'>} context * @return {Promise} */ - async getArtifact(passContext) { - return this._getArtifact(passContext, passContext.dependencies.Trace); + async getArtifact(context) { + return this._getArtifact(context, context.dependencies.Trace); } /** @@ -334,7 +334,6 @@ class TraceElements extends FRGatherer { */ async afterPass(passContext, loadData) { const context = {...passContext, dependencies: {}}; - await this.stopInstrumentation(context); return this._getArtifact(context, loadData.trace); } } From ffe39dcf654350307fa7d6b5f1c81ad50ece0528 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 4 May 2021 18:35:52 -0400 Subject: [PATCH 3/9] fix tests --- lighthouse-core/test/gather/gatherers/trace-elements-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/trace-elements-test.js b/lighthouse-core/test/gather/gatherers/trace-elements-test.js index ccd892d88be0..ec6be4daaa5a 100644 --- a/lighthouse-core/test/gather/gatherers/trace-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/trace-elements-test.js @@ -764,7 +764,6 @@ describe('FR compat', () => { const trace = ['1', '2']; await gatherer.afterPass({}, {trace}); expect(gatherer._getArtifact).toHaveBeenCalledWith({dependencies: {}}, trace); - expect(gatherer.stopInstrumentation).toHaveBeenCalledWith({dependencies: {}}); }); it('uses dependency in legacy mode', async () => { @@ -774,6 +773,5 @@ describe('FR compat', () => { }; await gatherer.getArtifact(context); expect(gatherer._getArtifact).toHaveBeenCalledWith(context, trace); - expect(gatherer.stopInstrumentation).not.toHaveBeenCalled(); }); }); From 10cf22977844df70c7b1fb1b96609a121ce4676e Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 4 May 2021 18:39:58 -0400 Subject: [PATCH 4/9] convert old tests --- .../test/gather/gatherers/trace-elements-test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/trace-elements-test.js b/lighthouse-core/test/gather/gatherers/trace-elements-test.js index ec6be4daaa5a..c0f4847c94e0 100644 --- a/lighthouse-core/test/gather/gatherers/trace-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/trace-elements-test.js @@ -537,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([ { @@ -640,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([ { @@ -732,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([ { From e30f536db6e98e667ebb582aee5dcdbdc6c96458 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 4 May 2021 18:44:00 -0400 Subject: [PATCH 5/9] i cant decide --- .../gather/gatherers/trace-elements-test.js | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/trace-elements-test.js b/lighthouse-core/test/gather/gatherers/trace-elements-test.js index c0f4847c94e0..4d45b3d4bd42 100644 --- a/lighthouse-core/test/gather/gatherers/trace-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/trace-elements-test.js @@ -751,26 +751,19 @@ describe('Trace Elements gatherer - Animated Elements', () => { }); describe('FR compat', () => { - /** @type {TraceElementsGatherer} */ - let gatherer; - - beforeEach(() => { - gatherer = new TraceElementsGatherer(); - gatherer._getArtifact = jest.fn(); - gatherer.stopInstrumentation = jest.fn(); - }); - 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 context = { - dependencies: {Trace: trace}, - }; + const gatherer = new TraceElementsGatherer(); + gatherer._getArtifact = jest.fn(); + const context = {dependencies: {Trace: trace}}; await gatherer.getArtifact(context); expect(gatherer._getArtifact).toHaveBeenCalledWith(context, trace); }); From f73fc4b4c27c29ae54eef107745225d0e7b293d5 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 4 May 2021 18:51:06 -0400 Subject: [PATCH 6/9] add to config --- lighthouse-core/fraggle-rock/config/default-config.js | 3 +++ lighthouse-core/test/fraggle-rock/api-test-pptr.js | 2 +- types/artifacts.d.ts | 1 - 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/fraggle-rock/config/default-config.js b/lighthouse-core/fraggle-rock/config/default-config.js index 6050ef52c2c5..e487492f940e 100644 --- a/lighthouse-core/fraggle-rock/config/default-config.js +++ b/lighthouse-core/fraggle-rock/config/default-config.js @@ -31,6 +31,7 @@ const artifacts = { RobotsTxt: '', Stacks: '', TapTargets: '', + TraceElements: '', ViewportDimensions: '', WebAppManifest: '', devtoolsLogs: '', @@ -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 */ @@ -103,6 +105,7 @@ const defaultConfig = { artifacts.RobotsTxt, artifacts.Stacks, artifacts.TapTargets, + artifacts.TraceElements, artifacts.ViewportDimensions, artifacts.WebAppManifest, diff --git a/lighthouse-core/test/fraggle-rock/api-test-pptr.js b/lighthouse-core/test/fraggle-rock/api-test-pptr.js index 42536ede532a..5f60136cbe0c 100644 --- a/lighthouse-core/test/fraggle-rock/api-test-pptr.js +++ b/lighthouse-core/test/fraggle-rock/api-test-pptr.js @@ -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`); expect(erroredAudits).toHaveLength(0); const failedAuditIds = failedAudits.map(audit => audit.id); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 18ca6142befe..f2c81cd18936 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -37,7 +37,6 @@ declare global { | 'ServiceWorker' | 'SourceMaps' | 'TagsBlockingFirstPaint' - | 'TraceElements' | keyof FRBaseArtifacts >; From 8cd5172835c313e74764391ff076c6f29945281c Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 5 May 2021 14:02:54 -0400 Subject: [PATCH 7/9] use instrumentation --- .../gather/gatherers/trace-elements.js | 65 +++---- .../gather/gatherers/trace-elements-test.js | 174 ++++++++---------- 2 files changed, 108 insertions(+), 131 deletions(-) diff --git a/lighthouse-core/gather/gatherers/trace-elements.js b/lighthouse-core/gather/gatherers/trace-elements.js index 2edf1a7bc048..c50aab9b6419 100644 --- a/lighthouse-core/gather/gatherers/trace-elements.js +++ b/lighthouse-core/gather/gatherers/trace-elements.js @@ -45,6 +45,9 @@ class TraceElements extends FRGatherer { dependencies: {Trace: Trace.symbol}, } + /** @type {Map} */ + animationIdToName = new Map(); + /** * @param {LH.TraceEvent | undefined} event * @return {number | undefined} @@ -95,33 +98,6 @@ class TraceElements extends FRGatherer { return RectHelpers.addRectTopAndBottom(rectArgs); } - /** - * @param {LH.Gatherer.FRTransitionalContext} context - * @param {string} animationId - * @return {Promise} - */ - static async resolveAnimationName(context, animationId) { - const session = context.driver.defaultSession; - try { - const result = await session.sendCommand('Animation.resolveAnimation', {animationId}); - const objectId = result.remoteObject.objectId; - if (!objectId) return undefined; - const response = await session.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', - }); - } - } - /** * 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). @@ -192,11 +168,10 @@ class TraceElements extends FRGatherer { /** * Find the node ids of elements which are animated using the Animation trace events. - * @param {LH.Gatherer.FRTransitionalContext} context * @param {Array} mainThreadEvents * @return {Promise>} */ - static async getAnimatedElements(context, mainThreadEvents) { + async getAnimatedElements(mainThreadEvents) { /** @type {Map} */ const animationPairs = new Map(); for (const event of mainThreadEvents) { @@ -220,10 +195,10 @@ class TraceElements extends FRGatherer { /** @type {Map>} */ 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}); @@ -235,7 +210,7 @@ class TraceElements extends FRGatherer { for (const [nodeId, animationIds] of elementAnimations) { const animations = []; for (const {animationId, failureReasonsMask, unsupportedProperties} of animationIds) { - const animationName = await this.resolveAnimationName(context, animationId); + const animationName = await this.animationIdToName.get(animationId); animations.push({name: animationName, failureReasonsMask, unsupportedProperties}); } animatedElementData.push({nodeId, animations}); @@ -248,6 +223,23 @@ class TraceElements extends FRGatherer { */ 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); + }; + + context.driver.defaultSession.on('Animation.animationStarted', this.onAnimationCreated); + } + + /** + * @param {LH.Gatherer.FRTransitionalContext} context + */ + async stopInstrumentation(context) { + if (this.onAnimationCreated) { + context.driver.defaultSession.off('Animation.animationStarted', this.onAnimationCreated); + } + await context.driver.defaultSession.sendCommand('Animation.disable'); } /** @@ -267,7 +259,7 @@ class TraceElements extends FRGatherer { const lcpNodeId = TraceElements.getNodeIDFromTraceEvent(largestContentfulPaintEvt); const clsNodeData = TraceElements.getTopLayoutShiftElements(mainThreadEvents); const animatedElementData = - await TraceElements.getAnimatedElements(context, mainThreadEvents); + await this.getAnimatedElements(mainThreadEvents); /** @type {Map} */ const backendNodeDataMap = new Map([ @@ -314,8 +306,6 @@ class TraceElements extends FRGatherer { } } - session.sendCommand('Animation.disable'); - return traceElements; } @@ -334,6 +324,7 @@ class TraceElements extends FRGatherer { */ async afterPass(passContext, loadData) { const context = {...passContext, dependencies: {}}; + await this.stopInstrumentation(context); return this._getArtifact(context, loadData.trace); } } diff --git a/lighthouse-core/test/gather/gatherers/trace-elements-test.js b/lighthouse-core/test/gather/gatherers/trace-elements-test.js index 4d45b3d4bd42..4c1a4d63971b 100644 --- a/lighthouse-core/test/gather/gatherers/trace-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/trace-elements-test.js @@ -11,10 +11,12 @@ const TraceElementsGatherer = require('../../../gather/gatherers/trace-elements. const Driver = require('../../../gather/driver.js'); const Connection = require('../../../gather/connections/connection.js'); const createTestTrace = require('../../create-test-trace.js'); -const {createMockSendCommandFn} = require('../mock-commands.js'); +const {createMockSendCommandFn, createMockOnFn} = require('../mock-commands.js'); const animationTrace = require('../../fixtures/traces/animation.json'); +jest.useFakeTimers(); + function makeLayoutShiftTraceEvent(score, impactedNodes, had_recent_input = false) { return { name: 'LayoutShift', @@ -74,14 +76,6 @@ function makeLCPTraceEvent(nodeId) { }; } -it('startInstrumentation', async () => { - 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 @@ -326,24 +320,6 @@ describe('Trace Elements gatherer - GetTopLayoutShiftElements', () => { describe('Trace Elements gatherer - Animated Elements', () => { it('gets animated node ids with non-composited animations', async () => { - const connectionStub = new Connection(); - connectionStub.sendCommand = createMockSendCommandFn() - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 1}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'alpha'}, - }]}) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 2}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: ''}, - }]}) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 3}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'beta'}, - }]}); - const driver = new Driver(connectionStub); const traceEvents = [ makeAnimationTraceEvent('0x363db876c1', 'b', {id: '1', nodeId: 5}), makeAnimationTraceEvent('0x363db876c1', 'n', { @@ -362,7 +338,11 @@ describe('Trace Elements gatherer - Animated Elements', () => { }), ]; - const result = await TraceElementsGatherer.getAnimatedElements({driver}, traceEvents); + const gatherer = new TraceElementsGatherer(); + gatherer.animationIdToName.set('1', 'alpha'); + gatherer.animationIdToName.set('3', 'beta'); + + const result = await gatherer.getAnimatedElements(traceEvents); expect(result).toEqual([ {nodeId: 5, animations: [ {name: 'alpha', failureReasonsMask: 8192, unsupportedProperties: ['height']}, @@ -375,19 +355,6 @@ describe('Trace Elements gatherer - Animated Elements', () => { }); it('get non-composited animations with no unsupported properties', async () => { - const connectionStub = new Connection(); - connectionStub.sendCommand = createMockSendCommandFn() - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 1}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'alpha'}, - }]}) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 2}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: ''}, - }]}); - const driver = new Driver(connectionStub); const traceEvents = [ makeAnimationTraceEvent('0x363db876c1', 'b', {id: '1', nodeId: 5}), makeAnimationTraceEvent('0x363db876c1', 'n', { @@ -401,7 +368,10 @@ describe('Trace Elements gatherer - Animated Elements', () => { }), ]; - const result = await TraceElementsGatherer.getAnimatedElements({driver}, traceEvents); + const gatherer = new TraceElementsGatherer(); + gatherer.animationIdToName.set('1', 'alpha'); + + const result = await gatherer.getAnimatedElements(traceEvents); expect(result).toEqual([ {nodeId: 5, animations: [ {name: 'alpha', failureReasonsMask: 2048, unsupportedProperties: []}, @@ -411,24 +381,6 @@ describe('Trace Elements gatherer - Animated Elements', () => { }); it('gets animated node ids with composited animations', async () => { - const connectionStub = new Connection(); - connectionStub.sendCommand = createMockSendCommandFn() - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 1}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'alpha'}, - }]}) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 2}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: ''}, - }]}) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 3}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'beta'}, - }]}); - const driver = new Driver(connectionStub); const traceEvents = [ makeAnimationTraceEvent('0x363db876c1', 'b', {id: '1', nodeId: 5}), makeAnimationTraceEvent('0x363db876c1', 'n', {compositeFailed: 0, unsupportedProperties: []}), @@ -438,7 +390,11 @@ describe('Trace Elements gatherer - Animated Elements', () => { makeAnimationTraceEvent('0x363db876c3', 'n', {compositeFailed: 0, unsupportedProperties: []}), ]; - const result = await TraceElementsGatherer.getAnimatedElements({driver}, traceEvents); + const gatherer = new TraceElementsGatherer(); + gatherer.animationIdToName.set('1', 'alpha'); + gatherer.animationIdToName.set('3', 'beta'); + + const result = await gatherer.getAnimatedElements(traceEvents); expect(result).toEqual([ {nodeId: 5, animations: [ {name: 'alpha', failureReasonsMask: 0, unsupportedProperties: []}, @@ -505,11 +461,6 @@ describe('Trace Elements gatherer - Animated Elements', () => { .mockResponse('DOM.resolveNode', () => { // 2nd CLS node throw Error('No node found'); }) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 4}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'example'}, - }]}) .mockResponse('DOM.resolveNode', {object: {objectId: 3}}) .mockResponse('Runtime.callFunctionOn', {result: {value: animationNodeData}}); const driver = new Driver(connectionStub); @@ -537,6 +488,8 @@ describe('Trace Elements gatherer - Animated Elements', () => { trace.traceEvents.push(makeLCPTraceEvent(6)); const gatherer = new TraceElementsGatherer(); + gatherer.animationIdToName.set('1', 'example'); + const result = await gatherer._getArtifact({driver}, trace); expect(result).toEqual([ @@ -611,34 +564,17 @@ describe('Trace Elements gatherer - Animated Elements', () => { .mockResponse('DOM.resolveNode', {object: {objectId: 1}}) .mockResponse('Runtime.callFunctionOn', {result: {value: LCPNodeData}}) // Animated node - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 2}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: ''}, - }]}) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 3}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'alpha'}, - }]}) - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 4}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'beta'}, - }]}) .mockResponse('DOM.resolveNode', {object: {objectId: 5}}) .mockResponse('Runtime.callFunctionOn', {result: {value: animationNodeData}}) // Composited node - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 6}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'gamma'}, - }]}) .mockResponse('DOM.resolveNode', {object: {objectId: 7}}) .mockResponse('Runtime.callFunctionOn', {result: {value: compositedNodeData}}); const driver = new Driver(connectionStub); const gatherer = new TraceElementsGatherer(); + gatherer.animationIdToName.set('2', 'alpha'); + gatherer.animationIdToName.set('3', 'beta'); + gatherer.animationIdToName.set('4', 'gamma'); const result = await gatherer._getArtifact({driver}, animationTrace); @@ -702,18 +638,10 @@ describe('Trace Elements gatherer - Animated Elements', () => { .mockResponse('DOM.resolveNode', {object: {objectId: 1}}) .mockResponse('Runtime.callFunctionOn', {result: {value: LCPNodeData}}) // Animation 1 - .mockResponse('Animation.resolveAnimation', () => { - throw Error(); - }) .mockResponse('DOM.resolveNode', () => { throw Error(); }) // Animation 2 - .mockResponse('Animation.resolveAnimation', {remoteObject: {objectId: 4}}) - .mockResponse('Runtime.getProperties', {result: [{ - name: 'animationName', - value: {type: 'string', value: 'example'}, - }]}) .mockResponse('DOM.resolveNode', {object: {objectId: 5}}) .mockResponse('Runtime.callFunctionOn', {result: {value: animationNodeData}}); const driver = new Driver(connectionStub); @@ -732,6 +660,9 @@ describe('Trace Elements gatherer - Animated Elements', () => { trace.traceEvents.push(makeLCPTraceEvent(7)); const gatherer = new TraceElementsGatherer(); + gatherer.animationIdToName.set('1', 'notgunnamatter'); + gatherer.animationIdToName.set('2', 'example'); + const result = await gatherer._getArtifact({driver}, trace); expect(result).toEqual([ @@ -750,21 +681,76 @@ describe('Trace Elements gatherer - Animated Elements', () => { }); }); +describe('instrumentation', () => { + it('resolves animation name', async () => { + const connectionStub = new Connection(); + connectionStub.on = createMockOnFn() + .mockEvent('protocolevent', { + method: 'Animation.animationStarted', + params: {animation: {id: '1', name: 'example'}}, + }); + connectionStub.sendCommand = createMockSendCommandFn() + .mockResponse('Animation.enable') + .mockResponse('Animation.disable'); + const driver = new Driver(connectionStub); + const gatherer = new TraceElementsGatherer(); + await gatherer.startInstrumentation({driver}); + + // Flush events. + jest.advanceTimersByTime(1); + + await gatherer.stopInstrumentation({driver}); + + expect(gatherer.animationIdToName.size).toEqual(1); + expect(gatherer.animationIdToName.get('1')).toEqual('example'); + }); + + it('ignores empty name', async () => { + const connectionStub = new Connection(); + connectionStub.on = createMockOnFn() + .mockEvent('protocolevent', { + method: 'Animation.animationStarted', + params: {animation: {id: '1', name: ''}}, + }); + connectionStub.sendCommand = createMockSendCommandFn() + .mockResponse('Animation.enable') + .mockResponse('Animation.disable'); + const driver = new Driver(connectionStub); + const gatherer = new TraceElementsGatherer(); + await gatherer.startInstrumentation({driver}); + + // Flush events. + jest.advanceTimersByTime(1); + + await gatherer.stopInstrumentation({driver}); + + expect(gatherer.animationIdToName.size).toEqual(0); + }); +}); + describe('FR compat', () => { it('uses loadData in legacy mode', async () => { const trace = ['1', '2']; const gatherer = new TraceElementsGatherer(); gatherer._getArtifact = jest.fn(); + gatherer.stopInstrumentation = jest.fn(); + await gatherer.afterPass({}, {trace}); + expect(gatherer._getArtifact).toHaveBeenCalledWith({dependencies: {}}, trace); + expect(gatherer.stopInstrumentation).toHaveBeenCalledWith({dependencies: {}}); }); it('uses dependency in legacy mode', async () => { const trace = ['1', '2']; const gatherer = new TraceElementsGatherer(); gatherer._getArtifact = jest.fn(); + gatherer.stopInstrumentation = jest.fn(); + const context = {dependencies: {Trace: trace}}; await gatherer.getArtifact(context); + expect(gatherer._getArtifact).toHaveBeenCalledWith(context, trace); + expect(gatherer.stopInstrumentation).not.toHaveBeenCalled(); }); }); From 4db9ee9a2a4448821671683b8a2b21bd2174520b Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 5 May 2021 16:12:22 -0400 Subject: [PATCH 8/9] comments --- .../gather/gatherers/trace-elements.js | 22 +++++++++---------- .../gather/gatherers/trace-elements-test.js | 7 +++--- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/lighthouse-core/gather/gatherers/trace-elements.js b/lighthouse-core/gather/gatherers/trace-elements.js index c50aab9b6419..c57b20b7c570 100644 --- a/lighthouse-core/gather/gatherers/trace-elements.js +++ b/lighthouse-core/gather/gatherers/trace-elements.js @@ -48,6 +48,14 @@ class TraceElements extends FRGatherer { /** @type {Map} */ animationIdToName = new Map(); + constructor() { + super(); + /** @param {LH.Crdp.Animation.AnimationStartedEvent} args */ + this.onAnimationStarted = ({animation: {id, name}}) => { + if (name) this.animationIdToName.set(id, name); + }; + } + /** * @param {LH.TraceEvent | undefined} event * @return {number | undefined} @@ -210,7 +218,7 @@ class TraceElements extends FRGatherer { for (const [nodeId, animationIds] of elementAnimations) { const animations = []; for (const {animationId, failureReasonsMask, unsupportedProperties} of animationIds) { - const animationName = await this.animationIdToName.get(animationId); + const animationName = this.animationIdToName.get(animationId); animations.push({name: animationName, failureReasonsMask, unsupportedProperties}); } animatedElementData.push({nodeId, animations}); @@ -223,22 +231,14 @@ class TraceElements extends FRGatherer { */ 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); - }; - - context.driver.defaultSession.on('Animation.animationStarted', this.onAnimationCreated); + context.driver.defaultSession.on('Animation.animationStarted', this.onAnimationStarted); } /** * @param {LH.Gatherer.FRTransitionalContext} context */ async stopInstrumentation(context) { - if (this.onAnimationCreated) { - context.driver.defaultSession.off('Animation.animationStarted', this.onAnimationCreated); - } + context.driver.defaultSession.off('Animation.animationStarted', this.onAnimationStarted); await context.driver.defaultSession.sendCommand('Animation.disable'); } diff --git a/lighthouse-core/test/gather/gatherers/trace-elements-test.js b/lighthouse-core/test/gather/gatherers/trace-elements-test.js index 4c1a4d63971b..629a7cc2b0fd 100644 --- a/lighthouse-core/test/gather/gatherers/trace-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/trace-elements-test.js @@ -12,6 +12,7 @@ const Driver = require('../../../gather/driver.js'); const Connection = require('../../../gather/connections/connection.js'); const createTestTrace = require('../../create-test-trace.js'); const {createMockSendCommandFn, createMockOnFn} = require('../mock-commands.js'); +const {flushAllTimersAndMicrotasks} = require('../../test-utils.js'); const animationTrace = require('../../fixtures/traces/animation.json'); @@ -696,8 +697,7 @@ describe('instrumentation', () => { const gatherer = new TraceElementsGatherer(); await gatherer.startInstrumentation({driver}); - // Flush events. - jest.advanceTimersByTime(1); + await flushAllTimersAndMicrotasks(); await gatherer.stopInstrumentation({driver}); @@ -719,8 +719,7 @@ describe('instrumentation', () => { const gatherer = new TraceElementsGatherer(); await gatherer.startInstrumentation({driver}); - // Flush events. - jest.advanceTimersByTime(1); + await flushAllTimersAndMicrotasks(); await gatherer.stopInstrumentation({driver}); From 4935e46b8bd6635aa8d18c177aa47a76cf798ca8 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 5 May 2021 16:49:21 -0400 Subject: [PATCH 9/9] bind --- lighthouse-core/gather/gatherers/trace-elements.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/gather/gatherers/trace-elements.js b/lighthouse-core/gather/gatherers/trace-elements.js index c57b20b7c570..bd8ca6d4f57f 100644 --- a/lighthouse-core/gather/gatherers/trace-elements.js +++ b/lighthouse-core/gather/gatherers/trace-elements.js @@ -50,10 +50,12 @@ class TraceElements extends FRGatherer { constructor() { super(); - /** @param {LH.Crdp.Animation.AnimationStartedEvent} args */ - this.onAnimationStarted = ({animation: {id, name}}) => { - if (name) this.animationIdToName.set(id, name); - }; + this._onAnimationStarted = this._onAnimationStarted.bind(this); + } + + /** @param {LH.Crdp.Animation.AnimationStartedEvent} args */ + _onAnimationStarted({animation: {id, name}}) { + if (name) this.animationIdToName.set(id, name); } /** @@ -231,14 +233,14 @@ class TraceElements extends FRGatherer { */ async startInstrumentation(context) { await context.driver.defaultSession.sendCommand('Animation.enable'); - context.driver.defaultSession.on('Animation.animationStarted', this.onAnimationStarted); + context.driver.defaultSession.on('Animation.animationStarted', this._onAnimationStarted); } /** * @param {LH.Gatherer.FRTransitionalContext} context */ async stopInstrumentation(context) { - context.driver.defaultSession.off('Animation.animationStarted', this.onAnimationStarted); + context.driver.defaultSession.off('Animation.animationStarted', this._onAnimationStarted); await context.driver.defaultSession.sendCommand('Animation.disable'); }