Removing static config, amp-ad changes, and fixing tests.

ampproject · Jul 24, 2018 · 627c3b7 · 627c3b7
1 parent cb6434d
commit 627c3b7
Show file tree

Hide file tree

Showing 9 changed files with 250 additions and 742 deletions.
diff --git a/extensions/amp-a4a/0.1/amp-ad-template.js b/extensions/amp-a4a/0.1/amp-ad-template.js
@@ -20,8 +20,6 @@ import {
 } from './amp-ad-type-defs';
 import {AmpAdNetworkBase} from './amp-ad-network-base';
 import {NameFrameRenderer} from './name-frame-renderer';
-import {NetworkRegistry} from './template-config';
-import {Services} from '../../../src/services';
 import {TemplateRenderer} from './template-renderer';
 import {TemplateValidator} from './template-validator';
 import {addParamToUrl} from '../../../src/url';
@@ -46,17 +44,8 @@ export class AmpAdTemplate extends AmpAdNetworkBase {
     this.registerRenderer(nameFrameRenderer, ValidatorResult.NON_AMP);
 
     /** @const {string} */
-    this.networkType_ = element.getAttribute('type');
-    dev().assert(this.networkType_,
-        'Template amp-ad elements must specify a type');
-
-    const networkConfig = NetworkRegistry[this.networkType_];
-    dev().assert(networkConfig, `Invalid network type ${this.networkType_}`);
-
-    /** @const {string} */
-    this.requestUrl_ = networkConfig.requestUrl;
-    dev().assert(this.requestUrl_,
-        'Invalid network configuration: no request URL specified');
+    this.baseRequestUrl_ = element.getAttribute('src');
+    dev().assert(this.baseRequestUrl_, 'A src attribute must be specified.');
 
     this.getContext().win = this.win;
   }
@@ -73,11 +62,7 @@ export class AmpAdTemplate extends AmpAdNetworkBase {
 
   /** @override */
   getRequestUrl() {
-    let url = Services.urlReplacementsForDoc(this.element)
-        .expandUrlSync(this.requestUrl_, {
-          width: this.getContext().size.width,
-          height: this.getContext().size.height,
-        });
+    let url = this.baseRequestUrl_;
 
     // We collect all fields in the dataset of the form
     // 'data-request-var-<field_name>=<val>`, and append &<field_name>=<val> to
@@ -91,8 +76,8 @@ export class AmpAdTemplate extends AmpAdNetworkBase {
           // will automatically put it into upper case.
           const finalParamName = requestParamName.charAt(0).toLowerCase() +
               requestParamName.slice(1);
-          url = addParamToUrl(
-              url, finalParamName, this.element.dataset[dataField]);
+          const finalParamValue = this.element.dataset[dataField];
+          url = addParamToUrl(url, finalParamName, finalParamValue);
         }
       }
     });

diff --git a/extensions/amp-a4a/0.1/amp-ad-type-defs.js b/extensions/amp-a4a/0.1/amp-ad-type-defs.js
@@ -21,12 +21,6 @@
     }} */
 export let LayoutInfoDef;
 
