Move extractSize to a separate hook

ads/google/a4a/test/test-utils.js

@@ -82,15 +82,13 @@ describe('Google A4A utils', () => {
           .to.eventually.deep.equal({
             creative,
             signature: base64UrlDecodeToBytes('AQAB'),
-            size: null,
           });
     });
 
     it('should return body and signature and size', () => {
       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});
     });
   });
 

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}));
   }
 }
 

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<string>,
@@ -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<?{AdResponseDef}>}
+         * @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<?ArrayBuffer>} */
-        .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

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

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_;
   }
 
   /**

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

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 */