From 769f71b2e24307081433692fb479ff9815a26761 Mon Sep 17 00:00:00 2001
From: keithwrightbos <kjwright@google.com>
Date: Fri, 22 Sep 2017 13:48:38 -0400
Subject: [PATCH 1/3] Amp Ad Fast Fetch allow amp-image prefetch

---
 extensions/amp-a4a/0.1/amp-a4a.js           |  13 +-
 extensions/amp-a4a/0.1/test/test-amp-a4a.js | 129 ++++++++++++--------
 2 files changed, 85 insertions(+), 57 deletions(-)

diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
index 04ab5f1205ee..af75f5465c66 100644
--- a/extensions/amp-a4a/0.1/amp-a4a.js
+++ b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -40,7 +40,7 @@ import {isArray, isObject, isEnumValue} from '../../../src/types';
 import {utf8Decode} from '../../../src/utils/bytes';
 import {getBinaryType, isExperimentOn} from '../../../src/experiments';
 import {setStyle} from '../../../src/style';
-import {assertHttpsUrl} from '../../../src/url';
+import {assertHttpsUrl, isSecureUrl} from '../../../src/url';
 import {parseJson} from '../../../src/json';
 import {handleClick} from '../../../ads/alp/handler';
 import {
@@ -106,7 +106,8 @@ export let SizeInfoDef;
 /** @typedef {{
       minifiedCreative: string,
       customElementExtensions: !Array<string>,
-      customStylesheets: !Array<{href: string}>
+      customStylesheets: !Array<{href: string}>,
+      ampImages: Array<string>|undefined,
     }} */
 let CreativeMetaDataDef;
 
@@ -738,6 +739,9 @@ export class AmpA4A extends AMP.BaseElement {
           // Preload any fonts.
           (creativeMetaDataDef.customStylesheets || []).forEach(font =>
               this.preconnect.preload(font.href));
+          // Preload any AMP images.
+          (creativeMetaDataDef.ampImages || []).forEach(image =>
+              isSecureUrl(image) && this.preconnect.preload(image));
           return creativeMetaDataDef;
         })
         .catch(error => {
@@ -1475,11 +1479,14 @@ export class AmpA4A extends AMP.BaseElement {
         metaData.customStylesheets.forEach(stylesheet => {
           if (!isObject(stylesheet) || !stylesheet['href'] ||
               typeof stylesheet['href'] !== 'string' ||
-              !/^https:\/\//i.test(stylesheet['href'])) {
+              !isSecureUrl(stylesheet['href'])) {
             throw new Error(errorMsg);
           }
         });
       }
+      if (isArray(metaDataObj['ampImages'])) {
+        metaData.ampImages = metaDataObj['ampImages'];
+      }
       // TODO(keithwrightbos): OK to assume ampRuntimeUtf16CharOffsets is before
       // metadata as its in the head?
       metaData.minifiedCreative =
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 201f8ea17479..f63a865e8aa1 100644
--- a/extensions/amp-a4a/0.1/test/test-amp-a4a.js
+++ b/extensions/amp-a4a/0.1/test/test-amp-a4a.js
@@ -152,7 +152,7 @@ describe('amp-a4a', () => {
 
   function buildCreativeString(opt_additionalInfo) {
     const baseTestDoc = testFragments.minimalDocOneStyle;
-    const offsets = opt_additionalInfo || {};
+    const offsets = Object.assign({}, opt_additionalInfo || {});
     offsets.ampRuntimeUtf16CharOffsets = [
       baseTestDoc.indexOf('<style amp4ads-boilerplate'),
       baseTestDoc.lastIndexOf('</script>') + '</script>'.length,
@@ -941,6 +941,35 @@ describe('amp-a4a', () => {
         });
       });
     });
+    // TODO (keithwrightbos) - move into above e2e once signed creative with
+    // ampImage within creative can be regenerated.
+    it('should prefetch amp images', () => {
+      return createIframePromise().then(fixture => {
+        setupForAdTesting(fixture);
+        fetchMock.getOnce(
+            TEST_URL + '&__amp_source_origin=about%3Asrcdoc', () => adResponse,
+            {name: 'ad'});
+        const doc = fixture.doc;
+        const a4aElement = createA4aElement(doc);
+        const a4a = new MockA4AImpl(a4aElement);
+        sandbox.stub(a4a, 'getAmpAdMetadata_', creative => {
+          const metaData = AmpA4A.prototype.getAmpAdMetadata_(creative);
+          metaData.ampImages = ['https://prefetch.me.com?a=b', 'http://do.not.prefetch.me.com?c=d',
+            'https://prefetch.metoo.com?e=f'];
+          return metaData;
+        });
+        a4a.buildCallback();
+        a4a.onLayoutMeasure();
+        return a4a.layoutCallback().then(() => {
+          expect(doc.querySelector('link[rel=preload]' +
+            '[href="https://prefetch.me.com?a=b"]')).to.be.ok;
+          expect(doc.querySelector('link[rel=preload]' +
+            '[href="https://prefetch.metoo.com?e=f"]')).to.be.ok;
+          expect(doc.querySelector('link[rel=preload]' +
+            '[href="http://do.not.prefetch.me.com?c=d"]')).to.not.be.ok;
+        });
+      });
+    });
     it('must not be position:fixed', () => {
       return createIframePromise().then(fixture => {
         setupForAdTesting(fixture);
@@ -1417,7 +1446,16 @@ describe('amp-a4a', () => {
 
   describe('#getAmpAdMetadata_', () => {
     let a4a;
+    let metaData;
     beforeEach(() => {
+      metaData = {
+        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
+        customStylesheets: [
+          {href: 'https://fonts.googleapis.com/css?foobar'},
+          {href: 'https://fonts.com/css?helloworld'},
+        ],
+        ampImages: ['https://some.image.com/a=b', 'https://other.image.com'],
+      };
       return createIframePromise().then(fixture => {
         setupForAdTesting(fixture);
         a4a = new MockA4AImpl(createA4aElement(fixture.doc));
@@ -1425,53 +1463,27 @@ describe('amp-a4a', () => {
       });
     });
     it('should parse metadata', () => {
-      const actual = a4a.getAmpAdMetadata_(buildCreativeString({
-        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
-        customStylesheets: [
-          {href: 'https://fonts.googleapis.com/css?foobar'},
-          {href: 'https://fonts.com/css?helloworld'},
-        ],
-      }));
-      const expected = {
+      const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
+      const expected = Object.assign(metaData, {
         minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
-        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
-        customStylesheets: [
-          {href: 'https://fonts.googleapis.com/css?foobar'},
-          {href: 'https://fonts.com/css?helloworld'},
-        ],
-      };
+      });
       expect(actual).to.deep.equal(expected);
     });
     // TODO(levitzky) remove the following two tests after metadata bug is
     // fixed.
     it('should parse metadata with wrong opening tag', () => {
-      const creative = buildCreativeString({
-        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
-        customStylesheets: [
-          {href: 'https://fonts.googleapis.com/css?foobar'},
-          {href: 'https://fonts.com/css?helloworld'},
-        ],
-      }).replace('<script type="application/json" amp-ad-metadata>',
+      const creative = buildCreativeString(metaData).replace(
+          '<script type="application/json" amp-ad-metadata>',
           '<script type=application/json amp-ad-metadata>');
       const actual = a4a.getAmpAdMetadata_(creative);
-      const expected = {
+      const expected = Object.assign({
         minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
-        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
-        customStylesheets: [
-          {href: 'https://fonts.googleapis.com/css?foobar'},
-          {href: 'https://fonts.com/css?helloworld'},
-        ],
-      };
+      }, metaData);
       expect(actual).to.deep.equal(expected);
     });
     it('should return null if metadata opening tag is (truly) wrong', () => {
-      const creative = buildCreativeString({
-        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
-        customStylesheets: [
-          {href: 'https://fonts.googleapis.com/css?foobar'},
-          {href: 'https://fonts.com/css?helloworld'},
-        ],
-      }).replace('<script type="application/json" amp-ad-metadata>',
+      const creative = buildCreativeString(metaData).replace(
+          '<script type="application/json" amp-ad-metadata>',
           '<script type=application/json" amp-ad-metadata>');
       expect(a4a.getAmpAdMetadata_(creative)).to.be.null;
     });
@@ -1485,28 +1497,37 @@ describe('amp-a4a', () => {
         baseTestDoc.slice(splicePoint))).to.be.null;
     });
     it('should return null if invalid extensions', () => {
-      expect(a4a.getAmpAdMetadata_(buildCreativeString({
-        customElementExtensions: 'amp-vine',
-        customStylesheets: [
-          {href: 'https://fonts.googleapis.com/css?foobar'},
-          {href: 'https://fonts.com/css?helloworld'},
-        ],
-      }))).to.be.null;
+      metaData.customElementExtensions = 'amp-vine';
+      expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData))).to.be.null;
     });
     it('should return null if non-array stylesheets', () => {
-      expect(a4a.getAmpAdMetadata_(buildCreativeString({
-        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
-        customStylesheets: 'https://fonts.googleapis.com/css?foobar',
-      }))).to.be.null;
+      metaData.customStylesheets = 'https://fonts.googleapis.com/css?foobar';
+      expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData))).to.be.null;
     });
     it('should return null if invalid stylesheet object', () => {
-      expect(a4a.getAmpAdMetadata_(buildCreativeString({
-        customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
-        customStylesheets: [
-          {href: 'https://fonts.googleapis.com/css?foobar'},
-          {foo: 'https://fonts.com/css?helloworld'},
-        ],
-      }))).to.be.null;
+      metaData.customStylesheets = [
+        {href: 'https://fonts.googleapis.com/css?foobar'},
+        {foo: 'https://fonts.com/css?helloworld'},
+      ];
+      expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData))).to.be.null;
+    });
+    it('should not include amp images if not an array', () => {
+      metaData.ampImages = 'https://foo.com';
+      const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
+      const expected = Object.assign({
+        minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
+      }, metaData);
+      delete expected.ampImages;
+      expect(actual).to.deep.equal(expected);
+    });
+    it('should tolerate missing ampImages', () => {
+      delete metaData.ampImages;
+      const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
+      const expected = Object.assign({
+        minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
+      }, metaData);
+      delete expected.ampImages;
+      expect(actual).to.deep.equal(expected);
     });
     // FAILURE cases here
   });

From 4da82342722529a36f297d0acc09a1b4c3b39520 Mon Sep 17 00:00:00 2001
From: keithwrightbos <kjwright@google.com>
Date: Fri, 22 Sep 2017 14:04:30 -0400
Subject: [PATCH 2/3] type check fix

---
 extensions/amp-a4a/0.1/amp-a4a.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
index af75f5465c66..0a54f2eaf415 100644
--- a/extensions/amp-a4a/0.1/amp-a4a.js
+++ b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -107,7 +107,7 @@ export let SizeInfoDef;
       minifiedCreative: string,
       customElementExtensions: !Array<string>,
       customStylesheets: !Array<{href: string}>,
-      ampImages: Array<string>|undefined,
+      ampImages: (Array<string>|undefined),
     }} */
 let CreativeMetaDataDef;
 

