diff --git a/ads/google/a4a/test/test-utils.js b/ads/google/a4a/test/test-utils.js index 1fa232e3d6f9..4fe4b933b541 100644 --- a/ads/google/a4a/test/test-utils.js +++ b/ads/google/a4a/test/test-utils.js @@ -82,7 +82,6 @@ describe('Google A4A utils', () => { .to.eventually.deep.equal({ creative, signature: base64UrlDecodeToBytes('AQAB'), - size: null, }); }); @@ -90,7 +89,6 @@ describe('Google A4A utils', () => { const creative = 'some test data'; const headerData = { 'X-AmpAdSignature': 'AQAB', - 'X-CreativeSize': '320x50', }; const headers = { has: h => { return h in headerData; }, @@ -100,7 +98,6 @@ describe('Google A4A utils', () => { .to.eventually.deep.equal({ creative, signature: base64UrlDecodeToBytes('AQAB'), - size: {width: 320, height: 50}, }); }); @@ -111,11 +108,7 @@ describe('Google A4A utils', () => { get: h => { throw new Error('Tried to get ' + h); }, }; return expect(extractGoogleAdCreativeAndSignature(creative, headers)) - .to.eventually.deep.equal({ - creative, - signature: null, - size: null, - }); + .to.eventually.deep.equal({creative, signature: null}); }); }); diff --git a/ads/google/a4a/utils.js b/ads/google/a4a/utils.js index 1f838b468d6e..6afcdaf20098 100644 --- a/ads/google/a4a/utils.js +++ b/ads/google/a4a/utils.js @@ -39,9 +39,6 @@ import { /** @const {string} */ const AMP_SIGNATURE_HEADER = 'X-AmpAdSignature'; -/** @const {string} */ -const CREATIVE_SIZE_HEADER = 'X-CreativeSize'; - /** @type {string} */ const AMP_ANALYTICS_HEADER = 'X-AmpAnalytics'; @@ -284,25 +281,16 @@ export function truncAndTimeUrl(baseUrl, parameters, startTime) { export function extractGoogleAdCreativeAndSignature( creative, responseHeaders) { let signature = null; - let size = null; try { if (responseHeaders.has(AMP_SIGNATURE_HEADER)) { signature = base64UrlDecodeToBytes(dev().assertString( responseHeaders.get(AMP_SIGNATURE_HEADER))); } - if (responseHeaders.has(CREATIVE_SIZE_HEADER)) { - const sizeHeader = responseHeaders.get(CREATIVE_SIZE_HEADER); - dev().assert(new RegExp('[0-9]+x[0-9]+').test(sizeHeader)); - const sizeArr = sizeHeader - .split('x') - .map(dim => Number(dim)); - size = {width: sizeArr[0], height: sizeArr[1]}; - } } finally { return Promise.resolve(/** @type { !../../../extensions/amp-a4a/0.1/amp-a4a.AdResponseDef} */ ( - {creative, signature, size})); + {creative, signature})); } } diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index dc64aa23c697..e26fb9e0c4ae 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -78,6 +78,9 @@ const METADATA_STRING_NO_QUOTES = /** @type {string} */ export const DEFAULT_SAFEFRAME_VERSION = '1-0-9'; +/** @const {string} */ +const CREATIVE_SIZE_HEADER = 'X-CreativeSize'; + /** @type {string} @visibleForTesting */ export const RENDERING_TYPE_HEADER = 'X-AmpAdRender'; @@ -110,10 +113,12 @@ const SHARED_IFRAME_PROPERTIES = dict({ /** @typedef {{ * creative: ArrayBuffer, * signature: ?Uint8Array, - * size: ?{width: number, height: number} * }} */ export let AdResponseDef; +/** @typedef {{width: number, height: number}} */ +export let SizeInfoDef; + /** @typedef {{ minifiedCreative: string, customElementExtensions: !Array, @@ -646,23 +651,26 @@ export class AmpA4A extends AMP.BaseElement { }; }); }) - // This block returns the ad creative and signature, if available; null - // otherwise. + // This block returns the ad creative, signature, and size, if + // available; null otherwise. /** - * @return {!Promise} + * @return {?Promise<{creativeParts: !AdResponseDef, size: ?SizeInfoDef}>} */ .then(responseParts => { checkStillCurrent(); - if (responseParts) { - this.protectedEmitLifecycleEvent_('extractCreativeAndSignature'); + if (!responseParts) { + return null; } - return responseParts && this.extractCreativeAndSignature( - responseParts.bytes, responseParts.headers); + this.protectedEmitLifecycleEvent_('extractCreativeAndSignature'); + const size = this.extractSize(responseParts.headers); + return this.extractCreativeAndSignature( + responseParts.bytes, responseParts.headers) + .then(creativeParts => ({creativeParts, size})); }) // This block returns the ad creative if it exists and validates as AMP; // null otherwise. /** @return {!Promise} */ - .then(creativeParts => { + .then(creativeData => { checkStillCurrent(); // Keep a handle to the creative body so that we can render into // SafeFrame or NameFrame later, if necessary. TODO(tdrl): Temporary, @@ -671,10 +679,11 @@ export class AmpA4A extends AMP.BaseElement { // src cache issue. If we decide to keep a SafeFrame-like solution, // we should restructure the promise chain to pass this info along // more cleanly, without use of an object variable outside the chain. - if (!creativeParts) { + if (!creativeData) { return Promise.resolve(); } - this.creativeSize_ = creativeParts.size || this.creativeSize_; + const {creativeParts, size} = creativeData; + this.creativeSize_ = size || this.creativeSize_; if (this.experimentalNonAmpCreativeRenderMethod_ != XORIGIN_MODE.CLIENT_CACHE && creativeParts.creative) { @@ -1076,6 +1085,29 @@ export class AmpA4A extends AMP.BaseElement { throw new Error('extractCreativeAndSignature not implemented!'); } + /** + * Determine the desired size of the creative based on the HTTP response + * headers. Must be less than or equal to the original size of the ad slot + * along each dimension. May be overridden by network. + * + * @param {!../../../src/service/xhr-impl.FetchResponseHeaders} responseHeaders + * @return {?SizeInfoDef} + */ + extractSize(responseHeaders) { + const headerValue = responseHeaders.get(CREATIVE_SIZE_HEADER); + if (!headerValue) { + return null; + } + const match = /^([0-9]+)x([0-9]+)$/.exec(headerValue); + if (!match) { + // TODO(@taymonbeal, #9274): replace this with real error reporting + user().error(TAG, `Invalid size header: ${headerValue}`); + return null; + } + return /** @type {?SizeInfoDef} */ ( + {width: Number(match[1]), height: Number(match[2])}); + } + /** * Forces the UI Handler to collapse this slot. * @visibleForTesting 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 4fc307f7c404..269475150343 100644 --- a/extensions/amp-a4a/0.1/test/test-amp-a4a.js +++ b/extensions/amp-a4a/0.1/test/test-amp-a4a.js @@ -2159,6 +2159,24 @@ describe('amp-a4a', () => { }); }); + describe('#extractSize', () => { + + it('should return a size', () => { + expect(AmpA4A.prototype.extractSize({ + get(name) { + return {'X-CreativeSize': '320x50'}[name]; + }, + })).to.deep.equal({width: 320, height: 50}); + }); + + it('should return no size', () => { + expect(AmpA4A.prototype.extractSize({ + get(unusedName) { + return null; + }, + })).to.be.null; + }); + }); // TODO(tdrl): Other cases to handle for parsing JSON metadata: // - Metadata tag(s) missing // - JSON parse failure diff --git a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js index 9d973f896c88..c4b04e16c3f8 100644 --- a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js +++ b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js @@ -206,11 +206,12 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { // Load amp-analytics extensions this.extensions_./*OK*/loadExtension('amp-analytics'); } - return extractGoogleAdCreativeAndSignature(responseText, responseHeaders) - .then(adResponse => { - adResponse.size = this.size_; - return Promise.resolve(adResponse); - }); + return extractGoogleAdCreativeAndSignature(responseText, responseHeaders); + } + + /** @override */ + extractSize(unusedResponseHeaders) { + return this.size_; } /** diff --git a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js index 891e52e75358..07ddf7fa61bc 100644 --- a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js +++ b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js @@ -337,7 +337,7 @@ describes.sandboxed('amp-ad-network-adsense-impl', {}, () => { has() { return false; }, }).then(adResponse => { expect(adResponse).to.deep.equal( - {creative, signature: null, size: null}); + {creative, signature: null}); expect(loadExtensionSpy.withArgs('amp-analytics')).to.not.be .called; }); @@ -356,8 +356,7 @@ describes.sandboxed('amp-ad-network-adsense-impl', {}, () => { }, }).then(adResponse => { expect(adResponse).to.deep.equal( - {creative, signature: base64UrlDecodeToBytes('AQAB'), - size: null}); + {creative, signature: base64UrlDecodeToBytes('AQAB')}); expect(loadExtensionSpy.withArgs('amp-analytics')).to.not.be .called; }); @@ -387,7 +386,6 @@ describes.sandboxed('amp-ad-network-adsense-impl', {}, () => { { creative, signature: base64UrlDecodeToBytes('AQAB'), - size: null, }); expect(loadExtensionSpy.withArgs('amp-analytics')).to.be.called; // exact value of ampAnalyticsConfig_ covered in diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js index a300849a755c..a7d575e8c1fc 100644 --- a/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js +++ b/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js @@ -342,19 +342,21 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A { // Load amp-analytics extensions this.extensions_./*OK*/loadExtension('amp-analytics'); } - const adResponsePromise = - extractGoogleAdCreativeAndSignature(responseText, responseHeaders); - return adResponsePromise.then(adResponse => { - // If the server returned a size, use that, otherwise use the size that - // we sent in the ad request. - if (adResponse.size) { - this.size_ = adResponse.size; - } else { - adResponse.size = this.size_; - } - this.handleResize_(adResponse.size.width, adResponse.size.height); - return Promise.resolve(adResponse); - }); + return extractGoogleAdCreativeAndSignature(responseText, responseHeaders); + } + + /** @override */ + extractSize(responseHeaders) { + // If the server returned a size, use that, otherwise use the size that we + // sent in the ad request. + let size = super.extractSize(responseHeaders); + if (size) { + this.size_ = size; + } else { + size = this.size_; + } + this.handleResize_(size.width, size.height); + return size; } /** @override */ diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js index 775eb38aef7c..8b2c61c4282c 100644 --- a/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js +++ b/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js @@ -151,68 +151,74 @@ describes.sandboxed('amp-ad-network-doubleclick-impl', {}, () => { }); it('without signature', () => { + const headers = { + get() { + return undefined; + }, + has() { + return false; + }, + }; return utf8Encode('some creative').then(creative => { - return impl.extractCreativeAndSignature( - creative, - { - get() { return undefined; }, - has() { return false; }, - }).then(adResponse => { - expect(adResponse).to.deep.equal( - {creative, signature: null, size}); - expect(loadExtensionSpy.withArgs('amp-analytics')).to.not.be - .called; + return impl.extractCreativeAndSignature(creative, headers) + .then(adResponse => { + expect(adResponse).to.deep.equal({creative, signature: null}); + expect(impl.extractSize(headers)).to.deep.equal(size); + expect(loadExtensionSpy.withArgs('amp-analytics')) + .to.not.be.called; }); }); }); it('with signature', () => { return utf8Encode('some creative').then(creative => { - return impl.extractCreativeAndSignature( - creative, - { - get(name) { - return name == 'X-AmpAdSignature' ? 'AQAB' : undefined; - }, - has(name) { - return name === 'X-AmpAdSignature'; - }, - }).then(adResponse => { - expect(adResponse).to.deep.equal( - {creative, signature: base64UrlDecodeToBytes('AQAB'), size}); - expect(loadExtensionSpy.withArgs('amp-analytics')).to.not.be - .called; + const headers = { + get(name) { + return name == 'X-AmpAdSignature' ? 'AQAB' : undefined; + }, + has(name) { + return name === 'X-AmpAdSignature'; + }, + }; + return impl.extractCreativeAndSignature(creative, headers) + .then(adResponse => { + expect(adResponse).to.deep.equal({ + creative, + signature: base64UrlDecodeToBytes('AQAB'), + }); + expect(impl.extractSize(headers)).to.deep.equal(size); + expect(loadExtensionSpy.withArgs('amp-analytics')) + .to.not.be.called; }); }); }); it('with analytics', () => { return utf8Encode('some creative').then(creative => { const url = ['https://foo.com?a=b', 'https://blah.com?lsk=sdk&sld=vj']; - return impl.extractCreativeAndSignature( - creative, - { - get(name) { - switch (name) { - case 'X-AmpAnalytics': - return JSON.stringify({url}); - case 'X-AmpAdSignature': - return 'AQAB'; - default: - return undefined; - } - }, - has(name) { - return !!this.get(name); - }, - }).then(adResponse => { - expect(adResponse).to.deep.equal( - { - creative, - signature: base64UrlDecodeToBytes('AQAB'), - size, - }); + const headers = { + get(name) { + switch (name) { + case 'X-AmpAnalytics': + return JSON.stringify({url}); + case 'X-AmpAdSignature': + return 'AQAB'; + default: + return undefined; + } + }, + has(name) { + return !!this.get(name); + }, + }; + return impl.extractCreativeAndSignature(creative, headers) + .then(adResponse => { + expect(adResponse).to.deep.equal({ + creative, + signature: base64UrlDecodeToBytes('AQAB'), + }); + expect(impl.extractSize(headers)).to.deep.equal(size); expect(loadExtensionSpy.withArgs('amp-analytics')).to.be.called; - // exact value of ampAnalyticsConfig covered in - // ads/google/test/test-utils.js + // exact value of ampAnalyticsConfig covered in + // ads/google/test/test-utils.js }); }); });