Implement getCustomRealTimeConfigMacros for Doubleclick Fast Fetch #12539

bradfrizzell · 2017-12-20T14:18:08Z

keithwrightbos · 2017-12-22T16:42:48Z

examples/doubleclick-rtc.amp.html

@@ -17,11 +17,18 @@
 <body>
  <h2> NOTE: Must disable check that isSecureUrl for testing </h2>
  <h2>Doubleclick with RTC</h2>
-    <amp-ad width="320" height="50"
+    <!--<amp-ad width="320" height="50"


remove commented out slot?

keithwrightbos · 2017-12-22T16:43:30Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

@@ -100,6 +100,7 @@ import {
 import {
  addExperimentIdToElement,
 } from '../../../ads/google/a4a/traffic-experiments';
+import {getAdCid} from '../../../src/ad-cid';


I don't see you using this yet. Presumably you'll remove and handle in second PR?

keithwrightbos · 2017-12-22T16:44:53Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+  getCustomRealTimeConfigMacros_() {
+    const macros = {};
+    const docInfo = Services.documentInfoForDoc(this.element);
+    macros['PAGEVIEWID'] = docInfo.pageViewId;


const docInfo = ..
return {
'HREF': this.win.location.href,
'DATASLOT': ...
};

Also I would have expected these to be function values such that they are only evaluated if/when the macro exists. In fact, you could make it so that the element is passed to the function thereby allowing this to be a static object. Seems much more efficient.

changed to function values

keithwrightbos · 2018-01-02T21:29:44Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+      () => this.element.getAttribute('data-multi-size-validation'),
+      OVERRIDEWIDTH: () => this.element.getAttribute('data-override-width'),
+      OVERRIDEHEIGHT: () => this.element.getAttribute('data-override-height'),
+      JSON: () => this.element.getAttribute('data-json'),


This could greatly increase the length of the url. Do we truncate the url if its too long? Do we indicate it was truncated? Is there only a portion of the JSON that they want?

added truncation to 16k

keithwrightbos · 2018-01-02T21:30:14Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+  /** @override */
+  getCustomRealTimeConfigMacros_() {
+    return {
+      PAGEVIEWID: () => {


nit: () => Services.documentInfoForDoc(this.element).pageViewId,

keithwrightbos · 2018-01-02T21:33:12Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+        return docInfo.pageViewId;
+      },
+      HREF: () => this.win.location.href,
+      DATASLOT: () => this.element.getAttribute('data-slot'),


Consider whitelist of allowed attributes and then single macro with parameter of name of attribute to be extracted:

const whitelist = {};
ATTR = name => !!whitelist[name] && this.element.getAttribute(name)

This doesn't work with how the url replacements works. It needs a full list of all valid macros to look for in the URL. This suggestion would only work if we know what the valid macros are already, and then go from there.

keithwrightbos · 2018-01-03T19:46:06Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

@@ -715,6 +715,25 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
    return {'artc': artc.join() || null, 'ati': ati.join(), 'ard': ard.join()};
  }

+  /** @override */
+  getCustomRealTimeConfigMacros_() {
+    const whitelist = {


Add documentation about why this exists (e.g. To make attributes accessible, add here...)

keithwrightbos · 2018-01-03T19:46:53Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+    return {
+      PAGEVIEWID: () => Services.documentInfoForDoc(this.element).pageViewId,
+      HREF: () => this.win.location.href,
+      ATTR: name => !!whitelist[name] && this.element.getAttribute(name),


You could be a bit fancier and dev or user warn on attempted access to missing attribute. Also, do you know if element names are case sensitive? Perhaps this should be insensitive?

keithwrightbos · 2018-01-03T21:12:32Z

extensions/amp-a4a/0.1/real-time-config-manager.js

@@ -157,6 +160,10 @@ function inflateAndSendRtc_(a4aElement, url, seenUrls, promiseArray,
        opt_vendor || url);
  }
  seenUrls[url] = true;
+  if (url.length > MAX_URL_LENGTH) {
+    url = url.substr(0, MAX_URL_LENGTH - 12).replace(/%\w?$/, '');


Nit: I would make this all one line

keithwrightbos · 2018-01-03T21:12:59Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-rtc.js

+        height: macros['height'],
+        type: 'doubleclick',
+        layout: 'fixed',
+        'data-slot': macros['data-slot'],


Verify case insensitivity

done, added some crazazy cases

keithwrightbos · 2018-01-03T21:13:20Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-rtc.js

@@ -271,4 +272,40 @@ describes.realWin('DoubleClick Fast Fetch RTC', {amp: true}, env => {
          .to.deep.equal(response);
    });
  });
+
+  describe('getCustomRealTimeConfigMacros', () => {


Add test coverage for truncation

done, broke out to separate function for easiness of testing

keithwrightbos

Assuming remaining items will be addressed.

…mpproject#12539) * Start work on macros * Add test coverage * Responding to feedback * Responded to feedback * Respond to feedback * Fix test * Change url replacement to be cleaner * Respond to feedback * feedback * Respond

bradfrizzell requested a review from keithwrightbos December 20, 2017 14:18

googlebot added the cla: yes label Dec 20, 2017

bradfrizzell force-pushed the frizz-macros branch from 9160c3d to 79e2cd9 Compare December 21, 2017 17:32

bradfrizzell added 2 commits December 21, 2017 12:32

Start work on macros

53cc966

Add test coverage

79e2cd9

keithwrightbos requested changes Dec 22, 2017

View reviewed changes

bradfrizzell added 2 commits January 2, 2018 15:05

Responding to feedback

ca71323

Responded to feedback

ac2a567

keithwrightbos requested changes Jan 2, 2018

View reviewed changes

bradfrizzell added 3 commits January 3, 2018 13:34

Respond to feedback

23c6c37

Fix test

f4370dc

Change url replacement to be cleaner

25db9af

keithwrightbos requested changes Jan 3, 2018

View reviewed changes

bradfrizzell added 2 commits January 3, 2018 14:58

Respond to feedback

04faa16

feedback

ef6c087

keithwrightbos requested changes Jan 3, 2018

View reviewed changes

keithwrightbos approved these changes Jan 3, 2018

View reviewed changes

Respond

85dca11

erwinmombay merged commit 50e3e0c into master Jan 3, 2018

bradfrizzell mentioned this pull request Jan 4, 2018

Add network-level RTC macros for doubleclick rtc implementation #12374

Closed

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Jan 10, 2018

bradfrizzell deleted the frizz-macros branch April 12, 2018 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement getCustomRealTimeConfigMacros for Doubleclick Fast Fetch #12539

Implement getCustomRealTimeConfigMacros for Doubleclick Fast Fetch #12539

bradfrizzell commented Dec 20, 2017

keithwrightbos Dec 22, 2017

bradfrizzell Dec 22, 2017

keithwrightbos Dec 22, 2017

bradfrizzell Dec 22, 2017

keithwrightbos Dec 22, 2017

bradfrizzell Jan 2, 2018

keithwrightbos Jan 2, 2018

bradfrizzell Jan 3, 2018

keithwrightbos Jan 2, 2018

bradfrizzell Jan 3, 2018

keithwrightbos Jan 2, 2018

bradfrizzell Jan 3, 2018

keithwrightbos Jan 3, 2018

bradfrizzell Jan 3, 2018

keithwrightbos Jan 3, 2018

bradfrizzell Jan 3, 2018

keithwrightbos Jan 3, 2018

keithwrightbos Jan 3, 2018

bradfrizzell Jan 3, 2018

keithwrightbos Jan 3, 2018

bradfrizzell Jan 3, 2018

keithwrightbos left a comment

Implement getCustomRealTimeConfigMacros for Doubleclick Fast Fetch #12539

Implement getCustomRealTimeConfigMacros for Doubleclick Fast Fetch #12539

Conversation

bradfrizzell commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithwrightbos left a comment

Choose a reason for hiding this comment