A4a 3p remove load from init

extensions/amp-a4a/0.1/amp-a4a.js

@@ -508,7 +508,6 @@ export class AmpA4A extends AMP.BaseElement {
     // creatives which rendered via the buildCallback promise chain.  Ensure
     // slot counts towards 3p loading count until we know that the creative is
     // valid AMP.
-    this.emitLifecycleEvent('preAdThrottle');
     return this.adPromise_.then(rendered => {
       if (rendered instanceof Error || !rendered) {
         this.emitLifecycleEvent('preAdThrottle');
@@ -791,14 +790,7 @@ export class AmpA4A extends AMP.BaseElement {
     // TODO(keithwrightbos): noContentCallback?
     this.xOriginIframeHandler_ = new AMP.AmpAdXOriginIframeHandler(this);
     this.rendered_ = true;
-    // Set opt_defaultVisible to true as 3p draw code never executed causing
-    // render-start event never to fire which will remove visiblity hidden.
-    const handlerPromise = this.xOriginIframeHandler_.init(
-      iframe, /* opt_isA4A */ true);
-    if (getMode().localDev || getMode().test) {
-      this.onCrossDomainIframeCreated(iframe);
-    }
-    return handlerPromise;
+    return this.xOriginIframeHandler_.init(iframe, /* opt_isA4A */ true);
   }
 
   /**

extensions/amp-a4a/0.1/test/test-amp-a4a.js

@@ -128,18 +128,10 @@ describe('amp-a4a', () => {
     return element;
   }
 
-  function verifyNonAMPRender(a4a, win) {
+  function verifyNonAMPRender(a4a) {
     a4a.onAmpCreativeRender = () => {
       assert.fail('AMP creative should never have rendered!');
     };
-    a4a.onCrossDomainIframeCreated = iframe => {
-      // Iframe should be hidden at time of creation and only made visible
-      // after load.
-      expect(isStyleVisible(win, iframe)).to.be.false;
-      iframe.onload = () => {
-        expect(isStyleVisible(win, iframe)).to.be.true;
-      };
-    };
   }
 
   /**
@@ -175,7 +167,7 @@ describe('amp-a4a', () => {
     });
 
     it('for SafeFrame rendering case', () => {
-      verifyNonAMPRender(a4a, fixture.win);
+      verifyNonAMPRender(a4a);
       // Make sure there's no signature, so that we go down the 3p iframe path.
       mockResponse.headers.delete('X-Google-header');
       // If rendering type is safeframe, we SHOULD attach a SafeFrame.
@@ -193,7 +185,7 @@ describe('amp-a4a', () => {
     });
 
     it('for cached content iframe rendering case', () => {
-      verifyNonAMPRender(a4a, fixture.win);
+      verifyNonAMPRender(a4a);
       // Make sure there's no signature, so that we go down the 3p iframe path.
       mockResponse.headers.delete('X-Google-header');
       fixture.doc.body.appendChild(a4aElement);
@@ -280,7 +272,7 @@ describe('amp-a4a', () => {
         a4aElement.setAttribute('height', 50);
         a4aElement.setAttribute('type', 'adsense');
         const a4a = new MockA4AImpl(a4aElement);
-        verifyNonAMPRender(a4a, fixture.win);
+        verifyNonAMPRender(a4a);
         doc.body.appendChild(a4aElement);
         a4a.onLayoutMeasure();
         return a4a.layoutCallback().then(() => {
@@ -315,7 +307,7 @@ describe('amp-a4a', () => {
           a4aElement.setAttribute('height', 50);
           a4aElement.setAttribute('type', 'adsense');
           const a4a = new MockA4AImpl(a4aElement);
-          verifyNonAMPRender(a4a, fixture.win);
+          verifyNonAMPRender(a4a);
           doc.body.appendChild(a4aElement);
           a4a.onLayoutMeasure();
           return a4a.layoutCallback().then(() => {
@@ -464,7 +456,7 @@ describe('amp-a4a', () => {
         a4aElement.setAttribute('height', 50);
         a4aElement.setAttribute('type', 'adsense');
         const a4a = new MockA4AImpl(a4aElement);
-        verifyNonAMPRender(a4a, fixture.win);
+        verifyNonAMPRender(a4a);
         const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl');
         sandbox.stub(a4a, 'extractCreativeAndSignature').returns(
           Promise.resolve({creative: mockResponse.arrayBuffer()}));
@@ -570,7 +562,7 @@ describe('amp-a4a', () => {
         a4aElement.setAttribute('type', 'adsense');
         const a4a = new MockA4AImpl(a4aElement);
         const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl');
-        verifyNonAMPRender(a4a, fixture.win);
+        verifyNonAMPRender(a4a);
         a4a.onLayoutMeasure();
         expect(a4a.adPromise_).to.be.instanceof(Promise);
         return a4a.layoutCallback().then(() => {
@@ -592,7 +584,7 @@ describe('amp-a4a', () => {
         const doc = fixture.doc;
         const a4aElement = createA4aElement(doc);
         const a4a = new MockA4AImpl(a4aElement);
-        verifyNonAMPRender(a4a, fixture.win);
+        verifyNonAMPRender(a4a);
         a4a.onLayoutMeasure();
         return a4a.adPromise_.then(() => a4a.layoutCallback().then(() => {
           a4a.vsync_.runScheduledTasks_();
@@ -614,7 +606,7 @@ describe('amp-a4a', () => {
         const doc = fixture.doc;
         const a4aElement = createA4aElement(doc);
         const a4a = new MockA4AImpl(a4aElement);
-        verifyNonAMPRender(a4a, fixture.win);
+        verifyNonAMPRender(a4a);
         a4a.onLayoutMeasure();
         const layoutCallbackPromise = a4a.layoutCallback();
         rejectXhr(new Error('XHR Error'));

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

@@ -15,7 +15,6 @@
  */
 
 import {removeElement} from '../../../src/dom';
-import {loadPromise} from '../../../src/event-helper';
 import {
   SubscriptionApi,
   listenFor,
@@ -69,7 +68,7 @@ export class AmpAdXOriginIframeHandler {
    * Sets up listeners and iframe state for iframe containing ad creative.
    * @param {!Element} iframe
    * @param {boolean=} opt_isA4A when true do not listen to ad response
-   * @return {!Promise} awaiting load event for ad frame
+   * @return {!Promise} awaiting render complete promise
    * @suppress {checkTypes}  // TODO(tdrl): Temporary, for lifecycleReporter.
    */
   init(iframe, opt_isA4A) {
@@ -96,10 +95,19 @@ export class AmpAdXOriginIframeHandler {
           this.updateSize_(data.height, data.width, source, origin);
         }, true, true));
 
-    // Install API that listen to ad response
+    this.unlisteners_.push(this.viewer_.onVisibilityChanged(() => {
+      this.sendEmbedInfo_(this.baseInstance_.isInViewport());
+    }));
+
     if (opt_isA4A) {
-      this.adResponsePromise_ = Promise.resolve();
-    } else if (this.baseInstance_.config
+      // A4A writes creative frame directly to page therefore does not expect
+      // post message to unset visibility hidden
+      this.element_.appendChild(this.iframe);
+      return Promise.resolve();
+    }
+
+    // Install API that listen to ad response
+    if (this.baseInstance_.config
         && this.baseInstance_.config.renderStartImplemented) {
       // If support render-start, create a race between render-start no-content
       this.adResponsePromise_ = listenForOncePromise(this.iframe,
@@ -124,23 +132,17 @@ export class AmpAdXOriginIframeHandler {
     // post message depending on if A4A flow.
     this.iframe.style.visibility = 'hidden';
 
-    this.unlisteners_.push(this.viewer_.onVisibilityChanged(() => {
-      this.sendEmbedInfo_(this.baseInstance_.isInViewport());
-    }));
-
     this.element_.appendChild(this.iframe);
-    return loadPromise(this.iframe).then(() => {
-      return timerFor(this.baseInstance_.win).timeoutPromise(TIMEOUT_VALUE,
-          this.adResponsePromise_,
-          'timeout waiting for ad response').catch(e => {
-            this.noContent_();
-            user().warn('AMP-AD', e);
-          }).then(() => {
-            if (this.iframe) {
-              this.iframe.style.visibility = '';
-            }
-          });
-    });
+    return timerFor(this.baseInstance_.win).timeoutPromise(TIMEOUT_VALUE,
+        this.adResponsePromise_,
+        'timeout waiting for ad response').catch(e => {
+          this.noContent_();
+          user().warn('AMP-AD', e);
+        }).then(() => {
+          if (this.iframe) {
+            this.iframe.style.visibility = '';
+          }
+        });
   }
 
   /**

extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js

@@ -61,7 +61,7 @@ describe('amp-ad-xorigin-iframe-handler', () => {
 
       let noContentSpy;
 
-      beforeEach(done => {
+      beforeEach(() => {
         adImpl.config = {renderStartImplemented: true};
         sandbox.stub(adImpl, 'attemptChangeSize', (height, width) => {
           expect(height).to.equal(217);
@@ -72,9 +72,6 @@ describe('amp-ad-xorigin-iframe-handler', () => {
             sandbox.spy/*OK*/(iframeHandler, 'freeXOriginIframe');
 
         initPromise = iframeHandler.init(iframe);
-        iframe.onload = () => {
-          done();
-        };
       });
 
       it('should resolve on message "render-start"', () => {
@@ -191,17 +188,15 @@ describe('amp-ad-xorigin-iframe-handler', () => {
 
       iframe.name = 'test_master';
       initPromise = iframeHandler.init(iframe);
-      iframe.onload = () => {
-        clock.tick(9999);
-        expect(noContentSpy).to.not.be.called;
-        clock.tick(1);
-        initPromise.then(() => {
-          expect(iframe.style.visibility).to.equal('');
-          expect(noContentSpy).to.be.calledOnce;
-          expect(noContentSpy).to.be.calledWith(true);
-          done();
-        });
-      };
+      clock.tick(9999);
+      expect(noContentSpy).to.not.be.called;
+      clock.tick(1);
+      initPromise.then(() => {
+        expect(iframe.style.visibility).to.equal('');
+        expect(noContentSpy).to.be.calledOnce;
+        expect(noContentSpy).to.be.calledWith(true);
+        done();
+      });
     });
 
     it('should resolve directly if it is A4A', () => {
@@ -213,11 +208,8 @@ describe('amp-ad-xorigin-iframe-handler', () => {
 
   describe('Initialized iframe', () => {
 
-    beforeEach(done => {
+    beforeEach(() => {
       iframeHandler.init(iframe);
-      iframe.onload = () => {
-        done();
-      };
     });
 
     it('should be able to use embed-state API', () => {