Re-Fix build being called before element is ready to build. (ampproje…

…ct#4476)

extensions/amp-analytics/0.1/test/test-visibility-impl.js

@@ -58,6 +58,7 @@ describe('amp-analytics.visibility', () => {
     visibility = new Visibility(window);
     sandbox.stub(visibility.resourcesService_, 'getResourceForElement')
         .returns({
+          getLayoutBox: () => {},
           element: {getIntersectionChangeEntry: getIntersectionStub},
           getId: getIdStub,
           isLayoutPending: () => false});

src/service/resource.js

@@ -147,13 +147,6 @@ export class Resource {
     /** @private {?Promise<undefined>} */
     this.layoutPromise_ = null;
 
-    /**
-     * Only used in the "runtime off" case when the monitoring code needs to
-     * known when the element is upgraded.
-     * @private {!Function|undefined}
-     */
-    this.onUpgraded_ = undefined;
-
    /**
     * Pending change size that was requested but could not be satisfied.
     * @private {!./resources-impl.SizeDef|undefined}
@@ -673,45 +666,4 @@ export class Resource {
     this.pause();
     this.unlayout();
   }
-
-  /**
-   * Only allowed in dev mode when runtime is turned off. Performs all steps
-   * necessary to render an element.
-   * @return {!Promise}
-   * @export
-   */
-  forceAll() {
-    dev().assert(!this.resources_.isRuntimeOn());
-    let p = Promise.resolve();
-    if (this.state_ == ResourceState.NOT_BUILT) {
-      if (!this.element.isUpgraded()) {
-        p = p.then(() => {
-          return new Promise(resolve => {
-            this.onUpgraded_ = resolve;
-          });
-        });
-      }
-      p = p.then(() => {
-        this.onUpgraded_ = undefined;
-        this.build(true);
-      });
-    }
-    return p.then(() => {
-      this.applySizesAndMediaQuery();
-      this.measure();
-      if (this.layoutPromise_) {
-        return this.layoutPromise_;
-      }
-      if (this.state_ == ResourceState.LAYOUT_COMPLETE ||
-              this.state_ == ResourceState.LAYOUT_FAILED ||
-              this.layoutCount_ > 0) {
-        return;
-      }
-      if (!this.isDisplayed()) {
-        return;
-      }
-      this.layoutCount_++;
-      return this.element.layoutCallback();
-    });
-  }
 }

src/service/resources-impl.js

@@ -336,20 +336,31 @@ export class Resources {
       element.id = 'AMP_' + resource.getId();
     }
     this.resources_.push(resource);
+    this.buildOrScheduleBuildForResource_(resource);
+    dev().fine(TAG_, 'element added:', resource.debugid);
+  }
 
+  /**
+   * Builds the element if ready to be built, otherwise adds it to pending resources.
+   * @param {!Resource} resource
+   * @param {boolean=} checkForDupes
+   * @private
+   */
+  buildOrScheduleBuildForResource_(resource, checkForDupes = false) {
     if (this.isRuntimeOn_) {
       if (this.documentReady_) {
         // Build resource immediately, the document has already been parsed.
         resource.build();
         this.schedulePass();
-      } else if (!element.isBuilt()) {
-        // Otherwise add to pending resources and try to build any ready ones.
-        this.pendingBuildResources_.push(resource);
-        this.buildReadyResources_();
+      } else if (!resource.element.isBuilt()) {
+        if (!checkForDupes ||
+            this.pendingBuildResources_.indexOf(resource) == -1) {
+          // Otherwise add to pending resources and try to build any ready ones.
+          this.pendingBuildResources_.push(resource);
+          this.buildReadyResources_();
+        }
       }
     }
-
-    dev().fine(TAG_, 'element added:', resource.debugid);
   }
 
   /**
@@ -372,6 +383,7 @@ export class Resources {
 
   /** @private */
   buildReadyResourcesUnsafe_() {
+    let builtElementsCount = 0;
     // This will loop over all current pending resources and those that
     // get added by other resources build-cycle, this will make sure all
     // elements get a chance to be built.
@@ -382,9 +394,14 @@ export class Resources {
         // Remove resource before build to remove it from the pending list
         // in either case the build succeed or throws an error.
         this.pendingBuildResources_.splice(i--, 1);
+        builtElementsCount++;
         resource.build();
       }
     }
+
+    if (builtElementsCount > 0) {
+      this.schedulePass();
+    }
   }
 
   /**
@@ -415,12 +432,7 @@ export class Resources {
    */
   upgraded(element) {
     const resource = Resource.forElement(element);
-    if (this.isRuntimeOn_) {
-      resource.build();
-      this.schedulePass();
-    } else if (resource.onUpgraded_) {
-      resource.onUpgraded_();
-    }
+    this.buildOrScheduleBuildForResource_(resource);
     dev().fine(TAG_, 'element upgraded:', resource.debugid);
   }
 
@@ -932,7 +944,7 @@ export class Resources {
     for (let i = 0; i < this.resources_.length; i++) {
       const r = this.resources_[i];
       if (r.getState() == ResourceState.NOT_BUILT) {
-        r.build();
+        this.buildOrScheduleBuildForResource_(r, /* checkForDupes */ true);
       }
       if (relayoutAll || r.getState() == ResourceState.NOT_LAID_OUT) {
         r.applySizesAndMediaQuery();
@@ -1001,7 +1013,7 @@ export class Resources {
     // Phase 3: Trigger "viewport enter/exit" events.
     for (let i = 0; i < this.resources_.length; i++) {
       const r = this.resources_[i];
-      if (r.hasOwner()) {
+      if (r.getState() == ResourceState.NOT_BUILT || r.hasOwner()) {
         continue;
       }
       // Note that when the document is not visible, neither are any of its

test/functional/test-resources.js

@@ -1423,6 +1423,8 @@ describe('Resources.add', () => {
   });
 
   it('should build elements immediately if the document is ready', () => {
+    child1.isBuilt = () => false;
+    child2.isBuilt = () => false;
     resources.documentReady_ = false;
     resources.add(child1);
     expect(child1.build.called).to.be.false;
@@ -1447,26 +1449,30 @@ describe('Resources.add', () => {
 
   describe('buildReadyResources_', () => {
     it('should build ready resources and remove them from pending', () => {
+      sandbox.stub(resources, 'schedulePass');
       resources.documentReady_ = false;
       resources.pendingBuildResources_ = [resource1, resource2];
       resources.buildReadyResources_();
       expect(child1.build.called).to.be.false;
       expect(child2.build.called).to.be.false;
       expect(resources.pendingBuildResources_.length).to.be.equal(2);
+      expect(resources.schedulePass.called).to.be.false;
 
       child1.nextSibling = child2;
       resources.buildReadyResources_();
       expect(child1.build.called).to.be.true;
       expect(child2.build.called).to.be.false;
       expect(resources.pendingBuildResources_.length).to.be.equal(1);
       expect(resources.pendingBuildResources_[0]).to.be.equal(resource2);
+      expect(resources.schedulePass.calledOnce).to.be.true;
 
       child2.parentNode = parent;
       parent.nextSibling = true;
       resources.buildReadyResources_();
       expect(child1.build.calledTwice).to.be.false;
       expect(child2.build.called).to.be.true;
       expect(resources.pendingBuildResources_.length).to.be.equal(0);
+      expect(resources.schedulePass.calledTwice).to.be.true;
     });
 
     it('should not try to build resources already being built', () => {

test/integration/test-extensions-loading.js

@@ -21,8 +21,8 @@ function checkElementUpgrade(element) {
   expect(element).to.have.class('-amp-element');
   expect(element).to.have.class('-amp-layout-responsive');
   expect(element).to.have.class('-amp-layout-size-defined');
-  expect(element).to.not.have.class('-amp-notbuilt');
   expect(element).to.not.have.class('amp-notbuilt');
+  expect(element).to.not.have.class('-amp-notbuilt');
   expect(element).to.not.have.class('amp-unresolved');
   expect(element).to.not.have.class('-amp-unresolved');
 }
@@ -40,7 +40,7 @@ function testLoadOrderFixture(fixtureName, testElements) {
       expect(fixture.doc.querySelectorAll(testElements[i]))
           .to.have.length(1);
     }
-    return fixture.awaitEvent('amp:load:start', 1);
+    return fixture.awaitEvent('amp:load:start', testElements.length);
   }).then(() => {
     for (let i = 0; i < testElements.length; i++) {
       const testElement = fixture.doc.querySelectorAll(testElements[i])[0];

testing/screenshots/make-screenshot.js

@@ -224,6 +224,7 @@ page.open(url, function() {
       resources.forEach(function(resource) {
         log('Resource started: ' + resource.debugid);
         prepareResource(resource);
+        // Note: forceAll is no longer available.
         resource.forceAll().then(function() {
           completeResource(resource);
         }, function(reason) {