intersect-resources: Always schedule pass after build (#27657)

* Always schedule pass after build. * Revert viewport-binding-natural debug code. * Fix test-resources.js.
ampproject · Apr 9, 2020 · 6043a17 · 6043a17
1 parent 3207eea
commit 6043a17
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 32 deletions.
diff --git a/src/service/mutator-impl.js b/src/service/mutator-impl.js
@@ -231,13 +231,13 @@ export class MutatorImpl {
       },
       mutate: () => {
         mutator();
+
+        // `skipRemeasure` is set by callers when we know that `mutator`
+        // cannot cause a change in size/position e.g. toggleLoading().
         if (skipRemeasure) {
           return;
         }
 
-        // TODO(willchou): toggleLoading() mutate causes a lot of unnecessary
-        // remeasures. Add affordance to mutateElement() to disable remeasures.
-
         if (element.classList.contains('i-amphtml-element')) {
           const r = Resource.forElement(element);
           r.requestMeasure();

diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js
@@ -422,6 +422,7 @@ export class ResourcesImpl {
     this.resources_.push(resource);
 
     if (this.intersectionObserver_) {
+      // The observer callback will schedule a pass to process this element.
       this.intersectionObserver_.observe(element);
     } else {
       this.remeasurePass_.schedule(1000);
@@ -450,14 +451,12 @@ export class ResourcesImpl {
    * resources.
    * @param {!Resource} resource
    * @param {boolean=} checkForDupes
-   * @param {boolean=} scheduleWhenBuilt
    * @param {boolean=} ignoreQuota
    * @private
    */
   buildOrScheduleBuildForResource_(
     resource,
     checkForDupes = false,
-    scheduleWhenBuilt = true,
     ignoreQuota = false
   ) {
     const buildingEnabled = this.isRuntimeOn_ || this.isBuildOn_;
@@ -472,41 +471,39 @@ export class ResourcesImpl {
     if (buildingEnabled && shouldBuildResource) {
       if (this.documentReady_) {
         // Build resource immediately, the document has already been parsed.
-        this.buildResourceUnsafe_(resource, scheduleWhenBuilt, ignoreQuota);
+        this.buildResourceUnsafe_(resource, ignoreQuota);
       } else if (!resource.isBuilt() && !resource.isBuilding()) {
         if (!checkForDupes || !this.pendingBuildResources_.includes(resource)) {
           // Otherwise add to pending resources and try to build any ready ones.
           this.pendingBuildResources_.push(resource);
-          this.buildReadyResources_(scheduleWhenBuilt);
+          this.buildReadyResources_();
         }
       }
     }
   }
 
   /**
    * Builds resources that are ready to be built.
-   * @param {boolean=} scheduleWhenBuilt
    * @private
    */
-  buildReadyResources_(scheduleWhenBuilt = true) {
+  buildReadyResources_() {
     // Avoid cases where elements add more elements inside of them
     // and cause an infinite loop of building - see #3354 for details.
     if (this.isCurrentlyBuildingPendingResources_) {
       return;
     }
     try {
       this.isCurrentlyBuildingPendingResources_ = true;
-      this.buildReadyResourcesUnsafe_(scheduleWhenBuilt);
+      this.buildReadyResourcesUnsafe_();
     } finally {
       this.isCurrentlyBuildingPendingResources_ = false;
     }
   }
 
   /**
-   * @param {boolean=} scheduleWhenBuilt
    * @private
    */
-  buildReadyResourcesUnsafe_(scheduleWhenBuilt = true) {
+  buildReadyResourcesUnsafe_() {
     // 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.
@@ -519,19 +516,18 @@ export class ResourcesImpl {
         // 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);
-        this.buildResourceUnsafe_(resource, scheduleWhenBuilt);
+        this.buildResourceUnsafe_(resource);
       }
     }
   }
 
   /**
    * @param {!Resource} resource
-   * @param {boolean} schedulePass
    * @param {boolean=} ignoreQuota
    * @return {?Promise}
    * @private
    */
-  buildResourceUnsafe_(resource, schedulePass, ignoreQuota = false) {
+  buildResourceUnsafe_(resource, ignoreQuota = false) {
     if (
       !this.isUnderBuildQuota_() &&
       !ignoreQuota &&
@@ -546,10 +542,6 @@ export class ResourcesImpl {
       return null;
     }
     this.buildAttemptsCount_++;
-    // With IntersectionObserver, no need to schedule passes after build.
-    if (!schedulePass || this.intersectionObserver_) {
-      return promise;
-    }
     return promise.then(
       () => this.schedulePass(),
       (error) => {
@@ -585,6 +577,7 @@ export class ResourcesImpl {
       resource.pauseOnRemove();
     }
     if (this.intersectionObserver_) {
+      // TODO(willchou): Fix observe/unobserve churn due to reparenting.
       this.intersectionObserver_.unobserve(resource.element);
     }
     this.cleanupTasks_(resource, /* opt_removePending */ true);
@@ -1315,7 +1308,6 @@ export class ResourcesImpl {
           this.buildOrScheduleBuildForResource_(
             r,
             /* checkForDupes */ true,
-            /* scheduleWhenBuilt */ undefined,
             /* ignoreQuota */ true
           );
         }

diff --git a/test/unit/test-resources.js b/test/unit/test-resources.js
@@ -366,7 +366,12 @@ describe('Resources', () => {
         getTaskId: () => 'resource#L',
         applySizesAndMediaQuery: () => {},
       };
-      resources.scheduleLayoutOrPreload(resource, true, 0, /* force */ true);
+      resources.scheduleLayoutOrPreload(
+        resource,
+        true,
+        0,
+        /* ignoreQuota */ true
+      );
       expect(resources.queue_.getSize()).to.equal(1);
       expect(resources.queue_.tasks_[0].forceOutsideViewport).to.be.true;
     }
@@ -931,7 +936,12 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => {
   });
 
   it('should force schedule resource execution outside viewport', () => {
-    resources.scheduleLayoutOrPreload(resource1, true, 0, /* force */ true);
+    resources.scheduleLayoutOrPreload(
+      resource1,
+      true,
+      0,
+      /* ignoreQuota */ true
+    );
     expect(resources.queue_.getSize()).to.equal(1);
     expect(resources.queue_.tasks_[0].resource).to.equal(resource1);
 
@@ -1046,8 +1056,7 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => {
     expect(resource1.build).to.be.calledOnce;
     expect(buildResourceSpy).calledWithExactly(
       resource1,
-      /* schedulePass */ true,
-      /* force */ false
+      /* ignoreQuota */ false
     );
   });
 
@@ -1069,10 +1078,7 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => {
     resources.discoverWork_();
 
     expect(resource1.build).to.be.calledOnce;
-    expect(buildResourceSpy).calledWithExactly(
-      resource1,
-      /* schedulePass */ true
-    );
+    expect(buildResourceSpy).calledWithExactly(resource1);
   });
 
   it('should NOT build non-prerenderable resources in prerender', () => {
@@ -1159,14 +1165,12 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => {
     // discoverWork_ phase 1 build.
     expect(buildResourceSpy).calledWithExactly(
       resource1,
-      /* schedulePass */ true,
-      /* force */ false
+      /* ignoreQuota */ false
     );
     // discoverWork_ phase 4 layout grants build.
     expect(buildResourceSpy).calledWithExactly(
       resource1,
-      /* schedulePass */ true,
-      /* force */ true
+      /* ignoreQuota */ true
     );
   });