From 3a2c0c887498e3bc8b78333d09ae48eded1b27b6 Mon Sep 17 00:00:00 2001
From: Jake Fried <samouri@users.noreply.github.com>
Date: Thu, 23 Jan 2020 11:29:28 -0500
Subject: [PATCH] address feedback

---
 extensions/amp-script/0.1/amp-script.js       |  8 ++++
 .../0.1/test/unit/test-amp-script.js          | 40 ++++++++++++++-----
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/extensions/amp-script/0.1/amp-script.js b/extensions/amp-script/0.1/amp-script.js
index cf226fc8a7ac4..8f8dfb5f70258 100644
--- a/extensions/amp-script/0.1/amp-script.js
+++ b/extensions/amp-script/0.1/amp-script.js
@@ -84,6 +84,9 @@ export class AmpScript extends AMP.BaseElement {
     /** @private {string} */
     this.debugId_ = 'amp-script[unknown].js';
 
+    /** @private {boolean} */
+    this.layoutCallbackHappened_ = false;
+
     /**
      * If true, most production constraints are disabled including script size,
      * script hash sum for local scripts, etc. Default is false.
@@ -138,6 +141,10 @@ export class AmpScript extends AMP.BaseElement {
    */
   onLayoutMeasure() {
     super.onLayoutMeasure();
+    if (this.layoutCallbackHappened_) {
+      return;
+    }
+
     const {width, height} = this.getLayoutBox();
     if (width === 0 && height === 0) {
       user().warn(
@@ -166,6 +173,7 @@ export class AmpScript extends AMP.BaseElement {
 
   /** @override */
   layoutCallback() {
+    this.layoutCallbackHappened_ = true;
     this.userActivation_ = new UserActivationTracker(this.element);
 
     // The displayed name of the combined script in dev tools.
diff --git a/extensions/amp-script/0.1/test/unit/test-amp-script.js b/extensions/amp-script/0.1/test/unit/test-amp-script.js
index b0943db5e4e85..54285b161ac51 100644
--- a/extensions/amp-script/0.1/test/unit/test-amp-script.js
+++ b/extensions/amp-script/0.1/test/unit/test-amp-script.js
@@ -94,17 +94,35 @@ describes.fakeWin('AmpScript', {amp: {runtimeOn: false}}, env => {
     expect(service.checkSha384).to.be.called;
   });
 
-  it('should warn if there is zero width/height', () => {
-    const warnStub = env.sandbox.stub(user(), 'warn');
-    env.sandbox.stub(script, 'getLayoutBox').returns({height: 0, width: 0});
-    script.onLayoutMeasure();
-
-    expect(warnStub).calledWith(
-      'amp-script',
-      'Skipped initializing amp-script due to zero width and height.',
-      script.element
-    );
-    expect(warnStub).to.have.callCount(1);
+  describe('Initialization skipped warning due to zero height/width', () => {
+    it('should not warn when there is positive width/height', () => {
+      const warnStub = env.sandbox.stub(user(), 'warn');
+      env.sandbox.stub(script, 'getLayoutBox').returns({height: 1, width: 1});
+      script.onLayoutMeasure();
+      expect(warnStub).to.have.callCount(0);
+    });
+
+    it('should warn if there is zero width/height', () => {
+      const warnStub = env.sandbox.stub(user(), 'warn');
+      env.sandbox.stub(script, 'getLayoutBox').returns({height: 0, width: 0});
+      script.onLayoutMeasure();
+
+      expect(warnStub).calledWith(
+        'amp-script',
+        'Skipped initializing amp-script due to zero width and height.',
+        script.element
+      );
+      expect(warnStub).to.have.callCount(1);
+    });
+
+    it('should only warn if layoutCallback hasnt happened', () => {
+      const warnStub = env.sandbox.stub(user(), 'warn');
+      allowConsoleError(() => {
+        script.layoutCallback();
+      });
+      script.onLayoutMeasure();
+      expect(warnStub).to.have.callCount(0);
+    });
   });
 
   it('should fail on invalid sha384(author_js) for cross-origin src', () => {