From d6f791604240b415b3de50f3f3c04b34358a585b Mon Sep 17 00:00:00 2001
From: keithwrightbos <kjwright@google.com>
Date: Mon, 25 Sep 2017 14:01:06 -0400
Subject: [PATCH 3/3] feedback from @dvoytenko

---
 extensions/amp-a4a/0.1/amp-a4a.js           |  9 ++++----
 extensions/amp-a4a/0.1/test/test-amp-a4a.js | 23 ++++++++++++++-------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
index 3c1fabd61e5f..88fb97268403 100644
--- a/extensions/amp-a4a/0.1/amp-a4a.js
+++ b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -107,7 +107,7 @@ export let SizeInfoDef;
       minifiedCreative: string,
       customElementExtensions: !Array<string>,
       customStylesheets: !Array<{href: string}>,
-      ampImages: (Array<string>|undefined),
+      images: (Array<string>|undefined),
     }} */
 let CreativeMetaDataDef;
 
@@ -735,7 +735,7 @@ export class AmpA4A extends AMP.BaseElement {
           (creativeMetaDataDef.customStylesheets || []).forEach(font =>
               this.preconnect.preload(font.href));
           // Preload any AMP images.
-          (creativeMetaDataDef.ampImages || []).forEach(image =>
+          (creativeMetaDataDef.images || []).forEach(image =>
               isSecureUrl(image) && this.preconnect.preload(image));
           return creativeMetaDataDef;
         })
