Amp-Analytics 3p vendor obfuscation #8887

jonkeller · 2017-04-22T15:10:40Z

Allows third-party analytics vendors to use the amp-analytics tag. They can now specify requestsFrameUrl in vendors.js, and that will be loaded as a cross-domain iframe. It will be sent the events, and they can use arbitrary JS to process them. The transport glue will not be subject to version lock.

Closes #9433

jonkeller · 2017-04-22T15:11:04Z

examples/analytics-3p-remote-frame-helper.js

+if (onNewAmpAnalyticsInstance) {
+  onNewAmpAnalyticsInstance(instance);
+  // Warning: the following code is likely only temporary. Don't check in
+  // before getting resolution on that.


^^ See comment

jonkeller · 2017-04-22T15:15:16Z

examples/analytics-3p.amp.html

+  <script async custom-element="amp-ad" src="https://cdn.ampproject.org/v0/amp-ad-0.1.js"></script>
+  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+  <style amp-custom>


Fake ad network is broken! Created Issue #8935
The following CSS hack fixes display issues. The next file (see below) is a .json file, which in the existing example contains a JSON object, one of whose values is the HTML of the fake creative. However the fake ad JS code ends up just displaying the entire JSON, not just the HTML. So that's why, for now, the next file is .json but contains only HTML.
We should fix the fake ad network. I can tackle that in a separate PR, and then pretty this one up. But for now, this file and the next contain hacks to get it functional.

jonkeller · 2017-04-22T15:30:22Z

extensions/amp-analytics/0.1/amp-analytics.js

+  /**
+   * Sends a message to a cross-domain frame, or queues the message if the
+   * frame is not yet ready to receive messages.
+   * TODO: Implement throttling if messages are sent too rapidly.


jonkeller · 2017-04-22T15:30:31Z

extensions/amp-analytics/0.1/amp-analytics.js

+      }
+      config['requestsFrameUrl'] += config['dataHash'];
+    }
+    // TODO: Safety check requestsFrameUrl!


jonkeller · 2017-04-22T15:31:15Z

examples/analytics-3p-remote-frame.html

+          }
+        });
+        // TODO: Could also do something like:
+        // instance.sendMessageToCreative('hello');


jonkeller · 2017-04-23T13:46:10Z

extensions/amp-analytics/0.1/amp-analytics.js

@@ -694,4 +719,100 @@ export class AmpAnalytics extends AMP.BaseElement {
  }
 }

+/**
+ * Keeps track of the third-party vendors' cross-domain iframes. Only one
+ * static instance will be created, which will keep manage ALL amp-analytics


Typo: should be "keep and manage"

keithwrightbos · 2017-04-24T14:10:06Z

@bradfrizzell to take first pass on review, please notify me when you think it is ready for my review.

bradfrizzell · 2017-04-24T15:17:33Z

extensions/amp-ad-network-fake-impl/0.1/data/fake_amp_ad_with_3p_analytics.json

@@ -0,0 +1 @@
+<!-- No, this is not actually JSON. Fake ad network is kind of broken and should be fixed separately. --><!doctype html><html amp4ads> <head> <meta charset=utf-8> <script async src='https://cdn.ampproject.org/v0.js'></script> <script async custom-element=amp-analytics src='https://cdn.ampproject.org/v0/amp-analytics-0.1.js'></script> <style amp4ads-boilerplate>body{visibility:hidden}</style> <meta name=viewport content=width=device-width,minimum-scale=1> </head> <body> <amp-img id=img src='https://upload.wikimedia.org/wikipedia/commons/6/6e/Golde33443.jpg' width=256 height=300></amp-img> <amp-analytics> <script type='application/json'> { "requestsFrameUrl": "http://localhost:8000/examples/analytics-3p-remote-frame.html", "dataHash": "key1=val1&key2=val2", "requests": { "pageview": "https://example.com/analytics?viewed=true&url=${canonicalUrl}&title=${title}&acct=${account}", "event": "https://example.com/analytics?eid=click&acct=${account}" }, "vars": { "account": "ABC123" }, "triggers": { "defaultPageview": { "on": "visible", "request": "pageview" }, "anchorClicks": { "on": "click", "selector": "img", "request": "event" } } } </script> </amp-analytics> </body></html>


If this isn't JSON, why is it in a .json file rather than .html?

See comment above beginning with "Fake ad network is broken!". I'll go fix it in a separate PR, and then correct this here. But tl;dr if you put JSON like it says you're supposed to, you see the raw JSON in your ad, and if you try to specify <amp-ad type="fake" src="foo.html"> rather than foo.json, then you see nothing at all.

whoops sorry see that now

bradfrizzell · 2017-04-24T15:18:40Z

extensions/amp-analytics/0.1/amp-analytics.js

@@ -256,6 +259,23 @@ export class AmpAnalytics extends AMP.BaseElement {
    return Promise.all(promises);
  }

+  handleRequestsFrameUrl_(config) {


bradfrizzell · 2017-04-24T15:19:26Z

extensions/amp-analytics/0.1/amp-analytics.js

@@ -256,6 +259,23 @@ export class AmpAnalytics extends AMP.BaseElement {
    return Promise.all(promises);
  }

+  handleRequestsFrameUrl_(config) {
+    if (config['requestsFrameUrl'] && config['dataHash']) {


make sure in the jsdoc that config is explicitly non-nullable, otherwise this predicate can throw

bradfrizzell · 2017-04-24T15:21:13Z

extensions/amp-analytics/0.1/amp-analytics.js

+ */
+class AmpAnalyticsFrames {
+  constructor() {
+    this.map = new Map();


Name something more descriptive than 'map'. What is the purpose of this map? Could the various properties stored within all instead be instance variables on the class? It seems like the map is a layer of obfuscation that will make reading this class less clear in the future (unless there is a reason for the map I don't know).

bradfrizzell · 2017-04-24T15:22:12Z

extensions/amp-analytics/0.1/amp-analytics.js

+   */
+  create(parent, requestsFrameUrl) {
+    const context = {
+      "scriptSrc": "/examples/analytics-3p-remote-frame-helper.js",


nit: I believe these should be single quotes, no?

Agree. Also note that this is the flip side of my comment on examples/analytics-3p-remote-frame-helper.js line 17.

bradfrizzell · 2017-04-24T15:33:53Z

examples/analytics-3p-remote-frame-helper.js

@@ -0,0 +1,26 @@
+
+class AmpAnalyticsInstance {


Just name the class AmpAnalytics or something else without Instance in the name. As it is now, an instance of this class is an AmpAnalyticsInstance instance, which is confusing.

bradfrizzell · 2017-04-24T15:34:08Z

examples/analytics-3p-remote-frame-helper.js

+    this.listener = listener;
+  }
+};
+let instance = new AmpAnalyticsInstance();


variable naming

Done / embarrassed

bradfrizzell · 2017-04-24T15:35:57Z

examples/analytics-3p-remote-frame-helper.js

+
+// The onNewAmpAnalyticsInstance() function must be implemented by the
+// vendor's page
+if (onNewAmpAnalyticsInstance) {


this should be window.onNewAmpAnalyticsInstance, as per the example frame.

bradfrizzell · 2017-04-24T15:38:18Z

examples/analytics-3p-remote-frame.html

+    <meta charset="UTF-8">
+    <title>Requests Frame</title>
+    <script>
+      window.onNewAmpAnalyticsInstance = (instance) => {


jsdoc would make this example more useful

Done, please let me know if not sufficient.

bradfrizzell · 2017-04-24T15:43:10Z

extensions/amp-analytics/0.1/amp-analytics.js

+  send(requestsFrameUrl, message) {
+    try {
+      const mapEntry = this.map.get(requestsFrameUrl);
+      if (mapEntry.isReady) {


mapEntry here as well

Done: renamed to frameData, refactored to transport.js.

jridgewell · 2017-04-25T18:18:28Z

extensions/amp-analytics/0.1/amp-analytics.js

+   */
+  handleRequestsFrameUrl_(config) {
+    if (config['requestsFrameUrl'] && config['dataHash']) {
+      if (!config['dataHash'].startsWith('#')) {


Please use the startsWith helper.

jridgewell · 2017-04-25T18:19:19Z

extensions/amp-analytics/0.1/amp-analytics.js

+      if (!config['dataHash'].startsWith('#')) {
+        config['requestsFrameUrl'] += '#';
+      }
+      config['requestsFrameUrl'] += config['dataHash'];


Is it really necessary to mutate this value? This method isn't reentrant as is.

You're right, this is kind of sketchy. My logic at the time is that the map is going to be keyed off of the concatenation, so might as well not duplicate that (admittedly small) effort every time we do a get/set/has on it. But...yeah, it's wrong. Will fix. Thanks.

jridgewell · 2017-04-25T18:22:11Z

extensions/amp-analytics/0.1/amp-analytics.js

+ */
+class AmpAnalyticsFrames {
+  constructor() {
+    this.requestsFrameMap = new Map();


Map is not available in all environments. Please use a prototype-less object (we have a helper).

jridgewell · 2017-04-25T18:22:40Z

extensions/amp-analytics/0.1/amp-analytics.js

+    let frame = createElementWithAttributes(parent, 'iframe', {
+      sandbox: 'allow-scripts',
+      name: JSON.stringify({
+        'scriptSrc': '/examples/analytics-3p-remote-frame-helper.js'


Yeah, comment at L803 applies here as well. I should annotate in the code that this also needs to be replaced with something more permanent before checking in.

@keithwrightbos @jridgewell Where do you guys think the helper JS should live?

jridgewell · 2017-04-25T18:23:24Z

extensions/amp-analytics/0.1/amp-analytics.js

+        'scriptSrc': '/examples/analytics-3p-remote-frame-helper.js'
+      }),
+    });
+    frame.onload = () => {


Please use loadPromise

jridgewell · 2017-04-25T18:24:08Z

extensions/amp-analytics/0.1/amp-analytics.js

+      this.setIsReady(requestsFrameUrl);
+    };
+    frame.src = requestsFrameUrl; // Set AFTER onload to avoid race
+    frame.style.cssText='width:0;height:0;visibility:hidden;';


Please use setStyles

jridgewell · 2017-04-25T18:25:46Z

extensions/amp-analytics/0.1/amp-analytics.js

+        requestsFrame.msgQueue.push(message);
+      }
+    } catch (err) {
+      console.error("Failed to send event '" + err + "' to URL '" +


There's not an equivalent catch in the deferred case.

jridgewell · 2017-04-25T18:26:23Z

extensions/amp-analytics/0.1/amp-analytics.js

+    requestsFrame.isReady = true;
+    this.send_(requestsFrame.frame, requestsFrame.msgQueue);
+    requestsFrame.msgQueue = [];
+    this.requestsFrameMap.set(requestsFrameUrl, requestsFrame);


You can avoid the reset by truncating the array.

Done (just deleted it instead of truncating, since will not need it again.)

jridgewell · 2017-04-25T18:28:35Z

extensions/amp-analytics/0.1/amp-analytics.js

+ * static instance will be created, which will keep and manage ALL amp-analytics
+ * tags.
+ */
+class AmpAnalyticsFrames {


This likely belongs in another class, hopefully one that reduces the duplication between this and transport.js.

Agree. Was not able to get to this today, but will do.

Done - refactored into transport.js

jonkeller · 2017-04-25T21:52:35Z

extensions/amp-analytics/0.1/amp-analytics.js

    requestsFrame.isReady = true;
    this.send_(requestsFrame.frame, requestsFrame.msgQueue);
    requestsFrame.msgQueue = [];
-    this.requestsFrameMap.set(requestsFrameUrl, requestsFrame);


@jridgewell You commented earlier here about avoiding the reset. I tested simply removing the .set() here and it works fine but please let me know if that is what you were intending.

jonkeller · 2017-04-25T21:54:06Z

extensions/amp-analytics/0.1/amp-analytics.js

      }),
    });
-    frame.onload = () => {
+    loadPromise(frame).then(() => {


Linter seems wary of this. Is this how you intended for loadPromise to be used here, @jridgewell ?

[16:45:49] ========== [16:45:49] Found forbidden: "{loadPromise" in extensions/amp-analytics/0.1/amp-analytics.js:19:8 [16:45:49] Most users should use BaseElement…loadPromise. [16:45:49] ========== [16:45:49] Found forbidden: " loadPromise" in extensions/amp-analytics/0.1/amp-analytics.js:771:4 [16:45:49] Most users should use BaseElement…loadPromise. [16:45:49] ========== [16:45:53] Please remove these usages or consult with the AMP team.

jonkeller · 2017-04-25T21:55:55Z

extensions/amp-analytics/0.1/amp-analytics.js

+          const requestsFrameKey = this.buildRequestsFrameUrlWithDataHash(
+              this.config_['requestsFrameUrl'], this.config_['dataHash']);
+          AmpAnalytics.requestsFrames.send(requestsFrameKey,
+            request);


This is another case where extracting requestsFrameKey is not strictly necessary, but looks better to my eye, as to not do so would be very cluttered. Please let me know if anyone disagrees.

bradfrizzell

I'm still finding the naming of requestsFrameMap and requestsFrameUrl etc to be pretty confusing. A 'requestsFrame' is a third party amp analytics iframe, right? And the map is a map of ping urls to the iframes? And the url is an analytics ping url? Is all that correct? If so, I think this code would be made a lot more readable with different names.

bradfrizzell · 2017-04-26T17:21:36Z

extensions/amp-analytics/0.1/amp-analytics.js

+    const frame = createElementWithAttributes(parent, 'iframe', {
+      sandbox: 'allow-scripts',
+      name: JSON.stringify({
+        'scriptSrc': '/examples/analytics-3p-remote-frame-helper.js',


// DO NOT SUBMIT

Agreed! This cannot be a relative url

Let's discuss what it should be. https://cdn.ampproject.org/something... but I'm not clear on the semantics of /c and /c/s etc. after that.

bradfrizzell · 2017-04-26T17:23:32Z

extensions/amp-analytics/0.1/amp-analytics.js

+    loadPromise(frame).then(() => {
+      this.setIsReady(requestsFrameUrl);
+    });
+    frame.src = requestsFrameUrl; // Set AFTER loadPromise to avoid race


If this needs to be done after loadPromise is done, the current implementation will not do that. loadPromise returns a promise, and frame.src will be set immediately after the previous line, whether or not that promise has actually resolved. Is that an issue?

I don't need to do it after the promise is done, just after the promise is set up. In other words I don't want the sequence to be: 1. src is set, 2. frame is loaded, 3. ready event is fired, 4. we set up something to listen for the ready event, because then the ready event may not get received by anything. I may be being overzealous here, given JavaScript's single-threadedness, though.

Ah I see. Just wanted clarification based on your comment there

Fair point. I can update the comment to make it clearer.

jonkeller · 2017-04-26T18:49:46Z

@bradfrizzell Re: the "I'm still finding..." comment, basically yes, you understand correctly. I will change the names, both because you're right that they're a bit confusing, and because the JSON config format just changed such that "requestsFrameUrl" is no longer called that.

keithwrightbos

We should handle the case where we remove the iframe if all amp analytics instances of a given type (e.g. moat) have been removed via unlayoutCallback.

keithwrightbos · 2017-04-28T13:42:03Z

examples/analytics-3p-remote-frame-helper.js

@@ -0,0 +1,26 @@
+


Add copyright comment?

keithwrightbos · 2017-04-28T13:42:17Z

examples/analytics-3p-remote-frame-helper.js

+
+class AmpAnalyticsRemoteFrameManager {
+  constructor() {
+    this.listener = null;


private? type doc?

keithwrightbos · 2017-04-28T13:42:53Z

examples/analytics-3p-remote-frame-helper.js

+    this.listener = listener;
+  }
+};
+let remoteFrameMgr = new AmpAnalyticsRemoteFrameManager();


keithwrightbos · 2017-04-28T13:43:48Z

examples/analytics-3p-remote-frame.html

+       */
+      window.onNewAmpAnalyticsInstance = (instance) => {
+        instance.registerAmpAnalyticsEventListener((arrayOfEventStrings) => {
+          for (evt of arrayOfEventStrings) {


I personally prefer arrayOfEventStrings.forEach(evt => {
....
});

keithwrightbos · 2017-04-28T15:12:58Z

extensions/amp-analytics/0.1/amp-analytics.js

@@ -199,6 +202,8 @@ export class AmpAnalytics extends AMP.BaseElement {
    this.analyticsGroup_ =
        this.instrumentation_.createAnalyticsGroup(this.element);

+    this.handleRequestsFrameUrl_();


This can be triggered from buildCallback if the amp-analytics element has attribute trigger=immediate but we definitely cannot modify the page at that point. Instead we should do this explicitly as part of layoutCallback. Also we need to find a way to "deprioritize" the frame creation as current code would give it priority of 1 while we want only the frame creation to be 2. This will need some discussion with Dima.

keithwrightbos · 2017-04-28T15:19:41Z

extensions/amp-analytics/0.1/amp-analytics.js

+    const frame = createElementWithAttributes(parent, 'iframe', {
+      sandbox: 'allow-scripts',
+      name: JSON.stringify({
+        'scriptSrc': '/examples/analytics-3p-remote-frame-helper.js',


Agreed! This cannot be a relative url

keithwrightbos · 2017-04-28T15:21:03Z

extensions/amp-analytics/0.1/amp-analytics.js

+      this.setIsReady(requestsFrameUrl);
+    });
+    frame.src = requestsFrameUrl; // Set AFTER loadPromise to avoid race
+    setStyles(frame, {width: 0,height: 0,visibility: 'hidden'});


Why not display none vs visibility hidden? This should also likely be position absolute at the top of the page or something similar to ensure it doesn't potentially interact with any elements on the page

keithwrightbos · 2017-04-28T15:21:59Z

extensions/amp-analytics/0.1/amp-analytics.js

+    });
+    frame.src = requestsFrameUrl; // Set AFTER loadPromise to avoid race
+    setStyles(frame, {width: 0,height: 0,visibility: 'hidden'});
+    this.requestsFrameMap[requestsFrameUrl] = {


Given load modifies this object, shouldn't it be initialized prior to load listener?

keithwrightbos · 2017-04-28T15:23:19Z

extensions/amp-analytics/0.1/amp-analytics.js

+    const requestsFrame = this.requestsFrameMap[requestsFrameUrl];
+    requestsFrame.isReady = true;
+    this.send_(requestsFrame.frame, requestsFrame.msgQueue);
+    requestsFrame.msgQueue = [];


Why not deallocate completely? delete requestFrame.requestsFrame

That's fine, since we can (currently) never go from isReady=true to isReady=false. Do you think that will ever be possible? If so, and if we delete msgQueue, then in that case we'd have to reset it to []. I currently can't think of any case where we'd need to do that but figured I'd bring it up.

keithwrightbos · 2017-04-28T15:24:05Z

extensions/amp-analytics/0.1/amp-analytics.js

+    // Warning: the following code is likely only temporary. Don't check
+    // in before getting resolution on that.
+    frame && frame.contentWindow &&
+      frame.contentWindow.postMessage({ampAnalyticsEvents: messages}, '*');


I assume all browsers we support will handle passing content as an object instead of string? Is this what we do in window context? Also, I thought we wanted to share the window.context infrastructure?

As I recall, we decided w/ Brad that that would bring too much baggage / require much effort to expose? Happy to re-open the discussion - as the comment on 816 says, the current impl isn't intended to necessarily be permanent.

jonkeller · 2017-05-12T18:12:54Z

extensions/amp-ad-network-fake-impl/0.1/data/fake_amp_ad_with_3p_analytics.json

@@ -0,0 +1 @@
+<!-- No, this is not actually JSON. Fake ad network is kind of broken and should be fixed separately. --><!doctype html><html amp4ads> <head> <meta charset=utf-8> <script async src='https://cdn.ampproject.org/v0.js'></script> <script async custom-element=amp-analytics src='https://cdn.ampproject.org/v0/amp-analytics-0.1.js'></script> <style amp4ads-boilerplate>body{visibility:hidden}</style> <meta name=viewport content=width=device-width,minimum-scale=1> </head> <body> <amp-img id=img src='https://upload.wikimedia.org/wikipedia/commons/6/6e/Golde33443.jpg' width=256 height=300></amp-img> <amp-analytics> <script type='application/json'> { "requests": { "pageview": "viewed=true&url=${canonicalUrl}&title=${title}&acct=${account}", "event": "eid=click&acct=${account}" }, "vars": { "account": "ABC123" }, "triggers": { "defaultPageview": { "on": "visible", "request": "pageview" }, "anchorClicks": { "on": "click", "selector": "img", "request": "event" } }, "transport": { "iframe": "http://localhost:8000/examples/analytics-3p-remote-frame.html", "dataHash": "key1=val1&key2=val2", "beacon": false, "xhrpost": false, "image": false } } </script> </amp-analytics> </body></html>


…f for a few days

lannka · 2017-05-12T20:55:31Z

As mentioned in email, before moving further on this PR, I'd like to see the overall plan of all the changes to amp-analytics, and how do you split up PRs and approach to that.

On the other side, again, I'd really like to have the user of this API (like Moat) to agree on the design.

jonkeller · 2017-05-18T02:04:07Z

extensions/amp-analytics/0.1/amp-analytics.js

@@ -573,6 +578,25 @@ export class AmpAnalytics extends AMP.BaseElement {
          return request;
        });
  }
+/* TODO REMOVE THIS BEFORE CHECKING IN!


I didn't add this, so suspect it's a conflict from rebasing or some such. Will investigate.

jonkeller · 2017-05-18T02:05:19Z

examples/analytics-3p-remote-frame-helper.js

+if (window.onNewAmpAnalyticsInstance) {
+  window.onNewAmpAnalyticsInstance(remoteFrameMgr_);
+  // Warning: the following code is likely only temporary. Don't check in
+  // before getting resolution on that.


Pointing out this comment again since previous one got marked outdated.

jonkeller · 2017-05-18T02:06:08Z

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

@@ -59,15 +59,24 @@ export class AmpAdNetworkFakeImpl extends AmpA4A {
          // and the signature is "FAKESIG". This mode is only allowed in
          // `localDev` and primarily used for A4A Envelope for testing.
          // See DEVELOPING.md for more info.
-          const creative = this.transformCreativeLocalDev_(deserialized);


Ignore changes to this file...will become moot once #9331 merges.

jonkeller · 2017-05-18T02:08:56Z

extensions/amp-analytics/0.1/transport.js

+    // DO NOT MERGE THIS
+    // Warning: the scriptSrc URL below is only temporary. Don't merge
+    // before getting resolution on that.
+    const frame = createElementWithAttributes(ampDoc, 'iframe', {


Pointing out this comment again since previous one got marked outdated. In previous thread, I suggested discussing what the actual value of scriptSrc should be.

jonkeller · 2017-05-18T02:09:54Z

extensions/amp-analytics/0.1/transport.js

+  static sendToCrossDomainIframe_(frame, messages) {
+    // DO NOT MERGE THIS
+    // Warning: the following code is likely only temporary. Don't check
+    // in before getting resolution on that.


Pointing out this comment again since previous one got marked outdated. Flip side of the one in examples/analytics-3p-remote-frame-helper.js

…e specified in vendors.js, allows iframe response to creative

jonkeller · 2017-05-19T13:57:35Z

@lannka Here is the tracking issue you requested: #9433

keithwrightbos · 2017-05-23T13:55:06Z

examples/analytics-3p-remote-frame-helper.js

+class AmpAnalyticsRemoteFrameManager {
+  constructor() {
+    /**
+     * @type {Function}


Looks like this function expects a parameter, if so please tighten up the typing (e.g. ?function<!Array> or similar)

keithwrightbos · 2017-05-23T13:56:28Z

examples/analytics-3p-remote-frame-helper.js

+  window.onNewAmpAnalyticsInstance(remoteFrameMgr_);
+  // Warning: the following code is likely only temporary. Don't check in
+  // before getting resolution on that.
+  window.addEventListener("message", (msg) => {


single parameter functions do not require parens here and throughout

keithwrightbos · 2017-05-23T13:57:14Z

examples/analytics-3p-remote-frame-helper.js

+ */
+const remoteFrameMgr_ = new AmpAnalyticsRemoteFrameManager();
+
+window.requestIdleCallback = window.requestIdleCallback || function(cb) {


window.requestIdleCallback = window.requestIdleCallback || cb => setTimeout(cb, 1);

keithwrightbos · 2017-05-23T13:58:39Z

extensions/amp-analytics/0.1/amp-analytics.js

@@ -220,6 +228,19 @@ export class AmpAnalytics extends AMP.BaseElement {
    this.analyticsGroup_ =
        this.instrumentation_.createAnalyticsGroup(this.element);

+    if (this.config_['transport'] && this.config_['transport']['iframe']) {
+      Transport.processCrossDomainIframe(this.getAmpDoc().win.document,


Can this just be this.win.document or this.element.ownerDocument? Or is this required to get access to the parent window when the creative is in a friendly frame?

I believe is required to get that access.

keithwrightbos · 2017-05-23T14:00:13Z

extensions/amp-analytics/0.1/transport.js

+   */
+  static sendRequestUsingCrossDomainIframe(request, transportOptions) {
+    const frameUrl = Transport.appendHashToUrl_(transportOptions['iframe'],
+      transportOptions['dataHash']);


No need for dataHash anymore, right?

Correct, that is the next step, but not part of the current commit.

keithwrightbos · 2017-05-23T14:21:07Z

extensions/amp-analytics/0.1/transport.js

+      ampDoc.body.appendChild(frame);
+    }
+    if (processResponse) {
+      window.addEventListener("message", (msg) => {


message should be single quoted. Make sure to run gulp lint and gulp check-types periodically

keithwrightbos · 2017-05-23T14:22:59Z

extensions/amp-analytics/0.1/amp-analytics.js

+        this.config_['transport'],
+        (msg) => {
+          try {
+            if (this.element.ownerDocument.location.href !== 'about:srcdoc') {


This is to allow the creative to communicate with the amp-analytics instance? Surely this code is temporary as otherwise it would be XSS? Add TODO/DO NOT SUBMIT?

keithwrightbos · 2017-05-23T14:28:51Z

extensions/amp-analytics/0.1/amp-analytics.js

+          inlineConfig.transport.iframe) ||
+         (this.remoteConfig_ && this.remoteConfig_.transport &&
+          this.remoteConfig_.transport.iframe))) {
+      user().error(this.getName_(), 'transport/iframe may only be specified' +


You need to actually return here or otherwise prevent this situation. As is, I believe this would allow the code to continue to be executed.

Ah, I thought user().error() throws. Will add return.

keithwrightbos · 2017-05-23T14:36:02Z

extensions/amp-analytics/0.1/amp-analytics.js

@@ -398,6 +419,18 @@ export class AmpAnalytics extends AMP.BaseElement {
    }
    const typeConfig = this.predefinedConfig_[type] || {};

+    // transport.iframe is only allowed to be specified in typeConfig, not
+    // the others. Allowed when running locally for testing purposes.
+    if (!getMode().localDev &&


This feels a bit more readable but up to you:

let hasFrame = false;
[defaultConfig, inlineConfig, this.remoteConfig_].forEach(config => {
if (config && config.transport && config.transport.iframe) {
user().error('invalid config contains iframe transport', config);
// WARN but allow in dev mode.
hasFrame = getMode().localDev;
}
});
if (hasFrame) {
... error and prevent execution
}

keithwrightbos · 2017-05-23T14:37:35Z

extensions/amp-analytics/0.1/transport.js

+    const frameData = Transport.crossDomainFrames[frameUrl];
+    frameData.isReady = true;
+    this.sendToCrossDomainIframe_(frameData.frame, frameData.msgQueue);
+    delete frameData.msgQueue;


Are you sure you want to delete it? What about grouping/throttling?

Yeah, will need to re-add the queue when throttling is implemented in a future phase. Though there is no need for it in the interim.

jonkeller · 2017-05-24T19:06:58Z

FYI this PR is old and crufty. I plan to just delete it and make a new one towards the end of the week.

jonkeller requested review from keithwrightbos and tdrl April 22, 2017 15:10

jonkeller commented Apr 22, 2017

View reviewed changes

jonkeller commented Apr 23, 2017

View reviewed changes

keithwrightbos requested review from bradfrizzell and removed request for tdrl April 24, 2017 14:09

bradfrizzell reviewed Apr 24, 2017

View reviewed changes

jonkeller mentioned this pull request Apr 25, 2017

amp-ad-network-fake-impl is broken #8935

Closed

jridgewell requested changes Apr 25, 2017

View reviewed changes

jonkeller commented Apr 25, 2017

View reviewed changes

bradfrizzell suggested changes Apr 26, 2017

View reviewed changes

keithwrightbos requested changes Apr 28, 2017

View reviewed changes

jonkeller commented May 12, 2017

View reviewed changes

googlebot added the cla: yes label May 12, 2017

jonkeller added 6 commits May 12, 2017 15:13

Checkpointing before picking this back up after working on other stuf…

083b276

…f for a few days

Initial implementation of 3p vendor analytics xframes

b0cf86d

Review feedback

ae7603b

Addresses most review feedback

7a4645d

Refactor into transport

cecb707

Checkpoint before rebasing

4b0f894

Checkpoint after fixing everything that rebasing broke

c7e2d04

Review feedback

3708c57

jonkeller commented May 18, 2017

View reviewed changes

Adds frame deprioritization/deallocation, allows iframe URL to only b…

f8ec3a1

…e specified in vendors.js, allows iframe response to creative

Teardown unused frame, polyfill requestIdleCallback, refactor usageCount

deb29e9

keithwrightbos requested changes May 23, 2017

View reviewed changes

Review feedback

0b87eef

jonkeller closed this May 24, 2017

		@@ -0,0 +1 @@
		<!-- No, this is not actually JSON. Fake ad network is kind of broken and should be fixed separately. --><!doctype html><html amp4ads> <head> <meta charset=utf-8> <script async src='https://cdn.ampproject.org/v0.js'></script> <script async custom-element=amp-analytics src='https://cdn.ampproject.org/v0/amp-analytics-0.1.js'></script> <style amp4ads-boilerplate>body{visibility:hidden}</style> <meta name=viewport content=width=device-width,minimum-scale=1> </head> <body> <amp-img id=img src='https://upload.wikimedia.org/wikipedia/commons/6/6e/Golde33443.jpg' width=256 height=300></amp-img> <amp-analytics> <script type='application/json'> { "requestsFrameUrl": "http://localhost:8000/examples/analytics-3p-remote-frame.html", "dataHash": "key1=val1&key2=val2", "requests": { "pageview": "https://example.com/analytics?viewed=true&url=${canonicalUrl}&title=${title}&acct=${account}", "event": "https://example.com/analytics?eid=click&acct=${account}" }, "vars": { "account": "ABC123" }, "triggers": { "defaultPageview": { "on": "visible", "request": "pageview" }, "anchorClicks": { "on": "click", "selector": "img", "request": "event" } } } </script> </amp-analytics> </body></html>

Amp-Analytics 3p vendor obfuscation #8887

Amp-Analytics 3p vendor obfuscation #8887

Conversation

jonkeller commented Apr 22, 2017 • edited by jridgewell Loading

Choose a reason for hiding this comment

jonkeller Apr 22, 2017 • edited Loading

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 commented Apr 24, 2017

Choose a reason for hiding this comment

jonkeller Apr 24, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradfrizzell left a 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

jonkeller commented Apr 26, 2017

keithwrightbos left a 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

jonkeller commented Apr 22, 2017 •

edited by jridgewell

Loading

jonkeller Apr 22, 2017 •

edited

Loading

jonkeller Apr 24, 2017 •

edited

Loading