🐛 Fixed first contentful paint race condition (ampproject#25259)

* 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
micajuine-ho · Dec 27, 2019 · d90b3c6 · d90b3c6
1 parent e01a6de
commit d90b3c6
Show file tree

Hide file tree

Showing 11 changed files with 232 additions and 28 deletions.
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<number>}
    */
   getFirstContentfulPaint() {
-    return this.firstContentfulPaint_;
+    return this.fcpDeferred_.promise;
   }
 
   /**
-   * @return {number|null}
+   * @return {!Promise<number>}
    */
   getMakeBodyVisible() {
-    return this.makeBodyVisible_;
+    return this.mbvDeferred_.promise;
   }
 
   /**
-   * @return {number|null}
+   * @return {!Promise<number>}
    */
   getFirstViewportReady() {
-    return this.firstViewportReady_;
+    return this.fvrDeferred_.promise;
   }
 }
 

diff --git 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
@@ -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';
       </script>
       <!-- put amp-analytics > 3 viewports away from viewport -->
-      <div style="height: 400vh"></div>
+      <div style="height: 400vh">
+        viewport
+      </div>
       <amp-analytics>
         <script type="application/json">
         {
@@ -50,6 +51,9 @@ describe('amp-analytics', function() {
             "cid": "\${clientId(_cid)}",
             "loadend": "\${navTiming(loadEventEnd)}",
             "default": "\$DEFAULT( , test)",
+            "fcp": "FIRST_CONTENTFUL_PAINT",
+            "fvr": "FIRST_VIEWPORT_READY",
+            "mbv": "MAKE_BODY_VISIBLE",
             "cookie": "\${cookie(test-cookie)}"
           }
         }
@@ -74,6 +78,11 @@ describe('amp-analytics', function() {
           // cookie set via http response header when requesting
           // localhost:9876/amp4test/compose-doc
           expect(q['cookie']).to.equal('test');
+
+          // FCP only resolves for Chrome and Opera
+          expect(q['fcp']).to.not.be.null;
+          expect(q['fvr']).to.not.be.null;
+          expect(q['mbv']).to.not.be.null;
           expect(
             req.headers.referer,
             'should keep referrer if no referrerpolicy specified'