Cleaned PR + tests.

glevitzky · Jan 12, 2018 · dfc6942 · dfc6942
1 parent 6136724
commit dfc6942
Show file tree

Hide file tree

Showing 6 changed files with 948 additions and 72 deletions.
diff --git a/3p/nameframe.max.html b/3p/nameframe.max.html
@@ -4,8 +4,8 @@
   <meta charset="utf-8">
   <script>
     var name_data;
-    var writeInHead = true;
-    var waitForOnload = true;
+    var writeInBody = false;
+    var instantLoad = false;
 
     var tryProtected = function(blockFn) {
       try {
@@ -19,8 +19,8 @@
 
     tryProtected(function() {
       name_data = JSON.parse(window.name);
-      writeInHead = name_data['writeInHead'];
-      waitForOnload = name_data['waitForOnload'];
+      writeInBody = name_data['writeInBody'];
+      instantLoad = name_data['instantLoad'];
       if (!name_data || !name_data['creative']) {
         throw new Error('Missing or incorrectly formatted JSON ' +
             'input to nameframe');
@@ -36,19 +36,18 @@
         window.name = JSON.stringify(name_data);
       });
     };
-
-    if (writeInHead && waitForOnload) {
+    if (!writeInBody && !instantLoad) {
       window.onload = render;
-    } else if (writeInHead) {
+    } else if (!writeInBody) {
       render();
     }
   </script>
 </head>
 <body>
   <script>
-    if (!writeInHead && waitForOnload) {
+    if (writeInBody && !instantLoad) {
       window.onload = render;
-    } else if (!writeInHead) {
+    } else if (writeInBody) {
       render();
     }
   </script>

diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -753,7 +753,9 @@ export class AmpA4A extends AMP.BaseElement {
           const {bytes, headers} = responseParts;
           const size = this.extractSize(responseParts.headers);
           this.creativeSize_ = size || this.creativeSize_;
-          if (bytes) {
+          if (this.experimentalNonAmpCreativeRenderMethod_ !=
+              XORIGIN_MODE.CLIENT_CACHE &&
+              bytes) {
             this.creativeBody_ = bytes;
           }
           this.handleLifecycleStage_('adResponseValidateStart');
@@ -990,7 +992,7 @@ export class AmpA4A extends AMP.BaseElement {
         this.handleLifecycleStage_('iframeAlreadyExists');
         return Promise.resolve();
       }
-      if (!creativeMetaData || true) {
+      if (!creativeMetaData) {
         // Non-AMP creative case, will verify ad url existence.
         return this.renderNonAmpCreative_();
       }
@@ -1284,7 +1286,7 @@ export class AmpA4A extends AMP.BaseElement {
     const method = this.experimentalNonAmpCreativeRenderMethod_;
     let renderPromise = Promise.resolve(false);
     if ((method == XORIGIN_MODE.SAFEFRAME ||
-         method == XORIGIN_MODE.NAMEFRAME || true) &&
+         method == XORIGIN_MODE.NAMEFRAME) &&
         this.creativeBody_) {
       renderPromise = this.renderViaNameAttrOfXOriginIframe_(
           this.creativeBody_);
@@ -1464,7 +1466,7 @@ export class AmpA4A extends AMP.BaseElement {
    */
   renderViaNameAttrOfXOriginIframe_(creativeBody) {
     /** @type {string} */
-    const method = 'nameframe';//this.experimentalNonAmpCreativeRenderMethod_;
+    const method = this.experimentalNonAmpCreativeRenderMethod_;
     dev().assert(method == XORIGIN_MODE.SAFEFRAME ||
         method == XORIGIN_MODE.NAMEFRAME,
     'Unrecognized A4A cross-domain rendering mode: %s', method);

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
@@ -375,11 +375,10 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
     /** @private {boolean} */
     this.isIdleRender_ = false;
 
-    /** @private {{writeInHead: boolean, waitForOnload: boolean, control: true}} */
+    /** @private {{instantLoad: boolean, writeInBody: boolean}} */
     this.nameframeExperimentConfig_ = {
-      waitForOnload: true,
-      writeInHead: true,
-      control: true,
+      instantLoad: false,
+      writeInBody: false,
     };
   }
 
@@ -805,18 +804,13 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
       this.element.setAttribute(DATA_ATTR_NAME, refreshInterval);
     }
 
-    const nameframeExperimentConfig = responseHeaders.get('amp-nameframe-exp') || 'writeInHead';
+    const nameframeExperimentConfig = responseHeaders.get('amp-nameframe-exp');
     if (nameframeExperimentConfig) {
-      nameframeExperimentConfig.split(';').map(config => { debugger;
-        switch (config) {
-          case 'waitForOnload':
-          case 'writeInHead':
-            // Both parameters are true by default; we flip those sent back on
-            // the header.
-            this.nameframeExperimentConfig_[config] = false;
+      nameframeExperimentConfig.split(';').forEach(config => {
+        if (config == 'instantLoad' || config == 'writeInBody') {
+          this.nameframeExperimentConfig_[config] = true;
         }
       });
-      this.nameframeExperimentConfig_['control'] = false;
     }
 
     if (this.isFluid_) {

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
@@ -260,6 +260,32 @@ describes.realWin('amp-ad-network-doubleclick-impl', realWinConfig, env => {
       expect(impl.refreshManager_.refreshInterval_).to.equal('45');
     });
 
+    it('should specify nameframe loading behavior; single arg', () => {
+      impl.extractSize({
+        get(name) {
+          return name == 'amp-nameframe-exp' ? 'instantLoad' : undefined;
+        },
+        has(name) {
+          return !!this.get(name);
+        },
+      });
+      expect(impl.nameframeExperimentConfig_.instantLoad).to.be.true;
+      expect(impl.nameframeExperimentConfig_.writeInBody).to.be.false;
+    });
+
+    it('should specify nameframe loading behavior; two args', () => {
+      impl.extractSize({
+        get(name) {
+          return name == 'amp-nameframe-exp' ?
+              'instantLoad;writeInBody' : undefined;
+        },
+        has(name) {
+          return !!this.get(name);
+        },
+      });
+      expect(impl.nameframeExperimentConfig_.instantLoad).to.be.true;
+      expect(impl.nameframeExperimentConfig_.writeInBody).to.be.true;
+    });
   });
 
   describe('#onCreativeRender', () => {

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
@@ -379,7 +379,7 @@ export class AmpAdXOriginIframeHandler {
     // The iframe may have been removed by the time we resize.
     if (!this.iframe) {
       return;
-    } debugger;
+    }
     postMessageToWindows(
         this.iframe,
         [{win: source, origin}],