@@ -1481,8 +1481,9 @@ export class AmpA4A extends AMP.BaseElement {
           }
         });
       }
-      if (isArray(metaDataObj['ampImages'])) {
-        metaData.ampImages = metaDataObj['ampImages'];
+      if (isArray(metaDataObj['images'])) {
+        // Load maximum of 5 images.
+        metaData.images = metaDataObj['images'].splice(0, 5);
       }
       // TODO(keithwrightbos): OK to assume ampRuntimeUtf16CharOffsets is before
       // metadata as its in the head?
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 37c2b2c437b7..998c12e9ca93 100644
--- a/extensions/amp-a4a/0.1/test/test-amp-a4a.js
+++ b/extensions/amp-a4a/0.1/test/test-amp-a4a.js
@@ -942,7 +942,7 @@ describe('amp-a4a', () => {
       });
     });
     // TODO (keithwrightbos) - move into above e2e once signed creative with
-    // ampImage within creative can be regenerated.
+    // image within creative can be regenerated.
     it('should prefetch amp images', () => {
       return createIframePromise().then(fixture => {
         setupForAdTesting(fixture);
@@ -954,7 +954,7 @@ describe('amp-a4a', () => {
         const a4a = new MockA4AImpl(a4aElement);
         sandbox.stub(a4a, 'getAmpAdMetadata_', creative => {
           const metaData = AmpA4A.prototype.getAmpAdMetadata_(creative);
-          metaData.ampImages = ['https://prefetch.me.com?a=b', 'http://do.not.prefetch.me.com?c=d',
+          metaData.images = ['https://prefetch.me.com?a=b', 'http://do.not.prefetch.me.com?c=d',
             'https://prefetch.metoo.com?e=f'];
           return metaData;
         });
@@ -1454,7 +1454,7 @@ describe('amp-a4a', () => {
           {href: 'https://fonts.googleapis.com/css?foobar'},
           {href: 'https://fonts.com/css?helloworld'},
         ],
-        ampImages: ['https://some.image.com/a=b', 'https://other.image.com'],
+        images: ['https://some.image.com/a=b', 'https://other.image.com'],
       };
       return createIframePromise().then(fixture => {
         setupForAdTesting(fixture);
@@ -1512,23 +1512,30 @@ describe('amp-a4a', () => {
       expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData))).to.be.null;
     });
     it('should not include amp images if not an array', () => {
-      metaData.ampImages = 'https://foo.com';
+      metaData.images = 'https://foo.com';
       const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
       const expected = Object.assign({
         minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
       }, metaData);
-      delete expected.ampImages;
+      delete expected.images;
       expect(actual).to.deep.equal(expected);
     });
-    it('should tolerate missing ampImages', () => {
-      delete metaData.ampImages;
+    it('should tolerate missing images', () => {
+      delete metaData.images;
       const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
       const expected = Object.assign({
         minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
       }, metaData);
-      delete expected.ampImages;
+      delete expected.images;
       expect(actual).to.deep.equal(expected);
     });
+    it('should limit to 5 images', () => {
+      while (metaData.images.length < 10) {
+        metaData.images.push('https://another.image.com?abc=def');
+      }
+      expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData)).images.length)
+          .to.equal(5);
+    });
     // FAILURE cases here
   });