From 8662b2bf1a123ef73320f364272fec6162b74910 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Wed, 30 Jun 2021 14:46:39 -0400 Subject: [PATCH] performance-impls: LCP should use startTime (#35070) * performance-impls: LCP should use startTime * Update test/unit/test-performance.js Co-authored-by: Ryan Cebulko * update TICKEVENTS.md Co-authored-by: Ryan Cebulko --- .../amp-viewer-integration/TICKEVENTS.md | 3 +- src/core/constants/enums.js | 3 +- src/service/performance-impl.js | 54 +++++++------------ test/unit/test-performance.js | 30 +++++------ 4 files changed, 35 insertions(+), 55 deletions(-) diff --git a/extensions/amp-viewer-integration/TICKEVENTS.md b/extensions/amp-viewer-integration/TICKEVENTS.md index f7a6c7d76069..f2bb5aae2ada 100644 --- a/extensions/amp-viewer-integration/TICKEVENTS.md +++ b/extensions/amp-viewer-integration/TICKEVENTS.md @@ -42,8 +42,7 @@ As an example if we executed `perf.tick('label')` we assume we have a counterpar | Layout Jank, second exit | `lj-2` | The aggregate jank score when the user leaves the page (navigation, tab switching, dismissing application) for the second time. | | Cumulative Layout Shift, first exit | `cls` | The aggregate layout shift score when the user leaves the page (navigation, tab switching, dismissing application) for the first time. See https://web.dev/layout-instability-api | | Cumulative Layout Shift, second exit | `cls-2` | The aggregate layout shift score when the user leaves the page (navigation, tab switching, dismissing application) for the second time. | -| Largest Contentful Paint, load time | `lcpl` | The time in milliseconds for the first contentful element to display. This is the load time version of this metric. See https://github.com/WICG/largest-contentful-paint | -| Largest Contentful Paint, render time | `lcpr` | The time in milliseconds for the first contentful element to display. This is the render time version of this metric. https://github.com/WICG/largest-contentful-paint | +| Largest Contentful Paint | `lcp` | The time in milliseconds for the largest contentful element to display. | | Largest Contentful Paint (since visible) | `lcpv` | The time in ms for largest contentful element to display, offset by first visible time. Based on render time, falls back to load time. | | DOM Complete | `domComplete` | Time immediately before the browser sets the current document readiness of the current document to complete | | DOM Content Loaded Event End | `domContentLoadedEventEnd` | Time immediately after the current document's DOMContentLoaded event completes | diff --git a/src/core/constants/enums.js b/src/core/constants/enums.js index bd55a70f6703..eb67c3137550 100644 --- a/src/core/constants/enums.js +++ b/src/core/constants/enums.js @@ -49,9 +49,8 @@ export const TickLabel = { FIRST_VIEWPORT_READY: 'pc', GOOD_FRAME_PROBABILITY: 'gfp', INSTALL_STYLES: 'is', + LARGEST_CONTENTFUL_PAINT: 'lcp', LARGEST_CONTENTFUL_PAINT_VISIBLE: 'lcpv', - LARGEST_CONTENTFUL_PAINT_LOAD: 'lcpl', - LARGEST_CONTENTFUL_PAINT_RENDER: 'lcpr', LONG_TASKS_CHILD: 'ltc', LONG_TASKS_SELF: 'lts', MAKE_BODY_VISIBLE: 'mbv', diff --git a/src/service/performance-impl.js b/src/service/performance-impl.js index 02f2675dbaea..4d2a3f1221ea 100644 --- a/src/service/performance-impl.js +++ b/src/service/performance-impl.js @@ -163,7 +163,7 @@ export class Performance { if (!this.supportsLargestContentfulPaint_) { this.metrics_.rejectSignal( - TickLabel.LARGEST_CONTENTFUL_PAINT_VISIBLE, + TickLabel.LARGEST_CONTENTFUL_PAINT, dev().createExpectedError('Largest Contentful Paint not supported') ); } @@ -176,20 +176,15 @@ export class Performance { this.supportsNavigation_ = supportedEntryTypes.includes('navigation'); /** - * The latest reported largest contentful paint time, where the loadTime - * is specified. + * The latest reported largest contentful paint time. Uses entry.startTime, + * which equates to: renderTime ?? loadTime. We can't always use one or the other + * because: + * - loadTime is 0 for non-remote resources (text) + * - renderTime is undefined for crossorigin resources * * @private {?number} */ - this.largestContentfulPaintLoadTime_ = null; - - /** - * The latest reported largest contentful paint time, where the renderTime - * is specified. - * - * @private {?number} - */ - this.largestContentfulPaintRenderTime_ = null; + this.largestContentfulPaint_ = null; this.onAmpDocVisibilityChange_ = this.onAmpDocVisibilityChange_.bind(this); @@ -353,12 +348,7 @@ export class Performance { this.layoutShifts_.push(entry); } } else if (entry.entryType === 'largest-contentful-paint') { - if (entry.loadTime) { - this.largestContentfulPaintLoadTime_ = entry.loadTime; - } - if (entry.renderTime) { - this.largestContentfulPaintRenderTime_ = entry.renderTime; - } + this.largestContentfulPaint_ = entry.startTime; } else if (entry.entryType == 'navigation' && !recordedNavigation) { [ 'domComplete', @@ -559,24 +549,18 @@ export class Performance { * Tick the largest contentful paint metrics. */ tickLargestContentfulPaint_() { - /** @type {?number} */ let end; - if (this.largestContentfulPaintLoadTime_ !== null) { - this.tickDelta( - TickLabel.LARGEST_CONTENTFUL_PAINT_LOAD, - this.largestContentfulPaintLoadTime_ - ); - end = this.largestContentfulPaintLoadTime_; - } - if (this.largestContentfulPaintRenderTime_ !== null) { - this.tickDelta( - TickLabel.LARGEST_CONTENTFUL_PAINT_RENDER, - this.largestContentfulPaintRenderTime_ - ); - end = end || this.largestContentfulPaintRenderTime_; - } - if (end !== null) { - this.tickSinceVisible(TickLabel.LARGEST_CONTENTFUL_PAINT_VISIBLE, end); + if (this.largestContentfulPaint_ == null) { + return; } + + this.tickDelta( + TickLabel.LARGEST_CONTENTFUL_PAINT, + this.largestContentfulPaint_ + ); + this.tickSinceVisible( + TickLabel.LARGEST_CONTENTFUL_PAINT_VISIBLE, + this.largestContentfulPaint_ + ); this.flush(); } diff --git a/test/unit/test-performance.js b/test/unit/test-performance.js index 5ed75a07e9d5..fd5c66ccaf90 100644 --- a/test/unit/test-performance.js +++ b/test/unit/test-performance.js @@ -1025,29 +1025,31 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => { perf.coreServicesAvailable(); expect(perf.events_.length).to.equal(0); - // Fake a largest-contentful-paint entry specifying a loadTime, - // simulating an image on a different origin without a proper - // Timing-Allow-Origin header. + // Fake a largest-contentful-paint entry specifying a renderTime/startTime, + // simulating an image on the same origin or with a Timing-Allow-Origin header. performanceObserver.triggerCallback({ getEntries() { return [ { entryType: 'largest-contentful-paint', loadTime: 10, + renderTime: 12, + startTime: 12, }, ]; }, }); - // Fake a largest-contentful-paint entry specifying a renderTime, - // simulating an image on the same origin or with a proper - // Timing-Allow-Origin header. + // Fake a largest-contentful-paint entry with a loadTime/startTime + // simulating an image on a different origin without a Timing-Allow-Origin header. performanceObserver.triggerCallback({ getEntries() { return [ { entryType: 'largest-contentful-paint', - renderTime: 23, + loadTime: 23, + renderTime: undefined, + startTime: 23, }, ]; }, @@ -1056,16 +1058,12 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => { // The document has become hidden, e.g. via the user switching tabs. toggleVisibility(perf, false); - const lcpEvents = perf.events_.filter((evt) => - evt.label.startsWith('lcp') + const lcpEvents = perf.events_.filter(({label}) => + label.startsWith('lcp') ); - expect(lcpEvents.length).to.equal(3); - expect(perf.events_).deep.include({ - label: 'lcpl', - delta: 10, - }); - expect(perf.events_).deep.include({ - label: 'lcpr', + expect(lcpEvents.length).to.equal(2); + expect(lcpEvents).deep.include({ + label: 'lcp', delta: 23, }); });