Remove grantBuildPermission() from resources-impl.js (ampproject#23345)

* Remove the reverse dependency of grantBuildPermission() from resources-impl.js

* add test

* update comment

src/service/resource.js

@@ -322,15 +322,10 @@ export class Resource {
   /**
    * Requests the resource's element to be built. See {@link AmpElement.build}
    * for details.
-   * @param {boolean=} force
    * @return {?Promise}
    */
-  build(force = false) {
-    if (
-      this.isBuilding_ ||
-      !this.element.isUpgraded() ||
-      (!force && !this.resources_.grantBuildPermission())
-    ) {
+  build() {
+    if (this.isBuilding_ || !this.element.isUpgraded()) {
       return null;
     }
     this.isBuilding_ = true;

src/service/resources-impl.js

@@ -522,7 +522,7 @@ export class Resources {
    * a finite number. Returns false if the number has been reached.
    * @return {boolean}
    */
-  grantBuildPermission() {
+  isUnderBuildQuota_() {
     // For pre-render we want to limit the amount of CPU used, so we limit
     // the number of elements build. For pre-render to "seem complete"
     // we only need to build elements in the first viewport. We can't know
@@ -531,7 +531,7 @@ export class Resources {
     // Most documents have 10 or less AMP tags. By building 20 we should not
     // change the behavior for the vast majority of docs, and almost always
     // catch everything in the first viewport.
-    return this.buildAttemptsCount_++ < 20 || this.viewer_.hasBeenVisible();
+    return this.buildAttemptsCount_ < 20 || this.viewer_.hasBeenVisible();
   }
 
   /**
@@ -553,7 +553,7 @@ export class Resources {
 
     // During prerender mode, don't build elements that aren't allowed to be
     // prerendered. This avoids wasting our prerender build quota.
-    // See grantBuildPermission() for more details.
+    // See isUnderBuildQuota_() for more details.
     const shouldBuildResource =
       this.viewer_.getVisibilityState() != VisibilityState.PRERENDER ||
       resource.prerenderAllowed();
@@ -621,8 +621,15 @@ export class Resources {
    * @private
    */
   buildResourceUnsafe_(resource, schedulePass, force = false) {
-    const promise = resource.build(force);
-    if (!promise || !schedulePass) {
+    if (!this.isUnderBuildQuota_() && !force) {
+      return null;
+    }
+    const promise = resource.build();
+    if (!promise) {
+      return null;
+    }
+    this.buildAttemptsCount_++;
+    if (!schedulePass) {
       return promise;
     }
     return promise.then(

test/unit/test-resource.js

@@ -116,27 +116,6 @@ describes.realWin('Resource', {amp: true}, env => {
     });
   });
 
-  it('should not build if permission is not granted', () => {
-    let permission = false;
-    elementMock
-      .expects('isUpgraded')
-      .returns(true)
-      .atLeast(1);
-    sandbox.stub(resources, 'grantBuildPermission').callsFake(() => permission);
-    elementMock.expects('updateLayoutBox').never();
-    expect(resource.build()).to.be.null;
-    expect(resource.getState()).to.equal(ResourceState.NOT_BUILT);
-
-    permission = true;
-    elementMock
-      .expects('build')
-      .returns(Promise.resolve())
-      .once();
-    return resource.build().then(() => {
-      expect(resource.getState()).to.equal(ResourceState.NOT_LAID_OUT);
-    });
-  });
-
   it('should blacklist on build failure', () => {
     sandbox
       .stub(resource, 'maybeReportErrorOnBuildFailure')

test/unit/test-resources.js

@@ -1515,20 +1515,6 @@ describe('Resources discoverWork', () => {
     expect(setInViewport).to.have.been.calledWith(true);
   });
 
-  it('should not grant permission to build when threshold reached', () => {
-    let hasBeenVisible = false;
-    sandbox
-      .stub(resources.viewer_, 'hasBeenVisible')
-      .callsFake(() => hasBeenVisible);
-
-    for (let i = 0; i < 20; i++) {
-      expect(resources.grantBuildPermission()).to.be.true;
-    }
-    expect(resources.grantBuildPermission()).to.be.false;
-    hasBeenVisible = true;
-    expect(resources.grantBuildPermission()).to.be.true;
-  });
-
   it('should build resource when not built', () => {
     const buildResourceSpy = sandbox.spy(resources, 'buildResourceUnsafe_');
     sandbox.stub(resources, 'schedule_');
@@ -1595,6 +1581,21 @@ describe('Resources discoverWork', () => {
     expect(resource1.build).to.not.be.called;
   });
 
+  it('should NOT build when quota reached', () => {
+    sandbox.stub(resources.viewer_, 'hasBeenVisible').callsFake(() => false);
+    sandbox.stub(resources, 'schedule_');
+    resources.documentReady_ = true;
+    resources.buildAttemptsCount_ = 21; // quota is 20
+
+    resource1.element.isBuilt = () => false;
+    resource1.prerenderAllowed = () => true;
+    resource1.state_ = ResourceState.NOT_BUILT;
+    resource1.build = sandbox.spy();
+
+    resources.buildOrScheduleBuildForResource_(resource1);
+    expect(resource1.build).to.not.be.called;
+  });
+
   it('should layout resource if outside viewport but idle', () => {
     const schedulePassStub = sandbox.stub(resources, 'schedulePass');
     resources.documentReady_ = true;
@@ -1615,8 +1616,7 @@ describe('Resources discoverWork', () => {
     const buildResourceSpy = sandbox.spy(resources, 'buildResourceUnsafe_');
     sandbox.stub(resources, 'schedule_');
     resources.documentReady_ = true;
-    // Emulates a resource not building, maybe because grantBuildPermission()
-    // returned false.
+    // Emulates a resource not building.
     resource1.element.isBuilt = sandbox.stub().returns(false);
     resource2.element.idleRenderOutsideViewport = () => false;
     resource1.state_ = ResourceState.NOT_BUILT;