diff --git a/src/service/mutator-impl.js b/src/service/mutator-impl.js index 11d47c66a24a..99a4e0f50d8d 100644 --- 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 index dd233526a01f..9b3de8eeceef 100644 --- 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,12 +471,12 @@ 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_(); } } } @@ -485,10 +484,9 @@ export class ResourcesImpl { /** * 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_) { @@ -496,17 +494,16 @@ export class ResourcesImpl { } 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 index 1328bfcbfdf9..e0c0341863ca 100644 --- 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 ); });