From 268782086f5d30fb099ae947a87c6b5f16cfd86d Mon Sep 17 00:00:00 2001 From: glevitzky Date: Wed, 31 Aug 2016 18:00:28 -0400 Subject: [PATCH 1/5] Update amp-ad-network-doubleclick-impl.md --- .../amp-ad-network-doubleclick-impl.md | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md b/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md index 3940dd4afda0..4a10cbb8aad9 100644 --- a/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md +++ b/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md @@ -48,5 +48,26 @@ Example - DoubleClick Ad ``` +Example - DoubleClick Ad with Multi-size Request +```html + + +``` + +Example - DoubleClick Ad with Multi-size Request Ignoring Size Validation +```html + + +``` + #### Attributes -TODO: Add attributes +Below the term `primary size` refers to the width and height pair specified by the `width` and `height` attributes of the tag. +- `data-multi-size` A string of comma separated sizes, which if present, forces the tag to request an ad with all of the given sizes, including the primary size. Each individual size must be a number (the width) followed by a lowercase 'x' followed by a number (the height). Each dimension specified this way must not be larger than its counterpart in the primary size. Further, each dimension must be no less than 2/3rds of the corresponding primary dimension, unless `data-mutli-size-validation` is set to false. +- `data-multi-size-validation` If set to false, this will allow secondary sizes (those specified in the `data-multi-size` attribute) to be less than 2/3rds of the corresponding primary size. By default this is assumed to be true. From 8d8c6dd858ae7d5d1e3962f4182fb3f91a60d295 Mon Sep 17 00:00:00 2001 From: glevitzky Date: Wed, 31 Aug 2016 18:02:50 -0400 Subject: [PATCH 2/5] Reverting doc changes The previous change was made in error (wrong branch). --- .../amp-ad-network-doubleclick-impl.md | 23 +------------------ 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md b/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md index 4a10cbb8aad9..3940dd4afda0 100644 --- a/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md +++ b/extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl.md @@ -48,26 +48,5 @@ Example - DoubleClick Ad ``` -Example - DoubleClick Ad with Multi-size Request -```html - - -``` - -Example - DoubleClick Ad with Multi-size Request Ignoring Size Validation -```html - - -``` - #### Attributes -Below the term `primary size` refers to the width and height pair specified by the `width` and `height` attributes of the tag. -- `data-multi-size` A string of comma separated sizes, which if present, forces the tag to request an ad with all of the given sizes, including the primary size. Each individual size must be a number (the width) followed by a lowercase 'x' followed by a number (the height). Each dimension specified this way must not be larger than its counterpart in the primary size. Further, each dimension must be no less than 2/3rds of the corresponding primary dimension, unless `data-mutli-size-validation` is set to false. -- `data-multi-size-validation` If set to false, this will allow secondary sizes (those specified in the `data-multi-size` attribute) to be less than 2/3rds of the corresponding primary size. By default this is assumed to be true. +TODO: Add attributes From 266418fa07a21bb1e295581a9b47951e8a4ac37c Mon Sep 17 00:00:00 2001 From: keithwrightbos Date: Tue, 1 Nov 2016 10:29:00 -0400 Subject: [PATCH 3/5] Remove load listener from AmpAdXOriginIframeHandler.init --- extensions/amp-a4a/0.1/amp-a4a.js | 54 +++++-------- extensions/amp-a4a/0.1/test/test-amp-a4a.js | 66 +++++++++++----- .../0.1/amp-ad-xorigin-iframe-handler.js | 77 +++++++++---------- .../test-amp-ad-xorigin-iframe-handler.js | 40 ++++------ 4 files changed, 120 insertions(+), 117 deletions(-) diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index 1f5fc7069460..1dc04cadbc12 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -15,7 +15,6 @@ */ import { allowRenderOutsideViewport, - decrementLoadingAds, incrementLoadingAds, } from '../../amp-ad/0.1/concurrent-load'; import {adConfig} from '../../../ads/_config'; @@ -149,9 +148,6 @@ export class AmpA4A extends AMP.BaseElement { /** @private {boolean} */ this.rendered_ = false; - /** @private {number} ID of timer used as part of 3p throttling. */ - this.timerId_ = 0; - /** @private {boolean} whether layoutMeasure has been executed. */ this.layoutMeasureExecuted_ = false; @@ -240,6 +236,13 @@ export class AmpA4A extends AMP.BaseElement { * @override */ preconnectCallback(unusedOnLayout) { + // TODO(tdrl): Temporary, while we're verifying whether SafeFrame is an + // acceptable solution to the 'Safari on iOS doesn't fetch iframe src from + // cache' issue. See https://github.com/ampproject/amphtml/issues/5614 + this.preconnect.url(SAFEFRAME_IMPL_PATH); + if (!this.config) { + return; + } const preconnect = this.config.preconnect; // NOTE(keithwrightbos): using onLayout to indicate if preconnect should be // given preferential treatment. Currently this would be false when @@ -256,10 +259,6 @@ export class AmpA4A extends AMP.BaseElement { this.preconnect.url(p, true); }); } - // TODO(tdrl): Temporary, while we're verifying whether SafeFrame is an - // acceptable solution to the 'Safari on iOS doesn't fetch iframe src from - // cache' issue. See https://github.com/ampproject/amphtml/issues/5614 - this.preconnect.url(SAFEFRAME_IMPL_PATH); } /** @override */ @@ -482,19 +481,20 @@ 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.lifecycleReporter.sendPing('preAdThrottle'); - this.timerId_ = incrementLoadingAds(this.win); return this.adPromise_.then(rendered => { if (rendered instanceof Error || !rendered) { + this.lifecycleReporter.sendPing('preAdThrottle'); + incrementLoadingAds(this.win); // Haven't rendered yet, so try rendering via one of our // cross-domain iframe solutions. if (this.experimentalNonAmpCreativeRenderMethod_ == 'safeframe' && this.creativeBody_) { - this.renderViaSafeFrame_(this.creativeBody_); + const renderPromise = this.renderViaSafeFrame_(this.creativeBody_); this.creativeBody_ = null; // Free resources. this.experimentalNonAmpCreativeRenderMethod_ = null; + return renderPromise; } else if (this.adUrl_) { - this.renderViaCachedContentIframe_(this.adUrl_, true); + return this.renderViaCachedContentIframe_(this.adUrl_); } else { throw new Error('No creative or URL available -- A4A can\'t render' + ' any ad'); @@ -526,7 +526,6 @@ export class AmpA4A extends AMP.BaseElement { this.creativeBody_ = null; this.experimentalNonAmpCreativeRenderMethod_ = null; this.rendered_ = false; - this.timerId_ = 0; if (this.xOriginIframeHandler_) { this.xOriginIframeHandler_.freeXOriginIframe(); this.xOriginIframeHandler_ = null; @@ -676,12 +675,6 @@ export class AmpA4A extends AMP.BaseElement { */ maybeRenderAmpAd_(bytes) { this.lifecycleReporter.sendPing('renderFriendlyStart'); - // Timer id will be set if we have entered layoutCallback at which point - // 3p throttling count was incremented. We want to "release" the throttle - // immediately since we now know we are not a 3p ad. - if (this.timerId_) { - decrementLoadingAds(this.timerId_, this.win); - } // AMP documents are required to be UTF-8 return utf8Decode(bytes).then(creative => { // Find the json blob located at the end of the body and parse it. @@ -757,18 +750,14 @@ export class AmpA4A extends AMP.BaseElement { * Shared functionality for cross-domain iframe-based rendering methods. * @param {!Element} iframe Iframe to render. Should be fully configured * (all attributes set), but not yet attached to DOM. - * @param is3p Whether the content is 3p / general HTML vs. verified A4A. + * @return {!Promise} awaiting load event for ad frame * @private */ - iframeRenderHelper_(iframe, is3p) { + iframeRenderHelper_(iframe) { // TODO(keithwrightbos): noContentCallback? this.xOriginIframeHandler_ = new AMP.AmpAdXOriginIframeHandler(this); - // TODO(keithwrightbos): init returns load event, do we need to wait? - // Set opt_defaultVisible to true as 3p draw code never executed causing - // render-start event never to fire which will remove visiblity hidden. - this.xOriginIframeHandler_.init(iframe, - is3p, /* opt_defaultVisible */ true, /* opt_isA4A */ true); this.rendered_ = true; + return this.xOriginIframeHandler_.init(iframe, /* opt_isA4A */ true); } /** @@ -783,12 +772,10 @@ export class AmpA4A extends AMP.BaseElement { * * @param {string} adUrl Ad request URL, as sent to #sendXhrRequest_ (i.e., * before any modifications that XHR module does to it.) - * @param {boolean=} opt_isNonAmpCreative whether creative within iframe - * is AMP creative (if not, intersection observer allows sending info into - * nested frames). + * @return {!Promise} awaiting load event for ad frame * @private */ - renderViaCachedContentIframe_(adUrl, opt_isNonAmpCreative) { + renderViaCachedContentIframe_(adUrl) { this.lifecycleReporter.sendPing('renderCrossDomainStart'); /** @const {!Element} */ const iframe = createElementWithAttributes( @@ -802,17 +789,18 @@ export class AmpA4A extends AMP.BaseElement { // modified url. 'src': xhrFor(this.win).getCorsUrl(this.win, adUrl), }, SHARED_IFRAME_PROPERTIES)); - this.iframeRenderHelper_(iframe, /* is3p */ !!opt_isNonAmpCreative); + return this.iframeRenderHelper_(iframe); } /** * Render creative via SafeFrame. * @param {!ArrayBuffer} creativeBody The creative, as raw bytes. + * @return {!Promise} awaiting load event for ad frame * @private */ renderViaSafeFrame_(creativeBody) { this.lifecycleReporter.sendPing('renderSafeFrameStart'); - utf8Decode(creativeBody).then(creative => { + return utf8Decode(creativeBody).then(creative => { /** @const {!Element} */ const iframe = createElementWithAttributes( /** @type {!Document} */(this.element.ownerDocument), @@ -822,7 +810,7 @@ export class AmpA4A extends AMP.BaseElement { 'src': SAFEFRAME_IMPL_PATH + '?n=0', 'name': `${SAFEFRAME_VERSION};${creative.length};${creative}`, }, SHARED_IFRAME_PROPERTIES)); - this.iframeRenderHelper_(iframe, true); + return this.iframeRenderHelper_(iframe); }); } diff --git a/extensions/amp-a4a/0.1/test/test-amp-a4a.js b/extensions/amp-a4a/0.1/test/test-amp-a4a.js index 0e4a55bf4293..bdd562031684 100644 --- a/extensions/amp-a4a/0.1/test/test-amp-a4a.js +++ b/extensions/amp-a4a/0.1/test/test-amp-a4a.js @@ -33,9 +33,12 @@ import {a4aRegistry} from '../../../../ads/_a4a-config'; import '../../../../extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler'; import * as sinon from 'sinon'; +const XHR_URL = 'http://iframe.localhost:' + location.port + + '/test/fixtures/served/iframe.html?args'; + class MockA4AImpl extends AmpA4A { getAdUrl() { - return Promise.resolve('https://test.location.org/ad/012345?args'); + return Promise.resolve(XHR_URL); } extractCreativeAndSignature(responseArrayBuffer, responseHeaders) { @@ -121,6 +124,12 @@ describe('amp-a4a', () => { return element; } + function verifyNonAMPRender(a4a) { + a4a.onAmpCreativeRender = () => { + assert.fail('AMP creative should never have rendered!'); + }; + } + /** * * @param {!Window} win @@ -136,7 +145,7 @@ describe('amp-a4a', () => { let a4a; let fixture; beforeEach(() => { - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -154,6 +163,7 @@ describe('amp-a4a', () => { }); it('for SafeFrame rendering case', () => { + 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. @@ -171,6 +181,7 @@ describe('amp-a4a', () => { }); it('for cached content iframe rendering case', () => { + 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); @@ -208,7 +219,7 @@ describe('amp-a4a', () => { // fixture.addElement() step fails with a 'element.build does not exist' // error. Skip this until we sort out how to properly do an E2E. it.skip('should render a single AMP ad in a friendly iframe', () => { - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -244,7 +255,7 @@ describe('amp-a4a', () => { mockResponse.headers.delete('X-Google-header'); // If rendering type is safeframe, we SHOULD attach a SafeFrame. mockResponse.headers.append(RENDERING_TYPE_HEADER, 'safeframe'); - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -257,6 +268,7 @@ describe('amp-a4a', () => { a4aElement.setAttribute('height', 50); a4aElement.setAttribute('type', 'adsense'); const a4a = new MockA4AImpl(a4aElement); + verifyNonAMPRender(a4a); doc.body.appendChild(a4aElement); a4a.onLayoutMeasure(); return a4a.layoutCallback().then(() => { @@ -278,7 +290,7 @@ describe('amp-a4a', () => { // If rendering type is anything but safeframe, we SHOULD NOT attach a // SafeFrame. mockResponse.headers.append(RENDERING_TYPE_HEADER, headerVal); - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -291,6 +303,7 @@ describe('amp-a4a', () => { a4aElement.setAttribute('height', 50); a4aElement.setAttribute('type', 'adsense'); const a4a = new MockA4AImpl(a4aElement); + verifyNonAMPRender(a4a); doc.body.appendChild(a4aElement); a4a.onLayoutMeasure(); return a4a.layoutCallback().then(() => { @@ -302,7 +315,7 @@ describe('amp-a4a', () => { const unsafeChild = a4aElement.querySelector('iframe'); expect(unsafeChild).to.be.ok; expect(unsafeChild.getAttribute('src')).to.have.string( - 'https://test.location.org/ad/012345?args'); + XHR_URL); }); }); }); @@ -312,7 +325,7 @@ describe('amp-a4a', () => { // Set safeframe header, but it should be ignored when a signature // exists and validates. mockResponse.headers.append(RENDERING_TYPE_HEADER, 'safeframe'); - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -346,7 +359,7 @@ describe('amp-a4a', () => { describe('#onLayoutMeasure', () => { it('should run end-to-end and render in friendly iframe', () => { - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -359,6 +372,10 @@ describe('amp-a4a', () => { a4aElement.setAttribute('height', 50); a4aElement.setAttribute('type', 'adsense'); const a4a = new MockA4AImpl(a4aElement); + let onAmpCreativeRenderFired = false; + a4a.onAmpCreativeRender = () => { + onAmpCreativeRenderFired = true; + }; const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl'); const extractCreativeAndSignatureSpy = sandbox.spy( a4a, 'extractCreativeAndSignature'); @@ -400,6 +417,7 @@ describe('amp-a4a', () => { 'link[href="https://fonts.googleapis.com/css?family=Questrial"]')) .to.be.ok; expect(doc.querySelector('script[src*="amp-font-0.1"]')).to.be.ok; + expect(onAmpCreativeRenderFired).to.be.true; }); }); }); @@ -421,7 +439,7 @@ describe('amp-a4a', () => { }); }); it('#layoutCallback not valid AMP', () => { - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -434,6 +452,7 @@ describe('amp-a4a', () => { a4aElement.setAttribute('height', 50); a4aElement.setAttribute('type', 'adsense'); const a4a = new MockA4AImpl(a4aElement); + verifyNonAMPRender(a4a); const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl'); sandbox.stub(a4a, 'extractCreativeAndSignature').returns( Promise.resolve({creative: mockResponse.arrayBuffer()})); @@ -455,7 +474,7 @@ describe('amp-a4a', () => { const iframe = a4aElement.getElementsByTagName('iframe')[0]; expect(iframe.getAttribute('srcdoc')).to.be.null; expect(iframe.src, 'verify iframe src w/ origin').to - .equal('https://test.location.org/ad/012345?args' + + .equal(XHR_URL + '&__amp_source_origin=about%3Asrcdoc'); expect(a4a.rendered_).to.be.true; }); @@ -463,7 +482,7 @@ describe('amp-a4a', () => { }); }); it('should not leak full response to rendered dom', () => { - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', @@ -506,6 +525,10 @@ describe('amp-a4a', () => { signature: base64UrlDecodeToBytes(validCSSAmp.signature), })); a4a.onLayoutMeasure(); + let onAmpCreativeRenderFired = false; + a4a.onAmpCreativeRender = () => { + onAmpCreativeRenderFired = true; + }; expect(a4a.adPromise_).to.be.instanceof(Promise); return a4a.adPromise_.then(() => { const friendlyIframe = a4aElement.getElementsByTagName('iframe')[0]; @@ -518,6 +541,7 @@ describe('amp-a4a', () => { expect(frameDoc.querySelector('style[amp-custom]')).to.be.ok; expect(frameDoc.body.innerHTML, 'body content') .to.contain('Hello, world.'); + expect(onAmpCreativeRenderFired).to.be.true; }); }); }); @@ -531,6 +555,7 @@ describe('amp-a4a', () => { a4aElement.setAttribute('type', 'adsense'); const a4a = new MockA4AImpl(a4aElement); const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl'); + verifyNonAMPRender(a4a); a4a.onLayoutMeasure(); expect(a4a.adPromise_).to.be.instanceof(Promise); return a4a.layoutCallback().then(() => { @@ -541,8 +566,8 @@ describe('amp-a4a', () => { expect(a4aElement.children.length).to.equal(1); const iframe = a4aElement.querySelector('iframe[src]'); expect(iframe).to.be.ok; - expect(iframe.src.indexOf('https://test.location.org')).to.equal(0); - expect(iframe.style.visibility).to.equal(''); + expect(iframe.src.indexOf(XHR_URL)).to.equal(0); + expect(isStyleVisible(fixture.win, iframe)).to.be.true; }); }); }); @@ -552,6 +577,7 @@ describe('amp-a4a', () => { const doc = fixture.doc; const a4aElement = createA4aElement(doc); const a4a = new MockA4AImpl(a4aElement); + verifyNonAMPRender(a4a); a4a.onLayoutMeasure(); return a4a.adPromise_.then(() => a4a.layoutCallback().then(() => { a4a.vsync_.runScheduledTasks_(); @@ -559,8 +585,8 @@ describe('amp-a4a', () => { expect(a4aElement.children.length).to.equal(1); const iframe = a4aElement.children[0]; expect(iframe.tagName).to.equal('IFRAME'); - expect(iframe.src.indexOf('https://test.location.org')).to.equal(0); - expect(iframe.style.visibility).to.equal(''); + expect(iframe.src.indexOf(XHR_URL)).to.equal(0); + expect(isStyleVisible(fixture.win, iframe)).to.be.true; })); }); }); @@ -573,6 +599,7 @@ describe('amp-a4a', () => { const doc = fixture.doc; const a4aElement = createA4aElement(doc); const a4a = new MockA4AImpl(a4aElement); + verifyNonAMPRender(a4a); a4a.onLayoutMeasure(); const layoutCallbackPromise = a4a.layoutCallback(); rejectXhr(new Error('XHR Error')); @@ -582,7 +609,7 @@ describe('amp-a4a', () => { expect(a4aElement.children.length).to.equal(1); const iframe = a4aElement.children[0]; expect(iframe.tagName).to.equal('IFRAME'); - expect(iframe.src.indexOf('https://test.location.org')).to.equal(0); + expect(iframe.src.indexOf(XHR_URL)).to.equal(0); expect(iframe.style.visibility).to.equal(''); }); }); @@ -598,11 +625,14 @@ describe('amp-a4a', () => { const a4aElement = createA4aElement(doc); a4aElement.setAttribute('type', 'adsense'); const a4a = new AmpA4A(a4aElement); + //a4a.config = {}; a4a.buildCallback(); a4a.preconnectCallback(false); const preconnects = doc.querySelectorAll('link[rel=preconnect]'); - expect(preconnects.length).to.not.equal(0); + expect(preconnects.length).to.equal(2); expect(preconnects[0].getAttribute('href')).to + .equal('https://tpc.googlesyndication.com'); + expect(preconnects[1].getAttribute('href')).to .equal('https://googleads.g.doubleclick.net'); }); }); @@ -843,7 +873,7 @@ describe('amp-a4a', () => { a4aElement.setAttribute('type', 'adsense'); doc.body.appendChild(a4aElement); const a4a = new MockA4AImpl(a4aElement); - xhrMock.withArgs('https://test.location.org/ad/012345?args', { + xhrMock.withArgs(XHR_URL, { mode: 'cors', method: 'GET', credentials: 'include', diff --git a/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js b/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js index d0ce59351419..93598c85acc6 100644 --- a/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js +++ b/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, @@ -55,9 +54,6 @@ export class AmpAdXOriginIframeHandler { /** @private {SubscriptionApi} */ this.embedStateApi_ = null; - /** @private {boolean} */ - this.is3p_ = false; - /** @private {!Array} functions to unregister listeners */ this.unlisteners_ = []; @@ -71,28 +67,24 @@ export class AmpAdXOriginIframeHandler { /** * Sets up listeners and iframe state for iframe containing ad creative. * @param {!Element} iframe - * @param {boolean} is3p whether iframe was loaded via 3p. - * @param {boolean=} opt_defaultVisible when true, visibility hidden is NOT - * set on the iframe element (remains visible * @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, is3p, opt_defaultVisible, opt_isA4A) { + init(iframe, opt_isA4A) { dev().assert( !this.iframe, 'multiple invocations of init without destroy!'); this.iframe = iframe; - this.is3p_ = is3p; this.iframe.setAttribute('scrolling', 'no'); this.baseInstance_.applyFillContent(this.iframe); this.intersectionObserver_ = new IntersectionObserver( - this.baseInstance_, this.iframe, is3p); + this.baseInstance_, this.iframe, true); this.embedStateApi_ = new SubscriptionApi( - this.iframe, 'send-embed-state', is3p, + this.iframe, 'send-embed-state', true, () => this.sendEmbedInfo_(this.baseInstance_.isInViewport())); // Triggered by context.reportRenderedEntityIdentifier(…) inside the ad // iframe. - listenForOncePromise(this.iframe, 'entity-id', this.is3p_) + listenForOncePromise(this.iframe, 'entity-id', true) .then(info => { this.element_.creativeId = info.data.id; }); @@ -101,16 +93,25 @@ export class AmpAdXOriginIframeHandler { this.unlisteners_.push(listenFor(this.iframe, 'embed-size', (data, source, origin) => { this.updateSize_(data.height, data.width, source, origin); - }, this.is3p_, this.is3p_)); + }, true, true)); + + this.unlisteners_.push(this.viewer_.onVisibilityChanged(() => { + this.sendEmbedInfo_(this.baseInstance_.isInViewport()); + })); - // Install API that listen to ad response 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, - ['render-start', 'no-content'], this.is3p_).then(info => { + ['render-start', 'no-content'], true).then(info => { const data = info.data; if (data.type == 'render-start') { this.renderStart_(info); @@ -122,34 +123,26 @@ export class AmpAdXOriginIframeHandler { // If NOT support render-start, listen to bootstrap-loaded no-content // respectively this.adResponsePromise_ = listenForOncePromise(this.iframe, - 'bootstrap-loaded', this.is3p_); - listenForOncePromise(this.iframe, 'no-content', this.is3p_) + 'bootstrap-loaded', true); + listenForOncePromise(this.iframe, 'no-content', true) .then(() => this.noContent_()); } - if (!opt_defaultVisible) { - // NOTE(tdrl,keithwrightbos): This will not work for A4A with an AMP - // creative as it will not expect having to send the render-start message. - this.iframe.style.visibility = 'hidden'; - } - - this.unlisteners_.push(this.viewer_.onVisibilityChanged(() => { - this.sendEmbedInfo_(this.baseInstance_.isInViewport()); - })); + // Set iframe initially hidden which will be removed on load event + + // post message depending on if A4A flow. + this.iframe.style.visibility = 'hidden'; 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 = ''; + } + }); } /** @@ -269,7 +262,7 @@ export class AmpAdXOriginIframeHandler { [{win: source, origin}], success ? 'embed-size-changed' : 'embed-size-denied', {requestedWidth, requestedHeight}, - this.is3p_); + true); } /** diff --git a/extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js b/extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js index 957ce098f058..313da261fbc9 100644 --- a/extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js +++ b/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); @@ -71,10 +71,7 @@ describe('amp-ad-xorigin-iframe-handler', () => { noContentSpy = sandbox.spy/*OK*/(iframeHandler, 'freeXOriginIframe'); - initPromise = iframeHandler.init(iframe, true); - iframe.onload = () => { - done(); - }; + initPromise = iframeHandler.init(iframe); }); it('should resolve on message "render-start"', () => { @@ -170,7 +167,7 @@ describe('amp-ad-xorigin-iframe-handler', () => { it('should resolve on message "bootstrap-loaded" if render-start is' + 'NOT implemented', done => { - initPromise = iframeHandler.init(iframe, true); + initPromise = iframeHandler.init(iframe); iframe.onload = () => { expect(iframe.style.visibility).to.equal('hidden'); iframe.postMessageToParent({ @@ -190,22 +187,20 @@ describe('amp-ad-xorigin-iframe-handler', () => { const clock = sandbox.useFakeTimers(); iframe.name = 'test_master'; - initPromise = iframeHandler.init(iframe, true); - 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(); - }); - }; + initPromise = iframeHandler.init(iframe); + 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', () => { - return iframeHandler.init(iframe, true, undefined, true).then(() => { + return iframeHandler.init(iframe, true).then(() => { expect(iframe.style.visibility).to.equal(''); }); }); @@ -213,11 +208,8 @@ describe('amp-ad-xorigin-iframe-handler', () => { describe('Initialized iframe', () => { - beforeEach(done => { - iframeHandler.init(iframe, true); - iframe.onload = () => { - done(); - }; + beforeEach(() => { + iframeHandler.init(iframe); }); it('should be able to use embed-state API', () => { From 7cff6bcdb0ae4b2fe94f2a32d18a5be55aac4d9b Mon Sep 17 00:00:00 2001 From: keithwrightbos Date: Tue, 1 Nov 2016 11:13:04 -0400 Subject: [PATCH 4/5] Correct conflict --- extensions/amp-a4a/0.1/amp-a4a.js | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index c1fb5767cd01..075a966d21bf 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -510,7 +510,7 @@ export class AmpA4A extends AMP.BaseElement { // valid AMP. return this.adPromise_.then(rendered => { if (rendered instanceof Error || !rendered) { - this.emitLifecycleEvent('preAdThrottle'); + this.emitLifecycleEvent('preAdThrottle'); incrementLoadingAds(this.win); // Haven't rendered yet, so try rendering via one of our // cross-domain iframe solutions. @@ -707,11 +707,7 @@ export class AmpA4A extends AMP.BaseElement { * @private */ maybeRenderAmpAd_(bytes) { -<<<<<<< HEAD - this.lifecycleReporter.sendPing('renderFriendlyStart'); -======= this.emitLifecycleEvent('renderFriendlyStart', bytes); ->>>>>>> e5501a30adf15c8fef049729f5e0e3137dbb18ca // AMP documents are required to be UTF-8 return utf8Decode(bytes).then(creative => { // Find the json blob located at the end of the body and parse it. @@ -794,18 +790,7 @@ export class AmpA4A extends AMP.BaseElement { // TODO(keithwrightbos): noContentCallback? this.xOriginIframeHandler_ = new AMP.AmpAdXOriginIframeHandler(this); this.rendered_ = true; -<<<<<<< HEAD return this.xOriginIframeHandler_.init(iframe, /* opt_isA4A */ 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; ->>>>>>> e5501a30adf15c8fef049729f5e0e3137dbb18ca } /** @@ -824,11 +809,7 @@ export class AmpA4A extends AMP.BaseElement { * @private */ renderViaCachedContentIframe_(adUrl) { -<<<<<<< HEAD - this.lifecycleReporter.sendPing('renderCrossDomainStart'); -======= this.emitLifecycleEvent('renderCrossDomainStart'); ->>>>>>> e5501a30adf15c8fef049729f5e0e3137dbb18ca /** @const {!Element} */ const iframe = createElementWithAttributes( /** @type {!Document} */(this.element.ownerDocument), @@ -851,11 +832,7 @@ export class AmpA4A extends AMP.BaseElement { * @private */ renderViaSafeFrame_(creativeBody) { -<<<<<<< HEAD - this.lifecycleReporter.sendPing('renderSafeFrameStart'); -======= this.emitLifecycleEvent('renderSafeFrameStart'); ->>>>>>> e5501a30adf15c8fef049729f5e0e3137dbb18ca return utf8Decode(creativeBody).then(creative => { /** @const {!Element} */ const iframe = createElementWithAttributes( From aaca0d938d4dc3c420cff507679d8388c5999bdd Mon Sep 17 00:00:00 2001 From: keithwrightbos Date: Tue, 1 Nov 2016 14:38:45 -0400 Subject: [PATCH 5/5] PR feedback --- extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js b/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js index 93598c85acc6..d7a800a8f9a1 100644 --- a/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js +++ b/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js @@ -129,7 +129,7 @@ export class AmpAdXOriginIframeHandler { } // Set iframe initially hidden which will be removed on load event + - // post message depending on if A4A flow. + // post message. this.iframe.style.visibility = 'hidden'; this.element_.appendChild(this.iframe);