From d90b3c6410dff5d483942bd6ce7eec9bedcd1903 Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Tue, 17 Dec 2019 14:47:07 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20first=20contentful=20pai?= =?UTF-8?q?nt=20race=20condition=20=20(#25259)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * edge case handling and testing * type fixing * cleaning up * changing to only serve chrome * install platform service for fcp * Clean up integration test * viewport * remove platform test --- .../0.1/test/test-amp-access-source.js | 4 + .../amp-access/0.1/test/test-amp-access.js | 9 + src/amp.js | 2 +- src/inabox/amp-inabox.js | 2 + src/service/performance-impl.js | 38 ++-- src/service/url-replacements-impl.js | 13 +- test/integration/test-amp-analytics.js | 13 +- test/manual/amp-analytics/analytics-fcp.html | 172 ++++++++++++++++++ test/unit/test-layout-delay-meter.js | 2 + test/unit/test-performance.js | 3 + test/unit/test-style-installer.js | 2 + 11 files changed, 232 insertions(+), 28 deletions(-) create mode 100644 test/manual/amp-analytics/analytics-fcp.html diff --git a/extensions/amp-access/0.1/test/test-amp-access-source.js b/extensions/amp-access/0.1/test/test-amp-access-source.js index af3e443524b0..4b5006b1eb61 100644 --- a/extensions/amp-access/0.1/test/test-amp-access-source.js +++ b/extensions/amp-access/0.1/test/test-amp-access-source.js @@ -23,6 +23,7 @@ import {AccessSource} from '../amp-access-source'; import {AccessVendorAdapter} from '../amp-access-vendor'; import {cidServiceForDocForTesting} from '../../../../src/service/cid-impl'; import {installPerformanceService} from '../../../../src/service/performance-impl'; +import {installPlatformService} from '../../../../src/service/platform-impl'; import {toggleExperiment} from '../../../../src/experiments'; describes.fakeWin( @@ -45,6 +46,7 @@ describes.fakeWin( document = win.document; cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); element = document.createElement('script'); @@ -266,6 +268,7 @@ describes.fakeWin( clock.tick(0); cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); const config = { @@ -385,6 +388,7 @@ describes.fakeWin( clock.tick(0); cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); const config = { diff --git a/extensions/amp-access/0.1/test/test-amp-access.js b/extensions/amp-access/0.1/test/test-amp-access.js index 55189a699359..29ea90831f1b 100644 --- a/extensions/amp-access/0.1/test/test-amp-access.js +++ b/extensions/amp-access/0.1/test/test-amp-access.js @@ -21,6 +21,7 @@ import {Observable} from '../../../../src/observable'; import {Services} from '../../../../src/services'; import {cidServiceForDocForTesting} from '../../../../src/service/cid-impl'; import {installPerformanceService} from '../../../../src/service/performance-impl'; +import {installPlatformService} from '../../../../src/service/platform-impl'; import {toggleExperiment} from '../../../../src/experiments'; describes.fakeWin( @@ -40,6 +41,7 @@ describes.fakeWin( document = win.document; cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); element = document.createElement('script'); @@ -275,6 +277,7 @@ describes.fakeWin( clock.tick(0); cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); configElement = document.createElement('script'); @@ -564,6 +567,7 @@ describes.fakeWin( document = win.document; cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); configElement = document.createElement('script'); @@ -724,6 +728,7 @@ describes.fakeWin( clock = env.sandbox.useFakeTimers(); cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); configElement = document.createElement('script'); @@ -1124,6 +1129,7 @@ describes.fakeWin( document = win.document; cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); configElement = document.createElement('script'); @@ -1208,6 +1214,7 @@ describes.fakeWin( clock = env.sandbox.useFakeTimers(); cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); configElement = document.createElement('script'); @@ -1620,6 +1627,7 @@ describes.fakeWin( document = win.document; cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); configElement = document.createElement('script'); @@ -1749,6 +1757,7 @@ describes.fakeWin( clock.tick(0); cidServiceForDocForTesting(ampdoc); + installPlatformService(win); installPerformanceService(win); configElement = document.createElement('script'); diff --git a/src/amp.js b/src/amp.js index 6780bc6527bb..d312148cac36 100644 --- a/src/amp.js +++ b/src/amp.js @@ -91,6 +91,7 @@ if (shouldMainBootstrapRun) { startupChunk(self.document, function initial() { /** @const {!./service/ampdoc-impl.AmpDoc} */ const ampdoc = ampdocService.getAmpDoc(self.document); + installPlatformService(self); installPerformanceService(self); /** @const {!./service/performance-impl.Performance} */ const perf = Services.performanceFor(self); @@ -99,7 +100,6 @@ if (shouldMainBootstrapRun) { ) { perf.addEnabledExperiment('no-boilerplate'); } - installPlatformService(self); fontStylesheetTimeout(self); perf.tick('is'); installStylesForDoc( diff --git a/src/inabox/amp-inabox.js b/src/inabox/amp-inabox.js index d3ea57d39c5d..173d7bba643f 100644 --- a/src/inabox/amp-inabox.js +++ b/src/inabox/amp-inabox.js @@ -35,6 +35,7 @@ import { import {installDocService} from '../service/ampdoc-impl'; import {installErrorReporting} from '../error'; import {installPerformanceService} from '../service/performance-impl'; +import {installPlatformService} from '../service/platform-impl'; import { installStylesForDoc, makeBodyVisible, @@ -71,6 +72,7 @@ try { startupChunk(self.document, function initial() { /** @const {!../service/ampdoc-impl.AmpDoc} */ const ampdoc = ampdocService.getAmpDoc(self.document); + installPlatformService(self); installPerformanceService(self); /** @const {!../service/performance-impl.Performance} */ const perf = Services.performanceFor(self); diff --git a/src/service/performance-impl.js b/src/service/performance-impl.js index ac03d37ae2b4..3049ceb6395a 100644 --- a/src/service/performance-impl.js +++ b/src/service/performance-impl.js @@ -14,6 +14,7 @@ * limitations under the License. */ +import {Deferred} from '../utils/promise'; import {Services} from '../services'; import {VisibilityState} from '../visibility-state'; import {dev} from '../log'; @@ -104,12 +105,19 @@ export class Performance { /** @private {string} */ this.ampexp_ = ''; - /** @private {number|null} */ - this.makeBodyVisible_ = null; - /** @private {number|null} */ - this.firstContentfulPaint_ = null; - /** @private {number|null} */ - this.firstViewportReady_ = null; + this.fcpDeferred_ = new Deferred(); + this.fvrDeferred_ = new Deferred(); + this.mbvDeferred_ = new Deferred(); + + // Platform service must be installed before performance serivce is + this.platform_ = Services.platformFor(this.win); + + // TODO (micajuineho) change this once all platforms + // support PerformancePaintTiming + // https://developer.mozilla.org/en-US/docs/Web/API/PerformancePaintTiming + if (!this.platform_.isChrome() && !this.platform_.isOpera()) { + this.fcpDeferred_.resolve(null); + } /** * How many times a layout jank metric has been ticked. @@ -684,13 +692,13 @@ export class Performance { ); switch (label) { case 'fcp': - this.firstContentfulPaint_ = storedVal; + this.fcpDeferred_.resolve(storedVal); break; case 'pc': - this.firstViewportReady_ = storedVal; + this.fvrDeferred_.resolve(storedVal); break; case 'mbv': - this.makeBodyVisible_ = storedVal; + this.mbvDeferred_.resolve(storedVal); break; } } @@ -828,24 +836,24 @@ export class Performance { } /** - * @return {number|null} + * @return {!Promise} */ getFirstContentfulPaint() { - return this.firstContentfulPaint_; + return this.fcpDeferred_.promise; } /** - * @return {number|null} + * @return {!Promise} */ getMakeBodyVisible() { - return this.makeBodyVisible_; + return this.mbvDeferred_.promise; } /** - * @return {number|null} + * @return {!Promise} */ getFirstViewportReady() { - return this.firstViewportReady_; + return this.fvrDeferred_.promise; } } diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index ff23ac41a45d..f77cc28e7eb0 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -45,7 +45,6 @@ import {WindowInterface} from '../window-interface'; import {getTrackImpressionPromise} from '../impression.js'; import {hasOwn} from '../utils/object'; import {internalRuntimeVersion} from '../internal-version'; -import {tryResolve} from '../utils/promise'; /** @private @const {string} */ const TAG = 'UrlReplacements'; @@ -654,21 +653,15 @@ export class GlobalVariableSource extends VariableSource { ); this.setAsync('FIRST_CONTENTFUL_PAINT', () => { - return tryResolve(() => - Services.performanceFor(win).getFirstContentfulPaint() - ); + return Services.performanceFor(win).getFirstContentfulPaint(); }); this.setAsync('FIRST_VIEWPORT_READY', () => { - return tryResolve(() => - Services.performanceFor(win).getFirstViewportReady() - ); + return Services.performanceFor(win).getFirstViewportReady(); }); this.setAsync('MAKE_BODY_VISIBLE', () => { - return tryResolve(() => - Services.performanceFor(win).getMakeBodyVisible() - ); + return Services.performanceFor(win).getMakeBodyVisible(); }); this.setAsync('AMP_STATE', key => { diff --git a/test/integration/test-amp-analytics.js b/test/integration/test-amp-analytics.js index 39b6b5f08ea8..8e63fedcc7e5 100644 --- a/test/integration/test-amp-analytics.js +++ b/test/integration/test-amp-analytics.js @@ -15,7 +15,6 @@ */ import {BrowserController, RequestBank} from '../../testing/test-helper'; - import {parseQueryString} from '../../src/url'; describe('amp-analytics', function() { @@ -28,7 +27,9 @@ describe('amp-analytics', function() { document.cookie='_cid=amp-12345'; -
+
+ viewport +
+ + + + + +

Scroll down to see content

+
+Container for analytics tags. Positioned far away from top to make sure that doesn't matter. + + + + + + +

AMP Analytics

+ + Click here to generate an event + +

+Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam pellentesque augue quis elementum tempus. Pellentesque sit amet neque bibendum, sagittis purus vitae, pellentesque magna. Vestibulum non viverra metus, eget feugiat lacus. Nulla in maximus orci. Maecenas id turpis vel ipsum vestibulum bibendum ut sit amet magna. Nullam hendrerit ex at est eleifend, nec dignissim nibh rutrum. Aliquam quis tellus et nibh faucibus laoreet in eget turpis. Nam quam nisl, porttitor vel ex eget, dapibus placerat dui. Mauris commodo pellentesque leo, eu tempus quam. In hac habitasse platea dictumst. Suspendisse non ante finibus, luctus augue non, luctus orci. Vestibulum ornare lacinia aliquam. In sollicitudin vehicula vulputate. Sed mi elit, commodo nec sapien nec, pretium bibendum leo. Donec id justo tortor. Ut in mauris dapibus, laoreet metus vitae, dictum nisi. +

+

+Integer dapibus egestas arcu. Nunc vitae velit congue, placerat augue quis, suscipit nisi. Donec suscipit imperdiet turpis pharetra feugiat. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Phasellus aliquam eleifend dolor, at lacinia orci semper vel. Nunc semper sem vel tincidunt posuere. Nunc lobortis velit vitae condimentum mollis. Morbi eu ullamcorper mauris. Pellentesque ac eros maximus, pulvinar sapien vitae, semper nisi. Curabitur imperdiet non mauris vitae sollicitudin. +

+

+Nam posuere velit euismod risus pulvinar, in sollicitudin sapien consectetur. Vestibulum nec ex odio. Quisque at elit nec nunc ultricies lacinia nec non lorem. Maecenas porttitor consequat mauris, vitae porttitor ligula pellentesque ut. Pellentesque rhoncus diam vel lacus lobortis imperdiet. Sed maximus dictum hendrerit. Vivamus ornare, purus in laoreet sagittis, est ante pretium mauris, vel vulputate arcu erat eget mauris. Suspendisse eu lorem metus. Aliquam tempus aliquet urna, vitae mollis lacus pretium vitae. Etiam semper gravida commodo. Maecenas at pulvinar quam. Nullam dolor ipsum, ornare a sollicitudin et, sodales porttitor neque. +

+

+Integer in felis at lacus mattis facilisis. Curabitur tincidunt, felis porttitor mollis finibus, tortor elit elementum dolor, vel vulputate lorem dui id ante. Vivamus in velit at lectus blandit gravida vitae quis arcu. Nam et magna magna. Fusce condimentum diam lacus, ac ullamcorper purus malesuada eu. Mauris ullamcorper elit et venenatis faucibus. Nullam lobortis molestie purus quis pellentesque. Sed at libero id nisi rhoncus tincidunt. Praesent vestibulum vehicula tristique. Etiam rutrum, nunc id porta interdum, nulla nisi molestie leo, at fermentum justo dolor at lorem. Duis in egestas sapien. +

+ +
+ This is a placeholder +
+
+ This is a fallback +
+ + +

+Donec pharetra molestie sollicitudin. Duis mattis eleifend rutrum. Quisque luctus tincidunt lacus, vitae lobortis nisi malesuada ac. Aliquam mattis leo vel elit rutrum, nec consequat massa vestibulum. Maecenas bibendum metus nec ante feugiat, eu faucibus orci mattis. Cras tristique sem non elit congue malesuada. Proin ornare, lacus et porttitor consequat, sapien urna rutrum diam, ac pellentesque ligula est eget nisi. Interdum et malesuada fames ac ante ipsum primis in faucibus. Donec ultrices sollicitudin eros a placerat. Proin eget pulvinar est. Donec posuere ultrices odio at ultrices. Suspendisse potenti. Phasellus id orci id purus porttitor consectetur a at erat. Nullam volutpat ultricies nisl id maximus. Morbi porta ex ante, et egestas odio ultricies consequat. +

+

+

    +
  • Lorem ipsum dolor sit amet, consectetur adipiscing elit.
    • +
  • Aliquam in ex porta, imperdiet elit sit amet, condimentum diam.
    • +
  • Etiam fermentum nisi at porta pulvinar.
    • +
+

+

+

    +
  • Proin mattis neque vel elit posuere molestie.
    • +
  • Integer tincidunt sem sed nunc auctor elementum.
    • +
  • Integer a felis in ipsum aliquet auctor sit amet a neque.
    • +
+

+

+

    +
  • Sed suscipit dolor molestie, rhoncus quam ac, lacinia ex.
    • +
  • Curabitur et tellus vel justo ultrices aliquet sed id turpis.
    • +
  • Nam finibus risus at justo elementum bibendum.
    • +
  • In non lacus non urna congue feugiat at vel diam.
    • +
+

+

+

    +
  • Integer hendrerit augue interdum dui venenatis, sit amet tristique mauris cursus.
    • +
  • Etiam quis eros viverra, tincidunt justo in, facilisis nunc.
    • +
  • Aliquam at lacus faucibus, congue lorem interdum, semper mauris.
    • +
  • Ut vulputate erat vel feugiat pharetra.
    • +
  • Morbi id augue id orci sagittis tempus.
    • +
  • Vestibulum varius libero ac dignissim sodales.
    • +
+

+

+

    +
  • Aenean ac sem eget libero varius viverra sit amet vitae nunc.
    • +
+

+ + + + + + +
+ + \ No newline at end of file diff --git a/test/unit/test-layout-delay-meter.js b/test/unit/test-layout-delay-meter.js index 6823adf9b60a..aa325b149e88 100644 --- a/test/unit/test-layout-delay-meter.js +++ b/test/unit/test-layout-delay-meter.js @@ -18,6 +18,7 @@ import * as lolex from 'lolex'; import {LayoutDelayMeter} from '../../src/layout-delay-meter'; import {Services} from '../../src/services'; import {installPerformanceService} from '../../src/service/performance-impl'; +import {installPlatformService} from '../../src/service/platform-impl'; describes.realWin( 'layout-delay-meter', @@ -34,6 +35,7 @@ describes.realWin( beforeEach(() => { win = env.win; + installPlatformService(win); installPerformanceService(win); const perf = Services.performanceFor(win); env.sandbox.stub(perf, 'isPerformanceTrackingOn').callsFake(() => true); diff --git a/test/unit/test-performance.js b/test/unit/test-performance.js index 65bd822ff730..48fb101f0ffb 100644 --- a/test/unit/test-performance.js +++ b/test/unit/test-performance.js @@ -20,6 +20,7 @@ import {Services} from '../../src/services'; import {VisibilityState} from '../../src/visibility-state'; import {getMode} from '../../src/mode'; import {installPerformanceService} from '../../src/service/performance-impl'; +import {installPlatformService} from '../../src/service/platform-impl'; import {installRuntimeServices} from '../../src/service/core-services'; describes.realWin('performance', {amp: true}, env => { @@ -35,6 +36,7 @@ describes.realWin('performance', {amp: true}, env => { target: win, toFake: ['Date', 'setTimeout', 'clearTimeout'], }); + installPlatformService(env.win); installPerformanceService(env.win); perf = Services.performanceFor(env.win); }); @@ -748,6 +750,7 @@ describes.realWin('performance with experiment', {amp: true}, env => { .withArgs('csi') .returns('1'); env.sandbox.stub(viewer, 'isEmbedded').returns(true); + installPlatformService(win); installPerformanceService(win); perf = Services.performanceFor(win); }); diff --git a/test/unit/test-style-installer.js b/test/unit/test-style-installer.js index c566793adabb..dd73397762ad 100644 --- a/test/unit/test-style-installer.js +++ b/test/unit/test-style-installer.js @@ -21,6 +21,7 @@ import {Services} from '../../src/services'; import {createShadowRoot} from '../../src/shadow-embed'; import {getStyle} from '../../src/style'; import {installPerformanceService} from '../../src/service/performance-impl'; +import {installPlatformService} from '../../src/service/platform-impl'; import {isAnimationNone} from '../../testing/test-helper'; import {setShadowDomSupportedVersionForTesting} from '../../src/web-components'; @@ -36,6 +37,7 @@ describe('Styles', () => { win = env.win; doc = win.document; ampdoc = env.ampdoc; + installPlatformService(win); installPerformanceService(win); const perf = Services.performanceFor(win); tickSpy = env.sandbox.spy(perf, 'tick');