From b048a5cb15257c45698f095350188394be5d7445 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 Aug 2018 16:20:40 -0700 Subject: [PATCH 1/8] core(trace-of-tab): use closest navstart --- lighthouse-core/gather/computed/trace-of-tab.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/computed/trace-of-tab.js b/lighthouse-core/gather/computed/trace-of-tab.js index a3addf08e535..25c733a2589e 100644 --- a/lighthouse-core/gather/computed/trace-of-tab.js +++ b/lighthouse-core/gather/computed/trace-of-tab.js @@ -78,8 +78,12 @@ class TraceOfTab extends ComputedArtifact { // Filter to just events matching the frame ID for sanity const frameEvents = keyEvents.filter(e => e.args.frame === frameId); - // Our navStart will be the last frame navigation in the trace - const navigationStart = frameEvents.filter(e => e.name === 'navigationStart').pop(); + // Our navStart will be the navigation start closest to the first outbound request + const requestSentEvt = keyEvents.find(e => e.name === 'ResourceSendRequest'); + const navigationStartCandidates = frameEvents.filter(e => e.name === 'navigationStart'); + const navigationStart = navigationStartCandidates + .sort((a, b) => Math.abs(a.ts - requestSentEvt.ts) - Math.abs(b.ts - requestSentEvt.ts)) + .shift(); if (!navigationStart) throw new LHError(LHError.errors.NO_NAVSTART); // Find our first paint of this frame From 296977905643f95aef592d1a47d54799446b1551 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 Aug 2018 16:43:13 -0700 Subject: [PATCH 2/8] add warning --- .../audits/metrics/first-contentful-paint.js | 8 ++++++++ lighthouse-core/gather/computed/trace-of-tab.js | 15 ++++++++------- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/audits/metrics/first-contentful-paint.js b/lighthouse-core/audits/metrics/first-contentful-paint.js index c299f43925f9..e344e0c50728 100644 --- a/lighthouse-core/audits/metrics/first-contentful-paint.js +++ b/lighthouse-core/audits/metrics/first-contentful-paint.js @@ -14,6 +14,9 @@ const UIStrings = { /** Description of the First Contentful Paint (FCP) metric, which marks the time at which the first text or image is painted by the browser. This is displayed within a tooltip when the user hovers on the metric name to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ description: 'First Contentful Paint marks the time at which the first text or image is ' + `painted. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/first-contentful-paint).`, + /** Warning message shown to the user when they audited a URL that had multiple navigations that could throw off metrics. */ + multipleNavigationWarning: 'The page you audited was redirected. This can skew ' + + 'performance metrics. Try auditing the final URL directly instead.', }; const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); @@ -53,9 +56,14 @@ class FirstContentfulPaint extends Audit { static async audit(artifacts, context) { const trace = artifacts.traces[Audit.DEFAULT_PASS]; const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS]; + const traceOfTab = await artifacts.requestTraceOfTab(trace); const metricComputationData = {trace, devtoolsLog, settings: context.settings}; const metricResult = await artifacts.requestFirstContentfulPaint(metricComputationData); + if (traceOfTab.hadMultipleNavigations) { + context.LighthouseRunWarnings.push(str_(UIStrings.multipleNavigationWarning)); + } + return { score: Audit.computeLogNormalScore( metricResult.timing, diff --git a/lighthouse-core/gather/computed/trace-of-tab.js b/lighthouse-core/gather/computed/trace-of-tab.js index 25c733a2589e..2aed45be57f5 100644 --- a/lighthouse-core/gather/computed/trace-of-tab.js +++ b/lighthouse-core/gather/computed/trace-of-tab.js @@ -80,19 +80,19 @@ class TraceOfTab extends ComputedArtifact { // Our navStart will be the navigation start closest to the first outbound request const requestSentEvt = keyEvents.find(e => e.name === 'ResourceSendRequest'); - const navigationStartCandidates = frameEvents.filter(e => e.name === 'navigationStart'); - const navigationStart = navigationStartCandidates - .sort((a, b) => Math.abs(a.ts - requestSentEvt.ts) - Math.abs(b.ts - requestSentEvt.ts)) - .shift(); + const navigationStart = frameEvents. + filter(e => e.name === 'navigationStart'). + sort((a, b) => Math.abs(a.ts - requestSentEvt.ts) - Math.abs(b.ts - requestSentEvt.ts)). + shift(); if (!navigationStart) throw new LHError(LHError.errors.NO_NAVSTART); // 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. Used in so many places we require it. - const firstContentfulPaint = frameEvents.find( - e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts - ); + const firstContentfulPaints = frameEvents. + filter(e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts); + const firstContentfulPaint = firstContentfulPaints[0]; if (!firstContentfulPaint) throw new LHError(LHError.errors.NO_FCP); // fMP will follow at/after the FP @@ -181,6 +181,7 @@ class TraceOfTab extends ComputedArtifact { loadEvt: load, domContentLoadedEvt: domContentLoaded, fmpFellBack, + hadMultipleNavigations: firstContentfulPaints.length > 1, }; } } From d4c2f7f6449951ac401058a15fd8a08aedbb8c0c Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 Aug 2018 17:14:14 -0700 Subject: [PATCH 3/8] revert --- .../audits/metrics/first-contentful-paint.js | 8 -------- lighthouse-core/gather/computed/trace-of-tab.js | 15 +++++---------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/lighthouse-core/audits/metrics/first-contentful-paint.js b/lighthouse-core/audits/metrics/first-contentful-paint.js index e344e0c50728..c299f43925f9 100644 --- a/lighthouse-core/audits/metrics/first-contentful-paint.js +++ b/lighthouse-core/audits/metrics/first-contentful-paint.js @@ -14,9 +14,6 @@ const UIStrings = { /** Description of the First Contentful Paint (FCP) metric, which marks the time at which the first text or image is painted by the browser. This is displayed within a tooltip when the user hovers on the metric name to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ description: 'First Contentful Paint marks the time at which the first text or image is ' + `painted. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/first-contentful-paint).`, - /** Warning message shown to the user when they audited a URL that had multiple navigations that could throw off metrics. */ - multipleNavigationWarning: 'The page you audited was redirected. This can skew ' + - 'performance metrics. Try auditing the final URL directly instead.', }; const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); @@ -56,14 +53,9 @@ class FirstContentfulPaint extends Audit { static async audit(artifacts, context) { const trace = artifacts.traces[Audit.DEFAULT_PASS]; const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS]; - const traceOfTab = await artifacts.requestTraceOfTab(trace); const metricComputationData = {trace, devtoolsLog, settings: context.settings}; const metricResult = await artifacts.requestFirstContentfulPaint(metricComputationData); - if (traceOfTab.hadMultipleNavigations) { - context.LighthouseRunWarnings.push(str_(UIStrings.multipleNavigationWarning)); - } - return { score: Audit.computeLogNormalScore( metricResult.timing, diff --git a/lighthouse-core/gather/computed/trace-of-tab.js b/lighthouse-core/gather/computed/trace-of-tab.js index 2aed45be57f5..a3addf08e535 100644 --- a/lighthouse-core/gather/computed/trace-of-tab.js +++ b/lighthouse-core/gather/computed/trace-of-tab.js @@ -78,21 +78,17 @@ class TraceOfTab extends ComputedArtifact { // Filter to just events matching the frame ID for sanity const frameEvents = keyEvents.filter(e => e.args.frame === frameId); - // Our navStart will be the navigation start closest to the first outbound request - const requestSentEvt = keyEvents.find(e => e.name === 'ResourceSendRequest'); - const navigationStart = frameEvents. - filter(e => e.name === 'navigationStart'). - sort((a, b) => Math.abs(a.ts - requestSentEvt.ts) - Math.abs(b.ts - requestSentEvt.ts)). - shift(); + // Our navStart will be the last frame navigation in the trace + const navigationStart = frameEvents.filter(e => e.name === 'navigationStart').pop(); if (!navigationStart) throw new LHError(LHError.errors.NO_NAVSTART); // 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. Used in so many places we require it. - const firstContentfulPaints = frameEvents. - filter(e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts); - const firstContentfulPaint = firstContentfulPaints[0]; + 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 @@ -181,7 +177,6 @@ class TraceOfTab extends ComputedArtifact { loadEvt: load, domContentLoadedEvt: domContentLoaded, fmpFellBack, - hadMultipleNavigations: firstContentfulPaints.length > 1, }; } } From 8dcd88f193efc071e961a0487cf01878d9c57183 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 Aug 2018 17:25:11 -0700 Subject: [PATCH 4/8] other fix --- lighthouse-core/gather/computed/trace-of-tab.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/gather/computed/trace-of-tab.js b/lighthouse-core/gather/computed/trace-of-tab.js index a3addf08e535..f838b20f9edc 100644 --- a/lighthouse-core/gather/computed/trace-of-tab.js +++ b/lighthouse-core/gather/computed/trace-of-tab.js @@ -22,11 +22,19 @@ const TracingProcessor = require('../../lib/traces/tracing-processor'); const LHError = require('../../lib/errors'); const Sentry = require('../../lib/sentry'); +const ACCEPTABLE_NAVIGATION_URL_REGEX = /^(chrome|https?):/; + class TraceOfTab extends ComputedArtifact { get name() { return 'TraceOfTab'; } + static isNavigationStartOfInterest(event) { + return event.name === 'navigationStart' && + (!event.args.data || !event.args.documentLoaderURL || + ACCEPTABLE_NAVIGATION_URL_REGEX.test(event.args.data.documentLoaderURL)); + } + /** * @param {LH.TraceEvent[]} traceEvents * @param {(e: LH.TraceEvent) => boolean} filter @@ -79,7 +87,7 @@ class TraceOfTab extends ComputedArtifact { const frameEvents = keyEvents.filter(e => e.args.frame === frameId); // Our navStart will be the last frame navigation in the trace - const navigationStart = frameEvents.filter(e => e.name === 'navigationStart').pop(); + const navigationStart = frameEvents.filter(TraceOfTab.isNavigationStartOfInterest).pop(); if (!navigationStart) throw new LHError(LHError.errors.NO_NAVSTART); // Find our first paint of this frame From 581fa8da7f2a278280ab2a0d80e92ed0359fcefc Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 Aug 2018 17:26:03 -0700 Subject: [PATCH 5/8] comment --- lighthouse-core/gather/computed/trace-of-tab.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lighthouse-core/gather/computed/trace-of-tab.js b/lighthouse-core/gather/computed/trace-of-tab.js index f838b20f9edc..7acc278da0df 100644 --- a/lighthouse-core/gather/computed/trace-of-tab.js +++ b/lighthouse-core/gather/computed/trace-of-tab.js @@ -29,6 +29,11 @@ class TraceOfTab extends ComputedArtifact { return 'TraceOfTab'; } + /** + * Returns true if the event is a navigation start event of a document whose URL seems valid. + * + * @param {LH.TraceEvent} event + */ static isNavigationStartOfInterest(event) { return event.name === 'navigationStart' && (!event.args.data || !event.args.documentLoaderURL || From ae1e1e88d0969990a1a5e87cbf048fd1debcbc85 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 Aug 2018 17:29:00 -0700 Subject: [PATCH 6/8] add test --- .../traces/preactjs.com_ts_of_undefined.json | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lighthouse-core/test/fixtures/traces/preactjs.com_ts_of_undefined.json b/lighthouse-core/test/fixtures/traces/preactjs.com_ts_of_undefined.json index 2241e1387e23..7146cff62a7f 100644 --- a/lighthouse-core/test/fixtures/traces/preactjs.com_ts_of_undefined.json +++ b/lighthouse-core/test/fixtures/traces/preactjs.com_ts_of_undefined.json @@ -71,6 +71,19 @@ "frame": "0x25edaa521e58" }, "tts": 826016 + }, + { + "pid": 6117, + "tid": 775, + "ts": 1805897263653, + "ph": "R", + "cat": "blink.user_timing", + "name": "navigationStart", + "args": { + "frame": "0x25edaa521e58", + "data": {"documentLoaderURL": "intent://this-one-should-be-ignored"} + }, + "tts": 673412 }, { "pid": 6117, From 94094438a0769ed55f7aa0609cba7789b6e28b28 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 Aug 2018 17:35:47 -0700 Subject: [PATCH 7/8] oops :) --- lighthouse-core/gather/computed/trace-of-tab.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/computed/trace-of-tab.js b/lighthouse-core/gather/computed/trace-of-tab.js index 7acc278da0df..ecc1afc9110f 100644 --- a/lighthouse-core/gather/computed/trace-of-tab.js +++ b/lighthouse-core/gather/computed/trace-of-tab.js @@ -36,7 +36,7 @@ class TraceOfTab extends ComputedArtifact { */ static isNavigationStartOfInterest(event) { return event.name === 'navigationStart' && - (!event.args.data || !event.args.documentLoaderURL || + (!event.args.data || !event.args.data.documentLoaderURL || ACCEPTABLE_NAVIGATION_URL_REGEX.test(event.args.data.documentLoaderURL)); } From 7688bfc103baa121a48199e46408df337317e243 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 28 Aug 2018 15:50:28 -0700 Subject: [PATCH 8/8] types --- typings/externs.d.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/typings/externs.d.ts b/typings/externs.d.ts index f68e5c61cca1..bae9a77db794 100644 --- a/typings/externs.d.ts +++ b/typings/externs.d.ts @@ -166,6 +166,7 @@ declare global { fileName?: string; snapshot?: string; data?: { + documentLoaderURL?: string; frames?: { frame: string; parent?: string;