-/** @typedef {{
-      templateUrl: string,
-      data: (JsonObject|undefined),
-    }} */
-export let AmpTemplateCreativeDef;
-
 /** @enum {string} */
 export const FailureType = {
   REQUEST_ERROR: 'REQUEST_ERROR',

diff --git a/extensions/amp-a4a/0.1/template-config.js b/extensions/amp-a4a/0.1/template-config.js
diff --git a/extensions/amp-a4a/0.1/template-renderer.js b/extensions/amp-a4a/0.1/template-renderer.js
@@ -25,7 +25,7 @@ export class TemplateRenderer extends FriendlyFrameRenderer {
   render(context, element, creativeData) {
     return super.render(context, element, creativeData).then(() => {
       const templateData =
-          /** @type {!./amp-ad-type-defs.AmpTemplateCreativeDef} */ (
+      /** @type {!./amp-ad-type-defs.AmpTemplateCreativeDef} */ (
           creativeData.templateData);
       const {data} = templateData;
       if (!data) {

diff --git a/extensions/amp-a4a/0.1/template-validator.js b/extensions/amp-a4a/0.1/template-validator.js
@@ -53,7 +53,7 @@ export class TemplateValidator extends Validator {
 
     const body = utf8Decode(/** @type {!ArrayBuffer} */ (unvalidatedBytes));
     const parsedResponseBody =
-        /** @type {./amp-ad-type-defs.AmpTemplateCreativeDef} */ (
+    /** @type {./amp-ad-type-defs.AmpTemplateCreativeDef} */ (
         tryParseJson(body));
 
     // If we're missing the relevant header, or headers altogether, we cannot

diff --git a/extensions/amp-a4a/0.1/test/test-amp-ad-template.js b/extensions/amp-a4a/0.1/test/test-amp-ad-template.js
@@ -20,7 +20,6 @@ import {
 } from '../template-validator';
 import {AmpAdTemplate} from '../amp-ad-template';
 import {AmpMustache} from '../../../amp-mustache/0.1/amp-mustache';
-import {NetworkRegistry} from '../template-config';
 import {data} from './testdata/valid_css_at_rules_amp.reserialized';
 import {tryParseJson} from '../../../../src/json';
 import {utf8Encode} from '../../../../src/utils/bytes';
@@ -42,7 +41,7 @@ describes.realWin('TemplateRenderer', realWinConfig, env => {
     containerElement = document.createElement('div');
     containerElement.setAttribute('height', 50);
     containerElement.setAttribute('width', 320);
-    containerElement.setAttribute('type', 'custom-template');
+    containerElement.setAttribute('src', 'http://www.foo.com');
     containerElement.signals = () => ({
       whenSignal: () => Promise.resolve(),
     });
@@ -172,36 +171,23 @@ describes.realWin('TemplateRenderer', realWinConfig, env => {
       impl.buildCallback();
       impl.getRequestUrl();
       expect(impl.getContext().adUrl).to
-          .equal(NetworkRegistry['custom-template'].requestUrl);
+          .equal(impl.element.getAttribute('src'));
     });
     it('should expand url', () => {
       impl.buildCallback();
       impl.element.setAttribute('data-request-param-bar', '123');
       impl.element.setAttribute('data-request-param-baz', '456');
       impl.element.setAttribute('data-request-param-camel-case', '789');
-      impl.requestUrl_ = 'https://foo.com?sz=widthxheight';
       expect(impl.getRequestUrl()).to.equal(
-          'https://foo.com?sz=320x50&bar=123&baz=456&camelCase=789');
+          'http://www.foo.com?bar=123&baz=456&camelCase=789');
     });
   });
 
   describe('mandatory fields', () => {
-    it('should throw if missing type attribute', () => {
-      containerElement.removeAttribute('type');
+    it('should throw if element does not have src attribute', () => {
+      containerElement.removeAttribute('src');
       expect(() => new AmpAdTemplate(containerElement)).to
-          .throw('Template amp-ad elements must specify a type');
-    });
-    it('should throw if type is not registered', () => {
-      containerElement.setAttribute('type', 'foo');
-      expect(() => new AmpAdTemplate(containerElement)).to
-          .throw('Invalid network type foo');
-    });
-    it('should throw if type does not have corresponding request url', () => {
-      const temp = NetworkRegistry['custom-template'].requestUrl;
-      delete NetworkRegistry['custom-template'].requestUrl;
-      expect(() => new AmpAdTemplate(containerElement)).to
-          .throw('Invalid network configuration: no request URL specified');
-      NetworkRegistry['custom-template'].requestUrl = temp;
+          .throw('A src attribute must be specified.');
     });
   });
 });

diff --git a/extensions/amp-ad/0.1/amp-ad.js b/extensions/amp-ad/0.1/amp-ad.js
@@ -15,7 +15,6 @@
 
 import {AmpAd3PImpl} from './amp-ad-3p-impl';
 import {AmpAdCustom} from './amp-ad-custom';
-import {AmpAdTemplate} from '../../amp-a4a/0.1/amp-ad-template';
 import {CSS} from '../../../build/amp-ad-0.1.css';
 import {Services} from '../../../src/services';
 import {adConfig} from '../../../ads/_config';
@@ -59,20 +58,14 @@ export class AmpAd extends AMP.BaseElement {
     const type = this.element.getAttribute('type');
     return consent.then(() => {
       const isCustom = type === 'custom';
-      const isTemplate = this.element.hasAttribute('template');
-      user().assert(isCustom || isTemplate || hasOwn(adConfig, type)
+      user().assert(isCustom || hasOwn(adConfig, type)
           || hasOwn(a4aRegistry, type), `Unknown ad type "${type}"`);
 
       // Check for the custom ad type (no ad network, self-service)
       if (isCustom) {
         return new AmpAdCustom(this.element);
       }
 
-      // Check for templatized amp-ad.
-      if (isTemplate) {
-        return new AmpAdTemplate(this.element);
-      }
-
       this.win.ampAdSlotIdCounter = this.win.ampAdSlotIdCounter || 0;
       const slotId = this.win.ampAdSlotIdCounter++;
 

diff --git a/extensions/amp-ad/0.1/test/test-amp-ad.js b/extensions/amp-ad/0.1/test/test-amp-ad.js
@@ -16,7 +16,6 @@
 
 import {AmpAd} from '../amp-ad';
 import {AmpAd3PImpl} from '../amp-ad-3p-impl';
-import {AmpAdTemplate} from '../../../amp-a4a/0.1/amp-ad-template';
 import {Services} from '../../../../src/services';
 import {adConfig} from '../../../../ads/_config';
 import {getA4ARegistry} from '../../../../ads/_a4a-config';
@@ -114,13 +113,6 @@ describes.realWin('Ad loader', {amp: true}, env => {
         });
       });
 
-      it('uses AmpAdTemplate when template attribute is present', () => {
-        ampAdElement.setAttribute('template', true);
-        ampAdElement.setAttribute('type', 'test');
-        return expect(ampAd.upgradeCallback()).to.eventually.be
-            .instanceof(AmpAdTemplate);
-      });
-
       it('fails upgrade on A4A upgrade with loadElementClass error', () => {
         a4aRegistry['zort'] = function() {
           return true;