From d7d5b34c3bea068237d9a9d6596669c5ae37e191 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 21 Aug 2018 17:33:40 -0700 Subject: [PATCH] throw on lack of fcp --- .../render-blocking-resources.js | 4 --- lighthouse-core/audits/metrics.js | 4 +-- .../metrics/first-contentful-paint.js | 7 +--- .../gather/computed/metrics/interactive.js | 6 ---- .../metrics/lantern-first-contentful-paint.js | 9 ----- .../gather/computed/trace-of-tab.js | 20 ++++++----- .../test/audits/bootup-time-test.js | 2 +- .../audits/mainthread-work-breakdown-test.js | 2 +- .../backgrounded-tab-missing-paints.json | 2 +- .../test/fixtures/traces/load.json | 1 + .../threeframes-blank_content_more.json | 12 +++++++ .../gather/computed/main-thread-tasks-test.js | 5 +-- .../metrics/first-meaningful-paint-test.js | 9 ----- .../test/gather/computed/trace-of-tab-test.js | 34 +++++++++++++------ typings/artifacts.d.ts | 2 +- 15 files changed, 59 insertions(+), 60 deletions(-) diff --git a/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js b/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js index 07b499c2b537..f823e655ef3c 100644 --- a/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js +++ b/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js @@ -15,7 +15,6 @@ const BaseNode = require('../../lib/dependency-graph/base-node'); const ByteEfficiencyAudit = require('./byte-efficiency-audit'); const UnusedCSS = require('./unused-css-rules'); const NetworkRequest = require('../../lib/network-request'); -const LHError = require('../../lib/errors'); /** @typedef {import('../../lib/dependency-graph/simulator/simulator')} Simulator */ /** @typedef {import('../../lib/dependency-graph/base-node.js').Node} Node */ @@ -92,9 +91,6 @@ class RenderBlockingResources extends Audit { const metricComputationData = {trace, devtoolsLog, simulator, settings: metricSettings}; // @ts-ignore - TODO(bckenny): allow optional `throttling` settings const fcpSimulation = await artifacts.requestFirstContentfulPaint(metricComputationData); - if (!traceOfTab.timestamps.firstContentfulPaint) { - throw new LHError(LHError.errors.NO_FCP); - } const fcpTsInMs = traceOfTab.timestamps.firstContentfulPaint / 1000; const nodesByUrl = getNodesAndTimingByUrl(fcpSimulation.optimisticEstimate.nodeTimings); diff --git a/lighthouse-core/audits/metrics.js b/lighthouse-core/audits/metrics.js index 6523d7452df8..9113cc5be475 100644 --- a/lighthouse-core/audits/metrics.js +++ b/lighthouse-core/audits/metrics.js @@ -117,8 +117,8 @@ class Metrics extends Audit { * @property {number} observedNavigationStartTs * @property {number=} observedFirstPaint * @property {number=} observedFirstPaintTs - * @property {number=} observedFirstContentfulPaint - * @property {number=} observedFirstContentfulPaintTs + * @property {number} observedFirstContentfulPaint + * @property {number} observedFirstContentfulPaintTs * @property {number=} observedFirstMeaningfulPaint * @property {number=} observedFirstMeaningfulPaintTs * @property {number=} observedTraceEnd diff --git a/lighthouse-core/gather/computed/metrics/first-contentful-paint.js b/lighthouse-core/gather/computed/metrics/first-contentful-paint.js index 635c15476532..d82bf4fb60c1 100644 --- a/lighthouse-core/gather/computed/metrics/first-contentful-paint.js +++ b/lighthouse-core/gather/computed/metrics/first-contentful-paint.js @@ -6,7 +6,6 @@ 'use strict'; const MetricArtifact = require('./metric'); -const LHError = require('../../../lib/errors'); class FirstContentfulPaint extends MetricArtifact { get name() { @@ -19,13 +18,9 @@ class FirstContentfulPaint extends MetricArtifact { */ async computeObservedMetric(data) { const {traceOfTab} = data; - if (!traceOfTab.timestamps.firstContentfulPaint) { - throw new LHError(LHError.errors.NO_FCP); - } return { - // FCP established as existing, so cast - timing: /** @type {number} */ (traceOfTab.timings.firstContentfulPaint), + timing: traceOfTab.timings.firstContentfulPaint, timestamp: traceOfTab.timestamps.firstContentfulPaint, }; } diff --git a/lighthouse-core/gather/computed/metrics/interactive.js b/lighthouse-core/gather/computed/metrics/interactive.js index 08ec019c8fe1..53ec137f12c1 100644 --- a/lighthouse-core/gather/computed/metrics/interactive.js +++ b/lighthouse-core/gather/computed/metrics/interactive.js @@ -90,9 +90,6 @@ class Interactive extends MetricArtifact { * @return {{cpuQuietPeriod: TimePeriod, networkQuietPeriod: TimePeriod, cpuQuietPeriods: Array, networkQuietPeriods: Array}} */ static findOverlappingQuietPeriods(longTasks, networkRecords, traceOfTab) { - if (!traceOfTab.timestamps.firstContentfulPaint) { - throw new LHError(LHError.errors.NO_FCP); - } const FcpTsInMs = traceOfTab.timestamps.firstContentfulPaint / 1000; /** @type {function(TimePeriod):boolean} */ @@ -151,9 +148,6 @@ class Interactive extends MetricArtifact { */ computeObservedMetric(data) { const {traceOfTab, networkRecords} = data; - if (!traceOfTab.timestamps.firstContentfulPaint) { - throw new LHError(LHError.errors.NO_FCP); - } if (!traceOfTab.timestamps.domContentLoaded) { throw new LHError(LHError.errors.NO_DCL); diff --git a/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js b/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js index ffbfe775bbfe..1f6cc7f6edd7 100644 --- a/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js +++ b/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js @@ -7,7 +7,6 @@ const MetricArtifact = require('./lantern-metric'); const BaseNode = require('../../../lib/dependency-graph/base-node'); -const LHError = require('../../../lib/errors'); /** @typedef {BaseNode.Node} Node */ @@ -34,10 +33,6 @@ class FirstContentfulPaint extends MetricArtifact { */ getOptimisticGraph(dependencyGraph, traceOfTab) { const fcp = traceOfTab.timestamps.firstContentfulPaint; - if (!fcp) { - throw new LHError(LHError.errors.NO_FCP); - } - const blockingScriptUrls = MetricArtifact.getScriptUrls(dependencyGraph, node => { return ( node.endTime <= fcp && node.hasRenderBlockingPriority() && node.initiatorType !== 'script' @@ -63,10 +58,6 @@ class FirstContentfulPaint extends MetricArtifact { */ getPessimisticGraph(dependencyGraph, traceOfTab) { const fcp = traceOfTab.timestamps.firstContentfulPaint; - if (!fcp) { - throw new LHError(LHError.errors.NO_FCP); - } - const blockingScriptUrls = MetricArtifact.getScriptUrls(dependencyGraph, node => { return node.endTime <= fcp && node.hasRenderBlockingPriority(); }); diff --git a/lighthouse-core/gather/computed/trace-of-tab.js b/lighthouse-core/gather/computed/trace-of-tab.js index baaa3586ef54..a3addf08e535 100644 --- a/lighthouse-core/gather/computed/trace-of-tab.js +++ b/lighthouse-core/gather/computed/trace-of-tab.js @@ -85,10 +85,11 @@ class TraceOfTab extends ComputedArtifact { // Find our first paint of this frame const firstPaint = frameEvents.find(e => e.name === 'firstPaint' && e.ts > navigationStart.ts); - // FCP will follow at/after the FP + // FCP will follow at/after the FP. Used in so many places we require it. const firstContentfulPaint = frameEvents.find( e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts ); + if (!firstContentfulPaint) throw new LHError(LHError.errors.NO_FCP); // fMP will follow at/after the FP let firstMeaningfulPaint = frameEvents.find( @@ -140,24 +141,27 @@ class TraceOfTab extends ComputedArtifact { const timestamps = { navigationStart: navigationStart.ts, firstPaint: getTimestamp(firstPaint), - firstContentfulPaint: getTimestamp(firstContentfulPaint), + firstContentfulPaint: firstContentfulPaint.ts, firstMeaningfulPaint: getTimestamp(firstMeaningfulPaint), traceEnd: fakeEndOfTraceEvt.ts, load: getTimestamp(load), domContentLoaded: getTimestamp(domContentLoaded), }; + + /** @param {number} ts */ + const getTiming = (ts) => (ts - navigationStart.ts) / 1000; /** @param {number=} ts */ - const getTiming = (ts) => ts === undefined ? undefined : (ts - navigationStart.ts) / 1000; + const maybeGetTiming = (ts) => ts === undefined ? undefined : getTiming(ts); /** @type {LH.Artifacts.TraceTimes} */ const timings = { navigationStart: 0, - firstPaint: getTiming(timestamps.firstPaint), + firstPaint: maybeGetTiming(timestamps.firstPaint), firstContentfulPaint: getTiming(timestamps.firstContentfulPaint), - firstMeaningfulPaint: getTiming(timestamps.firstMeaningfulPaint), - traceEnd: (timestamps.traceEnd - navigationStart.ts) / 1000, - load: getTiming(timestamps.load), - domContentLoaded: getTiming(timestamps.domContentLoaded), + firstMeaningfulPaint: maybeGetTiming(timestamps.firstMeaningfulPaint), + traceEnd: getTiming(timestamps.traceEnd), + load: maybeGetTiming(timestamps.load), + domContentLoaded: maybeGetTiming(timestamps.domContentLoaded), }; return { diff --git a/lighthouse-core/test/audits/bootup-time-test.js b/lighthouse-core/test/audits/bootup-time-test.js index 3a3117af04f5..420a9afd7f05 100644 --- a/lighthouse-core/test/audits/bootup-time-test.js +++ b/lighthouse-core/test/audits/bootup-time-test.js @@ -12,7 +12,7 @@ const assert = require('assert'); const acceptableTrace = require('../fixtures/traces/progressive-app-m60.json'); const acceptableDevtoolsLogs = require('../fixtures/traces/progressive-app-m60.devtools.log.json'); -const errorTrace = require('../fixtures/traces/airhorner_no_fcp.json'); +const errorTrace = require('../fixtures/traces/no_fmp_event.json'); describe('Performance: bootup-time audit', () => { const auditOptions = Object.assign({}, BootupTime.defaultOptions, {thresholdInMs: 10}); diff --git a/lighthouse-core/test/audits/mainthread-work-breakdown-test.js b/lighthouse-core/test/audits/mainthread-work-breakdown-test.js index 7061f988133b..5890bab7c7be 100644 --- a/lighthouse-core/test/audits/mainthread-work-breakdown-test.js +++ b/lighthouse-core/test/audits/mainthread-work-breakdown-test.js @@ -14,7 +14,7 @@ const options = PageExecutionTimings.defaultOptions; const acceptableTrace = require('../fixtures/traces/progressive-app-m60.json'); const siteWithRedirectTrace = require('../fixtures/traces/site-with-redirect.json'); const loadTrace = require('../fixtures/traces/load.json'); -const errorTrace = require('../fixtures/traces/airhorner_no_fcp.json'); +const errorTrace = require('../fixtures/traces/no_fmp_event.json'); const acceptableTraceExpectations = { parseHTML: 14, diff --git a/lighthouse-core/test/fixtures/traces/backgrounded-tab-missing-paints.json b/lighthouse-core/test/fixtures/traces/backgrounded-tab-missing-paints.json index 49557d5a2842..d903ac00ab78 100644 --- a/lighthouse-core/test/fixtures/traces/backgrounded-tab-missing-paints.json +++ b/lighthouse-core/test/fixtures/traces/backgrounded-tab-missing-paints.json @@ -91,7 +91,7 @@ "cat": "blink.user_timing,rail", "name": "firstContentfulPaint", "args": { - "frame": "0x1aff390e1e30" + "frame": "0x53965941e30" }, "tts": 239094, "s": "p" diff --git a/lighthouse-core/test/fixtures/traces/load.json b/lighthouse-core/test/fixtures/traces/load.json index 9106ffe97687..0ff31c49de5d 100644 --- a/lighthouse-core/test/fixtures/traces/load.json +++ b/lighthouse-core/test/fixtures/traces/load.json @@ -509,6 +509,7 @@ {"pid":442,"tid":461,"ts":73609802087,"ph":"X","cat":"toplevel","name":"TaskQueueManager::ProcessTaskFromWorkQueue","args":{"src_file":"../../ipc/ipc_channel_proxy.cc","src_func":"OnMessageReceivedNoFilter"},"dur":5851,"tdur":666,"tts":20275155}, {"pid":442,"tid":461,"ts":73609804410,"ph":"I","cat":"disabled-by-default-devtools.timeline","name":"TracingStartedInPage","args":{"data":{"sessionId":"442.138","page":"0xffffffffa1a94000"}},"tts":20275707,"s":"t"}, {"pid":442,"tid":461,"ts":73609804410,"ph":"R","cat":"blink.user_timing","name":"navigationStart","args":{"data":{"sessionId":"442.138"},"frame":"0xffffffffa1a94000"},"tts":20275707,"s":"t"}, +{"pid":442,"tid":461,"ts":73609804428,"ph": "R","cat":"loading,rail,devtools.timeline","name": "firstContentfulPaint","args":{"frame":"0xffffffffa1a94000"},"tts":20275720}, {"pid":442,"tid":461,"ts":73609804428,"ph":"I","cat":"disabled-by-default-devtools.timeline","name":"SetLayerTreeId","args":{"data":{"sessionId":"442.138","layerTreeId":1}},"tts":20275720,"s":"t"}, {"pid":442,"tid":461,"ts":73609807946,"ph":"X","cat":"toplevel","name":"TaskQueueManager::ProcessTaskFromWorkQueue","args":{"src_file":"../../media/audio/fake_audio_worker.cc","src_func":"DoRead"},"dur":88,"tdur":85,"tts":20275831}, {"pid":442,"tid":461,"ts":73609808041,"ph":"X","cat":"toplevel","name":"TaskQueueManager::ProcessTaskFromWorkQueue","args":{"src_file":"../../ipc/ipc_channel_proxy.cc","src_func":"OnMessageReceivedNoFilter"},"dur":193,"tdur":116,"tts":20275927}, diff --git a/lighthouse-core/test/fixtures/traces/threeframes-blank_content_more.json b/lighthouse-core/test/fixtures/traces/threeframes-blank_content_more.json index ab528381c16c..a58b3c2db8e2 100644 --- a/lighthouse-core/test/fixtures/traces/threeframes-blank_content_more.json +++ b/lighthouse-core/test/fixtures/traces/threeframes-blank_content_more.json @@ -45,6 +45,18 @@ "tdur": 186, "tts": 4516530 }, + { + "pid": 93449, + "tid": 1295, + "ts": 900001000000, + "ph": "R", + "cat": "loading,rail,devtools.timeline", + "name": "firstContentfulPaint", + "args": { + "frame": "dummy" + }, + "tts": 63259760 + }, { "pid": 93267, "tid": 1295, diff --git a/lighthouse-core/test/gather/computed/main-thread-tasks-test.js b/lighthouse-core/test/gather/computed/main-thread-tasks-test.js index 2f2ac83fd330..79518a7b33da 100644 --- a/lighthouse-core/test/gather/computed/main-thread-tasks-test.js +++ b/lighthouse-core/test/gather/computed/main-thread-tasks-test.js @@ -23,6 +23,7 @@ describe('MainResource computed artifact', () => { boilerplateTrace = [ {ph: 'I', name: 'TracingStartedInPage', ts: baseTs, args}, {ph: 'I', name: 'navigationStart', ts: baseTs, args}, + {ph: 'R', name: 'firstContentfulPaint', ts: baseTs + 1, args}, ]; computedArtifacts = Runner.instantiateComputedArtifacts(); }); @@ -79,7 +80,7 @@ describe('MainResource computed artifact', () => { attributableURLs: [], children: [taskB], - event: traceEvents[2], + event: traceEvents[3], startTime: 0, endTime: 100, duration: 100, @@ -92,7 +93,7 @@ describe('MainResource computed artifact', () => { attributableURLs: [], children: [taskC], - event: traceEvents[3], + event: traceEvents[4], startTime: 5, endTime: 55, duration: 50, diff --git a/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js b/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js index 278a1bf29dd7..dd57e63f3007 100644 --- a/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js +++ b/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js @@ -16,7 +16,6 @@ const badNavStartTrace = require(`${TRACE_FIXTURES}/bad-nav-start-ts.json`); const lateTracingStartedTrace = require(`${TRACE_FIXTURES}/tracingstarted-after-navstart.json`); const preactTrace = require(`${TRACE_FIXTURES}/preactjs.com_ts_of_undefined.json`); const noFMPtrace = require(`${TRACE_FIXTURES}/no_fmp_event.json`); -const noFCPtrace = require(`${TRACE_FIXTURES}/airhorner_no_fcp.json`); /* eslint-env jest */ @@ -98,12 +97,4 @@ describe('Metrics: FMP', () => { assert.equal(Math.round(result.timing), 4461); assert.equal(result.timestamp, 2146740268666); }); - - it('handles cases when no FCP exists', async () => { - trace = noFCPtrace; - addEmptyTask(); - const result = await artifacts.requestFirstMeaningfulPaint({trace, devtoolsLog, settings}); - assert.equal(Math.round(result.timing), 482); - assert.equal(result.timestamp, 2149509604903); - }); }); diff --git a/lighthouse-core/test/gather/computed/trace-of-tab-test.js b/lighthouse-core/test/gather/computed/trace-of-tab-test.js index f1b2f4687508..ea5e624b54ee 100644 --- a/lighthouse-core/test/gather/computed/trace-of-tab-test.js +++ b/lighthouse-core/test/gather/computed/trace-of-tab-test.js @@ -15,6 +15,7 @@ const lateTracingStartedTrace = require('../../fixtures/traces/tracingstarted-af const preactTrace = require('../../fixtures/traces/preactjs.com_ts_of_undefined.json'); const noFMPtrace = require('../../fixtures/traces/no_fmp_event.json'); const noFCPtrace = require('../../fixtures/traces/airhorner_no_fcp'); +const noNavStartTrace = require('../../fixtures/traces/no_navstart_event'); const backgroundTabTrace = require('../../fixtures/traces/backgrounded-tab-missing-paints'); /* eslint-env jest */ @@ -89,15 +90,6 @@ describe('Trace of Tab computed artifact:', () => { }); }); - it('handles traces missing an FCP', async () => { - const trace = await traceOfTab.compute_(noFCPtrace); - assert.equal(trace.startedInPageEvt.ts, 2149509117532, 'bad tracingstartedInPage'); - assert.equal(trace.navigationStartEvt.ts, 2149509122585, 'bad navStart'); - assert.equal(trace.firstContentfulPaintEvt, undefined, 'bad fcp'); - assert.equal(trace.firstMeaningfulPaintEvt.ts, 2149509604903, 'bad fmp'); - assert.ok(trace.fmpFellBack); - }); - it('handles traces missing a paints (captured in background tab)', async () => { const trace = await traceOfTab.compute_(backgroundTabTrace); assert.equal(trace.startedInPageEvt.ts, 1966813248134); @@ -108,7 +100,6 @@ describe('Trace of Tab computed artifact:', () => { 1966813258737, 'didnt select navStart event with same timestamp as usertiming measure' ); - assert.equal(trace.firstContentfulPaintEvt, undefined, 'bad fcp'); assert.equal(trace.firstMeaningfulPaintEvt, undefined, 'bad fmp'); }); @@ -147,6 +138,17 @@ describe('Trace of Tab computed artifact:', () => { }, }, 'tts': 141371, + }, { + 'pid': 69920, + 'tid': 1, + 'ts': 2193564790060, + 'ph': 'R', + 'cat': 'loading,rail,devtools.timeline', + 'name': 'firstContentfulPaint', + 'args': { + 'frame': 'B192D1F3355A6F961EC8F0B01623C1FB', + }, + 'tts': 141372, }, { 'pid': 69920, 'tid': 1, @@ -190,4 +192,16 @@ describe('Trace of Tab computed artifact:', () => { assert.deepStrictEqual(sortedEvents, tsGroup); } }); + + it('throws on traces missing a navigationStart', () => { + return traceOfTab.compute_(noNavStartTrace) + .then(_ => assert(false, 'NO_NAVSTART error not throw'), + err => assert.equal(err.message, 'NO_NAVSTART')); + }); + + it('throws on traces missing an FCP', () => { + return traceOfTab.compute_(noFCPtrace) + .then(_ => assert(false, 'NO_FCP error not throw'), + err => assert.equal(err.message, 'NO_FCP')); + }); }); diff --git a/typings/artifacts.d.ts b/typings/artifacts.d.ts index 8c79fa10a359..02c85555dc7e 100644 --- a/typings/artifacts.d.ts +++ b/typings/artifacts.d.ts @@ -368,7 +368,7 @@ declare global { export interface TraceTimes { navigationStart: number; firstPaint?: number; - firstContentfulPaint?: number; + firstContentfulPaint: number; firstMeaningfulPaint?: number; traceEnd: number; load?: number;