review fixes

build-system/tasks/presubmit-checks.js

@@ -147,7 +147,6 @@ const forbiddenTerms = {
       'extensions/amp-analytics/0.1/requests.js',
     ],
   },
-  // Service factories that should only be installed once.
   '\\.buildInternal': {
     message: 'can only be called by the framework',
     allowlist: [
@@ -160,6 +159,7 @@ const forbiddenTerms = {
     message: 'can only be used by the runtime',
     allowlist: ['src/custom-element.js', 'src/service/builder.js'],
   },
+  // Service factories that should only be installed once.
   'installActionServiceForDoc': {
     message: privateServiceFactory,
     allowlist: [

src/base-element.js

@@ -108,10 +108,8 @@ export class BaseElement {
    * Whether this element supports V2 protocol, which includes:
    * 1. Layout/unlayout are not managed by the runtime, but instead are
    *    implemented by the element as needed.
-   * 2. The element will wait until it's fully parsed before building, unless
-   *    it's mutable. See `mutable`.
-   * 3. The element can defer its build until later. See `deferredBuild`.
-   * 4. The construction of the element is delayed until build.
+   * 2. The element can defer its build until later. See `deferredBuild`.
+   * 3. The construction of the element is delayed until build.
    *
    * Notice, in this mode `layoutCallback`, `pauseCallback`, `onLayoutMeasure`,
    * `getLayoutSize`, and other methods are deprecated. The element must
@@ -124,22 +122,6 @@ export class BaseElement {
     return false;
   }
 
-  /**
-   * Whether this element supports mutations. A mutable element can be built
-   * immediately, even before the element has been fully parsed, thus it should
-   * be able to apply additional markup when it's parsed. Normally, however,
-   * the element will wait until it's fully parsed before building to save
-   * resources.
-   *
-   * Only used for V2 elements.
-   *
-   * @return {boolean}
-   * @nocollapse
-   */
-  static mutable() {
-    return false;
-  }
-
   /**
    * Whether this element supports deferred-build mode. In this mode, the
    * element's build will be deferred roughly based on the

src/custom-element.js

@@ -645,7 +645,6 @@ function createBaseCustomElementClass(win) {
         return;
       }
 
-      // V2 processing.
       switch (state) {
         case ReadyState.LOADING:
           this.signals_.signal(CommonSignals.LOAD_START);
@@ -706,16 +705,6 @@ function createBaseCustomElementClass(win) {
       return this.implClass_ ? this.implClass_.V2() : false;
     }
 
-    /**
-     * See `BaseElement.mutable()`.
-     *
-     * @return {boolean}
-     * @final
-     */
-    mutable() {
-      return this.implClass_ ? this.implClass_.mutable() : false;
-    }
-
     /**
      * See `BaseElement.deferredBuild()`.
      *
@@ -1029,33 +1018,37 @@ function createBaseCustomElementClass(win) {
      * @private @final
      */
     upgradeOrSchedule_() {
-      if (this.V2()) {
-        if (!this.buildingPromise_) {
-          this.onReadyStateInternal(ReadyState.BUILDING);
-          const builder = getBuilderForDoc(this.getAmpDoc());
-          builder.schedule(this);
-
-          // Preconnect, since the build scheduling may take a while.
-          const urls = this.implClass_.getPreconnects(this);
-          if (urls && urls.length > 0) {
-            // If we do early preconnects we delay them a bit. This is kind of
-            // an unfortunate trade off, but it seems faster, because the DOM
-            // operations themselves are not free and might delay
-            const ampdoc = this.getAmpDoc();
-            startupChunk(ampdoc, () => {
-              const {win} = ampdoc;
-              if (!win) {
-                return;
-              }
-              const preconnect = Services.preconnectFor(win);
-              urls.forEach((url) =>
-                preconnect.url(ampdoc, url, /* alsoConnecting */ false)
-              );
-            });
-          }
-        }
-      } else {
+      if (!this.V2()) {
         this.tryUpgrade_();
+        return;
+      }
+      if (this.buildingPromise_) {
+        // Already building.
+        return;
+      }
+
+      // Schedule build.
+      this.onReadyStateInternal(ReadyState.BUILDING);
+      const builder = getBuilderForDoc(this.getAmpDoc());
+      builder.schedule(this);
+
+      // Schedule preconnects.
+      const urls = this.implClass_.getPreconnects(this);
+      if (urls && urls.length > 0) {
+        // If we do early preconnects we delay them a bit. This is kind of
+        // an unfortunate trade off, but it seems faster, because the DOM
+        // operations themselves are not free and might delay
+        const ampdoc = this.getAmpDoc();
+        startupChunk(ampdoc, () => {
+          const {win} = ampdoc;
+          if (!win) {
+            return;
+          }
+          const preconnect = Services.preconnectFor(win);
+          urls.forEach((url) =>
+            preconnect.url(ampdoc, url, /* alsoConnecting */ false)
+          );
+        });
       }
     }
 
@@ -1595,6 +1588,8 @@ function createBaseCustomElementClass(win) {
           this.actionQueue_ = [];
         }
         devAssert(this.actionQueue_).push(invocation);
+        // Schedule build sooner.
+        this.build();
       } else {
         this.executionAction_(invocation, false);
       }
@@ -1997,3 +1992,12 @@ export function getImplClassSyncForTesting(element) {
 export function getImplSyncForTesting(element) {
   return element.impl_;
 }
+
+/**
+ * @param {!AmpElement} element
+ * @return {?Array<!./service/action-impl.ActionInvocation>|undefined}
+ * @visibleForTesting
+ */
+export function getActionQueueForTesting(element) {
+  return element.actionQueue_;
+}

src/service/builder.js

@@ -71,14 +71,7 @@ export class Builder {
    */
   scheduleAsap(target) {
     this.targets_.set(target, {asap: true, isIntersecting: false});
-
-    if (target.mutable() && this.parsingTargets_) {
-      // Don't need to wait until parsing is complete.
-      removeItem(this.parsingTargets_, target);
-      this.maybeBuild_(target);
-    } else {
-      this.waitParsing_(target);
-    }
+    this.waitParsing_(target);
   }
 
   /**
@@ -189,7 +182,7 @@ export class Builder {
         continue;
       }
 
-      this.targets_.set(target, {...current, isIntersecting});
+      this.targets_.set(target, {asap: current.asap, isIntersecting});
       if (isIntersecting) {
         this.maybeBuild_(target);
       }

test/unit/test-amp-img-v2.js

@@ -17,7 +17,7 @@
 import {AmpImg} from '../../builtins/amp-img';
 import {BaseElement} from '../../src/base-element';
 import {Layout, LayoutPriority} from '../../src/layout';
-import {dispatchCustomEvent} from '../../src/dom';
+import {createElementWithAttributes, dispatchCustomEvent} from '../../src/dom';
 import {testElementV2} from '../../testing/element-v2';
 
 describes.realWin('amp-img V2', {amp: true}, (env) => {
@@ -44,10 +44,7 @@ describes.realWin('amp-img V2', {amp: true}, (env) => {
   });
 
   function createImg(attributes, children) {
-    const img = doc.createElement('amp-img');
-    for (const key in attributes) {
-      img.setAttribute(key, attributes[key]);
-    }
+    const img = createElementWithAttributes(doc, 'amp-img', attributes);
 
     if (children != null) {
       for (let i = 0; i < children.length; i++) {

test/unit/test-builder.js

@@ -70,7 +70,6 @@ describes.realWin('Builder', {amp: true}, (env) => {
     element.prerenderAllowed = () => options.prerenderAllowed || false;
     element.getBuildPriority = () =>
       options.buildPriority || LayoutPriority.CONTENT;
-    element.mutable = () => options.mutable || false;
     element.buildInternal = env.sandbox.stub();
     return element;
   }
@@ -191,14 +190,6 @@ describes.realWin('Builder', {amp: true}, (env) => {
       expect(element2.buildInternal).to.not.be.called;
     });
 
-    it('should build asap when mutab;e', async () => {
-      const element = createAmpElement({deferredBuild: false, mutable: true});
-      doc.body.appendChild(element);
-      builder.scheduleAsap(element);
-      clock.tick(1);
-      expect(element.buildInternal).to.be.calledOnce;
-    });
-
     it('should wait the deferred even when parsed', async () => {
       await setAmpdocReady();
       const element = createAmpElement({deferredBuild: true});

test/unit/test-custom-element-v2.js

@@ -22,6 +22,7 @@ import {Services} from '../../src/services';
 import {chunkInstanceForTesting} from '../../src/chunk';
 import {
   createAmpElementForTesting,
+  getActionQueueForTesting,
   getImplClassSyncForTesting,
   getImplSyncForTesting,
 } from '../../src/custom-element';
@@ -563,4 +564,65 @@ describes.realWin('CustomElement V2', {amp: true}, (env) => {
       expect(loadEventSpy).to.be.calledOnce; // no change.
     });
   });
+
+  describe('executeAction', () => {
+    beforeEach(async () => {
+      builderMock.expects('schedule').atLeast(0);
+    });
+
+    it('should enqueue actions until built and schedule build', () => {
+      const element = new ElementClass();
+      const handler = env.sandbox.stub(TestElement.prototype, 'executeAction');
+
+      builderMock.expects('scheduleAsap').withExactArgs(element).once();
+
+      doc.body.appendChild(element);
+
+      const inv = {};
+      element.enqueAction(inv);
+      const actionQueue = getActionQueueForTesting(element);
+      expect(actionQueue.length).to.equal(1);
+      expect(actionQueue[0]).to.equal(inv);
+      expect(handler).to.not.be.called;
+    });
+
+    it('should execute action immediately after built', async () => {
+      builderMock.expects('scheduleAsap').never();
+      const element = new ElementClass();
+      const handler = env.sandbox.stub(TestElement.prototype, 'executeAction');
+      doc.body.appendChild(element);
+      await element.buildInternal();
+
+      const inv = {};
+      element.enqueAction(inv);
+      expect(handler).to.be.calledOnce.calledWith(inv, false);
+      expect(getActionQueueForTesting(element)).to.not.exist;
+    });
+
+    it('should dequeue all actions after build', async () => {
+      const element = new ElementClass();
+      builderMock.expects('scheduleAsap').withExactArgs(element).atLeast(1);
+
+      const handler = env.sandbox.stub(TestElement.prototype, 'executeAction');
+
+      const inv1 = {};
+      const inv2 = {};
+      element.enqueAction(inv1);
+      element.enqueAction(inv2);
+      const actionQueue = getActionQueueForTesting(element);
+      expect(actionQueue).to.have.length(2);
+      expect(actionQueue[0]).to.equal(inv1);
+      expect(actionQueue[1]).to.equal(inv2);
+      expect(handler).to.not.be.called;
+
+      doc.body.appendChild(element);
+      await element.buildInternal();
+      await new Promise((resolve) => setTimeout(resolve, 10));
+
+      expect(getActionQueueForTesting(element)).to.not.exist;
+      expect(handler)
+        .to.be.calledTwice.calledWith(inv1, true)
+        .calledWith(inv2, true);
+    });
+  });
 });