Amp analytics 3p vendors #9661

jonkeller · 2017-06-01T14:50:27Z

Enables amp-analytics tag to use third-party vendors. These vendors' code will be run in a non-AMP iframe.

One-pager: https://docs.google.com/document/d/1mCW7HbQBWQONTWA2q_rKNJyTEdqGOyhX-x-ces4JHWc/edit#

jonkeller · 2017-06-01T14:51:27Z

build-system/dep-check-config.js

@@ -148,6 +148,10 @@ exports.rules = [
  {
    filesMatching: '{src,extensions}/**/*.js',
    mustNotDependOn: '3p/**/*.js',
+    whitelist: [
+      'extensions/amp-analytics/0.1/transport.js->' +
+        '3p/iframe-messaging-client.js',


This exception is added because the amp-analytics tag needs to send messages to the third-party iframe. It covers the import of the file that contains sendMessage().

jonkeller · 2017-06-01T14:52:12Z

build-system/tasks/presubmit-checks.js

@@ -317,6 +317,7 @@ var forbiddenTerms = {
      '3p/ampcontext.js',
      '3p/ampcontext-integration.js',
      'dist.3p/current/integration.js', // includes previous
+      'extensions/amp-analytics/0.1/transport.js',


This exception is added because the amp-analytics tag needs to send messages to the third-party iframe. It covers the actual calls to the sendMessage() method.

jonkeller · 2017-06-01T14:53:21Z

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

@@ -0,0 +1,188 @@
+/**


Upon approval of its content, this file will be placed at a public/static URL. In so doing, we'll include a small set of other AMP files, which will resolve (or enable to be resolved) the TODOs on lines 17, 44 and 139.

Done, pending separate code review

jonkeller · 2017-06-01T14:57:16Z

extensions/amp-analytics/0.1/transport.js

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


As mentioned in the comments of /examples/analytics-3p-remote-frame-helper.js, upon approval that frame will be placed at a public/static URL. Once that is done, the value on line 249 must change to that URL.

This should be an absolute path

jonkeller · 2017-06-01T15:02:02Z

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

+   */
+  processCrossDomainIframeResponse_(msg) {
+    // Used in ../../../src/anchor-click-interceptor.js
+    this.win.document.mostRecentCrossDomainIframeResponse = msg;


^^ Reviewers: extra attention requested on this line. Is there a better place to store this? The logic is that, when there are multiple ads with amp-analytics tags on the page, then if the user clicks on ad A, we want to use the most recent response from ad A. But if they click on ad B, we want to use the most recent response from ad B. So I've stored the response as an attribute on the ad's document (not the containg AMP publisher page). Then anchorClickInterceptor pulls it from the event target's document.

You should be able to do this with an enclosed object as a field which maps the creative to its last message, no?

True, but enclosed in what? AnchorClickInterceptor knows what "A" tag has been clicked, and can get from there to the enclosing document in O(1) time. But if I store it in the amp-analytics tag then we'd have to go walk the DOM tree to find it at the time of the click.

Done. Note that it's enclosed in the anchor click listener not amp-analytics, to avoid anchor click listener having a dependency on amp-analytics which lives in extensions.

jonkeller · 2017-06-01T15:03:05Z

extensions/amp-analytics/0.1/transport.js

- * @param {string} request
- * @param {!Object<string, string>} transportOptions
- */
-export function sendRequest(win, request, transportOptions) {


Not deleted, just moved inside the Transport class below, because needs to maintain extraData as non-static.

jonkeller · 2017-06-01T15:07:08Z

extensions/amp-analytics/0.1/transport.js

+      if (!frameData.isReady) {
+        timerFor(window).delay(() => {
+          this.sendExtraData(frameUrl, extraData);
+        }, 10);


The issue here is that the sequence could go:

Ad A amp-analytics tag calls processCrossDomainIframe() (line 164).

That calls createCrossDomainIframe_(), which sets up the frame load promise (line 254)

Ad B amp-analytics tag calls processCrossDomainIframe() too, before the load promise from Remove usages of for-of #2 has fired. So there is no point in doing sendMessage() (line 232) yet since the recipient isn't listening yet.
I'd like to explore chaining the loadPromise somehow but personally I see this as an optimization that is not necessary for phase 1. Please let me know if there is disagreement here.

Couple of things:

After consideration, I do not want to use load as an indicator that the slot is ready. Instead assume library within the frame will send a message indicating it is ready to which you will response

Given Add commit hooks #1, you should have sendExtraData (or send any post messages) store pending messages in a queue and send them upon receiving the ready message (you should consider grouping into a single message to ensure no potential race conditions though I assume post message receiver would always get them in the order they were sent)

The ready signal should trigger the queue to be flushed for all creatives waiting on the frame

Waiting for an arbitrary 10 ms seems odd...

Ok, cool. Am already doing the queue idea for event messages, but not for extraData. Will not be difficult to switch to using the queue for all. Will also not be difficult to switch from load to a proactive send.

keithwrightbos · 2017-06-01T15:32:54Z

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

+ * limitations under the License.
+ */
+
+const RTV_VERSION = '1234'; // TODO Replace with import from AMP, or just remove


I believe you can just put $internalRuntimeVersion$ and the compiler will macro it in for you

Done (removed entire line)

keithwrightbos · 2017-06-01T15:34:50Z

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

+class AmpAnalyticsRemoteFrameManager {
+  constructor() {
+    /**
+     * @type {Function<!String>}


type def should be: {!function(string)} but I'm curious why you bother with an empty function? Would seem better to allow these to be null by default and then only bother calling them when they are not

Ok, can change.
(I did it that way because "if (listener_) { listener_(); }" or "listener_ && listener_()" seemed uglier than what's here IMO)

keithwrightbos · 2017-06-01T15:35:34Z

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

+   * @param {!String} message The message that was received.
+   */
+  receiveMessageFromCreative(message) {
+    if (message && message.data) {


I prefer returns in order to remove indents. Something like:

if (!message || !message.data) {
return;
}
if (!deserialized) {
return;
}

Done, here and elsewhere

keithwrightbos · 2017-06-01T15:36:20Z

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

+   */
+  setSentinel_(sentinel) {
+    if (this.sentinel_ && sentinel != this.sentinel_) {
+      console.warn("Attempting to set sentinel to " + sentinel + " when it" +


dev().warn and use single quotes for strings

Will fix (in concert with bringing in the AMP imports - as-is, dev() is not defined yet)

Done but temporarily added function dev() { return console; } at the bottom of the page, which can stay until I do the AMP imports.

Done completely now

keithwrightbos · 2017-06-01T15:37:10Z

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

+      console.warn("Attempting to set sentinel to " + sentinel + " when it" +
+          " was already set to " + this.sentinel_);
+    }
+    this.sentinel_ = sentinel;


Is this safe? It implies that you'll honor the first sentinel you receive? Would be preferable to set the sentinel at the time you create the analytics instance as presumably you're passing it to the 3p frame

The sentinel is created in transport.js, in createCrossDomainIframe():
iframeMessagingClient.setSentinel(Transport.createUniqueId_());
So each frame (since we may have multiple vendors) gets a unique sentinel, which associated to the frame URL in a map that is static to the Transport class. And then this is indeed passed to the 3p frame (this file is running in the 3p frame).

Fixed - now set sentinel on frame's name attr at creation time instead.

keithwrightbos · 2017-06-01T15:49:20Z

extensions/amp-analytics/0.1/transport.js

+      Transport.incrementFrameUsageCount_(frameUrl);
+      this.sendExtraData(frameUrl, transportOptions['extraData']);
+    } else {
+      const frame = this.createCrossDomainIframe_(ampDoc, frameUrl,


Consider creating getOrCreateCrossDomainFrame which combines this logic in an easily testable location?

Done. Called it beginUsingCrossDomainIframe_() since get...() implies a return value and none was needed. Name pairs with existing doneUsing...()

keithwrightbos · 2017-06-01T15:49:37Z

extensions/amp-analytics/0.1/transport.js

+    // Regardless of whether we just created it, or are re-using an existing
+    // one, wire up the response callback
+    if (processResponse) {
+      const imc = Transport.crossDomainIframes_[frameUrl].iframeMessagingClient;


give this variable a better name

Yeah, I also dislike the current name. The line is precisely 80 chars, hence choosing something with length=3. Will switch to better name and just bite the bullet and carriage-return on "."

keithwrightbos · 2017-06-01T16:43:08Z

extensions/amp-analytics/0.1/transport.js

+   * Sends extra data (from the amp-analytics config object) to the
+   * cross-domain iframe.
+   * @param {!string} frameUrl The URL of the frame to send the data to
+   * @param {?string=} extraData The data to send to the frame


is ? needed (would you expect null?) and variable name should start with opt_

Done, here and elsewhere.

keithwrightbos · 2017-06-01T16:47:59Z

extensions/amp-analytics/0.1/transport.js

+      if (!frameData.isReady) {
+        timerFor(window).delay(() => {
+          this.sendExtraData(frameUrl, extraData);
+        }, 10);


Couple of things:

After consideration, I do not want to use load as an indicator that the slot is ready. Instead assume library within the frame will send a message indicating it is ready to which you will response

Given Add commit hooks #1, you should have sendExtraData (or send any post messages) store pending messages in a queue and send them upon receiving the ready message (you should consider grouping into a single message to ensure no potential race conditions though I assume post message receiver would always get them in the order they were sent)

The ready signal should trigger the queue to be flushed for all creatives waiting on the frame

Waiting for an arbitrary 10 ms seems odd...

keithwrightbos · 2017-06-01T16:48:19Z

extensions/amp-analytics/0.1/transport.js

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


This should be an absolute path

jonkeller · 2017-06-04T23:54:27Z

gulpfile.js

@@ -228,6 +228,18 @@ function compile(watch, shouldMinify, opt_preventRemoveAndMakeDir,
      includePolyfills: false,
    }),

+    compileJs('./3p/', 'ampanalytics-lib.js',


TODO: This file was changed by another PR which will require resolving

keithwrightbos

I didn't complete the review but some initial proposed changes to the API and flow

keithwrightbos · 2017-06-02T14:39:45Z

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

+class AmpAnalyticsRemoteFrameManager {
+  constructor() {
+    /**
+     * @type {function<!string>}


{?function}

...I think you may be reviewing an old version of the code. This region of this file currently looks like:
class AmpAnalytics3pRemoteFrameHelper {
constructor(win) {
/**
* @type {!Array<function(string)>}
* @Private
*/
this.eventsListeners_ = [];

...in fact, this file was renamed to /3p/ampanalytics-lib.js so you're definitely looking at old stuff.

keithwrightbos · 2017-06-02T14:39:55Z

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

+    this.listener_ = null;
+
+    /**
+     * @type {Function<!String>}


{?function}

keithwrightbos · 2017-06-02T14:40:09Z

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

+
+    /**
+     * Holds a mapping between sender ID and extra data
+     * @type {Map}


Why map as opposed to just object? note that @Private can set type and in this case it should not be nullable: @Private {!Map} Also can you type the contents of the map (e.g. !Map<string, string>)?

...and actually I just changed all my instances of Map back to Object, because now that my "gulp check-types" is working correctly again, it complains about those:
ERROR - Cannot do '[]' access on a struct

keithwrightbos · 2017-06-02T14:42:31Z

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

+
+  /**
+   * Registers a callback function to be called when AMP Analytics events occur
+   * @param {!Function} listener A function that takes an array of event


types should match field types. Any reason this couldn't be part of the constructor? If a frame is loading the library, I think we can assume it wants to listen to any inbound messages.

Would be happy to make this change. But, you previously preferred the frame to contain this:
window.onNewAmpAnalyticsInstance = ampAnalytics => {
ampAnalytics.registerAmpAnalytics3pEventsListener(events => {
...which means that ampAnalytics must exist in order to be passed to the frame code. Is this no longer preferable?

keithwrightbos · 2017-06-02T14:43:24Z

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

+       * passing your function which will receive the analytics events.
+       */
+      window.onNewAmpAnalyticsInstance = ampAnalytics => {
+        ampAnalytics.registerAmpAnalyticsEventListener(


I was thinking a better name would be onAmpAnalyticsEvents

keithwrightbos · 2017-06-06T13:22:11Z

extensions/amp-analytics/0.1/transport.js

+    const frameData = Transport.crossDomainIframes_[frameUrl];
+    const iframeMessagingClient = frameData.iframeMessagingClient;
+    iframeMessagingClient.registerCallback(
+      'ampAnalytics3pReady', () => {


Ideally the enum declaring these message types would be in a common location shared between AMP analytics and the library to ensure they remain consistent

keithwrightbos · 2017-06-06T13:22:55Z

extensions/amp-analytics/0.1/transport.js

+    }
+    iframeMessagingClient.registerCallback(
+      'ampAnalytics3pResponse', response => {
+        if (!response || !response.ampAnalytics3pResponse) {


Should this be dev().assert(response && response.ampAnalytics3pResponse) as I would assume you expect them to be present?

keithwrightbos · 2017-06-06T13:23:54Z

extensions/amp-analytics/0.1/transport.js

+        if (!response || !response.ampAnalytics3pResponse) {
+          return;
+        }
+        opt_processResponse(this.type_, response.ampAnalytics3pResponse);


Consider removing opt_processResponse existence check and instead put it here such that if you receive a message you did not expect, you do dev().warn('unexpected response...') so that developers can get feedback as to why integration is not working

keithwrightbos · 2017-06-06T13:26:24Z

3p/ampanalytics-lib.js

+if (window.onNewAmpAnalyticsInstance) {
+  window.onNewAmpAnalyticsInstance(remoteFrameHelper_);
+} else {
+  dev().error(TAG_, 'Vendor page must implement onNewAmpAnalyticsInstance.');


I would add ' prior to loading library script.' I would also advocate that we wait to create the remoteFrameHelper instance until we get a message indicating a new amp analytics instance has been created as we don't need it until then. The flow is:

create xdomain frame for provider passing sentinel and lib source path in name

xdomain frame executes by loading script which send out 'ready' post message

AMP analytics responds to ready post message by sending 'new instance' message (with any additional state)

xdomain frame receives 'new instance' message (which I believe you're referring to as "extraData"?) and calls window.onNewAmpAnalyticsInstance

For any additional ad slots that use the same vendor, window.onNewAmpAnalyticsInstance would be called again and should use the latest value (e.g. a vendor could re-assign onNewAmpAnalyticsInstance between slot instances which is ugly but not impossible).

We need to move away from using an extraData listener and instead you should call onNewAmpAnalyticsInstance whenever you receive a "new slot" message. This will ensure the vendor can properly enclose a slot's state in a single call to onNewAmpAnalyticsInstance. Does that make sense?

keithwrightbos · 2017-06-06T13:29:03Z

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

+          });
+        });
+        // The following is optional:
+        ampAnalytics.registerAmpAnalytics3pExtraDataListener(extraDataMsgs => {


As mentioned, we should remove the registerAmpAnalytics3pExtraDataListener and instead you should either pass the initial state in onNewAmpAnalyticsInstance (e.g. (ampAnalytics, startUpState)) or make it a field of the ampAnalytics instance such that it has 1 field (startUpState) and 2 functions (onAmpAnalytics3pEvents & sendMessageToCreative)

How about I just pass it in the frame's name attribute?

jonkeller · 2017-06-07T13:29:25Z

gulpfile.js

+      externs: ['ads/ads.extern.js',],
+      include3pDirectories: true,
+      includePolyfills: false,
+    }),


The type checking issues I was having yesterday on unchanged files are fixed. I had duplicated the "closureCompile(['./3p/ampcontext-lib.js'], './dist'," block beginning on line 653 and changed "ampcontext" to "ampanalytics". It also made "gulp check-types" not report any typing errors in my actual code. Removing that fixed the compile issue, and allowed me to see my own typing errors again. I am currently investigating whether that code is necessary, and if so, whether its re-inclusion now that my typing errors are resolved will be better behaved.

Yep, putting it back in hoses things again. The block of code is:
closureCompile(['./3p/ampanalytics-lib.js'], './dist', 'ampanalytics-check-types.js', { externs: ['ads/ads.extern.js'], include3pDirectories: true, includePolyfills: true, checkTypes: true, }),
The errors are stuff like this:
src/services.js:225: ERROR - Bad type annotation. Unknown type module$src$service$template_impl.Templates return /** @type {!./service/template-impl.Templates} */ ( ^
(the caret points to the dot when not using a variable-width font...)

keithwrightbos · 2017-06-07T13:11:15Z

3p/ampanalytics-lib.js

+    /** @private {string} */
+    this.sentinel_ = null;
+
+    /** @private {Object<string, AmpAnalytics3pCreativeMessageRouter>} */


!Object<string, !AmpAnalytics3pCreativeMessageRouter>

keithwrightbos · 2017-06-07T13:11:55Z

3p/ampanalytics-lib.js

+    this.creativeMessageRouters_ = [];
+
+    /**
+     * Simple requestIdleCallback polyfill


native requestIdleCallback when available, setTimeout based polyfill otherwise

keithwrightbos · 2017-06-07T13:18:16Z

extensions/amp-analytics/0.1/transport.js

+    let newId;
+    do {
+      newId = String(Math.random()).substr(2);
+    } while (Transport.usedIds_[newId]);


let newId;
while (Transport.usedIds[(newId = String(Math.random()).substr(2))]);
Transport.usedIds_[newId] = true;
return newId;

Why do you substring? Possibly unending while loops make me nervous as they could cause the browser to hang. Just to be uber safe, let's bound this to a max # of iterations (say 20).

Substring: because without it the string will be like "0.xxxxxxxx" instead of "xxxxxxxx" and that looked ugly.
I agree we don't want it to hang. And likely the universe will end before it chooses and already-chosen number 20x in a row. But there is still the tiny chance that it will therefore return a number that is not unique. Are we okay with that miniscule risk?

I'm hoping we move to incrementing number but assuming we don't, I would argue the probability of an inadvertent, neverending loop is higher than 20x random not creating unique ID and as mentioned, you could append the type (e.g. moat) to ensure its truly unique.

Fair enough. Moved to incrementing number.

keithwrightbos · 2017-06-07T13:19:58Z

3p/ampanalytics-lib.js

+    /** @private {!Window} */
+    this.win_ = win;
+
+    /** @private {string} */


Can you add a comment that the sentinel is required by iframe messaging client and you use it to uniquely identify the frame as part of message routing? FWIW - you could have used the type given it must be unique, right?

Comment done.
Not sure what you mean about the type...a frame can send multiple responses. If the order goes:

creative 1 creates xframe

creative 2 wants to use xframe

xframe renders
...then xframe will send ready messages to both creatives in order to release their queues

keithwrightbos · 2017-06-07T13:20:49Z

3p/ampanalytics-lib.js

+     */
+    this.requestIdleCallback =
+      this.win_.requestIdleCallback.bind(this.win_) ||
+      (cb => setTimeout(cb, 1));


Any reason not use timeout delay of 0? I believe most browsers enforce a minimum of higher anyway (5 by my recollection)

keithwrightbos · 2017-06-07T14:08:43Z

extensions/amp-analytics/0.1/transport.js

+    iframeMessagingClient.registerCallback(
+      MessageTypes.ampAnalytics3pReady, () => {
+        iframeMessagingClient.setHostWindow(frameData.frame.contentWindow);
+        Transport.setIsReady_(frameUrl);


You have reference to frameData so might as well pass to isReady_ directly instead of imposing a lookup. In general, you should only ever need to lookup frameData once here.

keithwrightbos · 2017-06-07T14:12:21Z

extensions/amp-analytics/0.1/transport.js

+   * @param {string=} opt_extraData The data to send to the frame
+   * @private
+   */
+  beginUsingCrossDomainIframe_(ampDoc, frameUrl, opt_extraData) {


Have this return frameData so that you do not need to look it up again within processCrossDomainIframe.

keithwrightbos · 2017-06-07T14:18:06Z

extensions/amp-analytics/0.1/transport.js

+    if (frameData.sendTimer) {
+      return; // Timer is already about to fire, no need to set a new one
+    }
+    frameData.sendTimer = Transport.timer_.delay(() => {


The negative with this approach is that a new message will ALWAYS need to wait the throttle time even if there has been no messaging traffic. The intent here is to not send them to often so I suggest moving to an interval based solution which you clear when destroying the frameData (note that you currently do not actually cancel the timer, at least not that I could find nor do you attempt to flush any existing messages)

So you mean, send the first one immediately, and then set an invariant "I will not send again for at least 100ms", and if subsequent ones come in during that 100ms queue them until that time has elapsed? I like this.
(Currently implementation destroys the timer about 4 lines down from here, upon it firing.)

My point regarding destruction is that its possible you set the delay, unlayoutCallback executes clearing a bunch on state, and then the timeout runs leading to errors. To be safe, you should ensure that unlayoutCallback cancels the timer.

keithwrightbos · 2017-06-07T14:19:48Z

extensions/amp-analytics/0.1/transport.js

      removeElement(iframe);
    }, 5000);
  };
-  user().assert(
+  dev().assert(


Why did you downgrade to dev?

Didn't touch the sendRequestUsingIframe() method at all. Note that it is comparing line 141 to line 426, so I suspect that's just the diff tool trying to gain an efficiency when it shouldn't. Want me to change it to dev(), though?

Could it be an merge issue? I believe it should match whatever is in master.

Huh. It is user() in master...guess it was a merge issue after all. Fixed.

keithwrightbos · 2017-06-07T14:20:14Z

src/3p-analytics-common.js

+ */
+
+/** @private @const {Object<string,string>} */
+export const MessageTypes = {


Should not be plural and consider a name more specific to your usage case

keithwrightbos · 2017-06-08T13:50:40Z

3p/ampanalytics-lib.js

+     */
+    this.iframeMessagingClient_ = new IframeMessagingClient(win);
+    try {
+      const frameNameData = JSON.parse(this.win_.name);


If you move this parse up in the constructor, you can make sentinel a const and pass it directly to the frame. I'm curious why we don't allow passing sentinel in the constructor.

keithwrightbos · 2017-06-08T13:52:31Z

3p/ampanalytics-lib.js

+    this.iframeMessagingClient_ = new IframeMessagingClient(win);
+    try {
+      const frameNameData = JSON.parse(this.win_.name);
+      if (frameNameData.sentinel) {


currently if there is valid JSON in the name but no sentinel, the catch block does not execute

keithwrightbos · 2017-06-08T13:55:51Z

3p/ampanalytics-lib.js

+   */
+  receiveMessage(message) {
+    if (message[AMP_ANALYTICS_3P_MESSAGE_TYPE.NEW]) {
+      this.extraData_ = message[AMP_ANALYTICS_3P_MESSAGE_TYPE.NEW];


Ideally onNewAmpAnalyticsInstance is called here but I believe you're working on that.

Correct. Done but not committed/pushed yet.

keithwrightbos · 2017-06-08T13:56:21Z

3p/ampanalytics-lib.js

+   * in the queue.
+   */
+  sendQueuedMessagesToListeners() {
+    if (this.receivedMessagesQueue_.length == 0 ||


FYI, You don't need the == 0, can instead do !this.receivedMessagesQueue_

I have switched to (!this.receivedMessagesQueue.length) but what you said is not correct:

var foo = [];
if (foo) { console.log('truthy'); } else { console.log('falsey'); }
truthy

keithwrightbos · 2017-06-08T13:57:38Z

3p/ampanalytics-lib.js

+      this.eventListeners_.length == 0) {
+      return;
+    }
+    this.eventListeners_.forEach(listener => {


this.eventListeners_.forEach(listener => listener(this. receivedMessagesQueue_));

We should consider wrapping each listener call with try/catch as otherwise the first throwing would not only cause the rest not to execute, it would cause the queue not to be cleared.

Moot in this particular location due to refactor, but done in new location.

keithwrightbos · 2017-06-08T13:58:24Z

3p/iframe-messaging-client.js

@@ -33,7 +33,7 @@ export class IframeMessagingClient {
    this.win_ = win;
    /** @private {?string} */
    this.rtvVersion_ = getMode().rtvVersion || null;
-    /** @private {!Window} */
+    /** @private {!(Window|Element)} */


Is there a type specific to frames?

Tried this originally, but unfortunately there is not.
`
3p/iframe-messaging-client.js:131: ERROR - Bad type annotation. Unknown type Frame

@param {!(Window|Frame)} win The window to communicate with. This may
`

try HTMLIFrameElement

Done. (Where did you find that, so I don't have to bug you next time I need to type something as a checkbox or whatever?)

jonkeller · 2017-06-08T21:17:56Z

3p/ampanalytics-lib.js

+          'Received empty events envelope');
+        // Iterate through the array of events, dispatching each to the
+        // listener for the appropriate creative
+        envelope[AMP_ANALYTICS_3P_MESSAGE_TYPE.EVENTS].forEach(event => {


Need to loop through them here, because they may have come from different creatives

jonkeller · 2017-06-08T21:18:48Z

3p/ampanalytics-lib.js

+initLogConstructor();
+// TODO(alanorozco): Refactor src/error.reportError so it does not contain big
+// transitive dependencies and can be included here.
+setReportError(() => {});


Re: your suggestion over lunch, lines 23-25 are from ampcontext, and fix the issue of assert() complaining that ampdoc does not exist.

jonkeller · 2017-06-08T21:19:51Z

extensions/amp-analytics/0.1/cross-domain-iframe-message-queue.js

+    /** @type {Array<(
+    *            ../../../src/3p-analytics-common.ampAnalytics3pEvent|
+    *            ../../../src/3p-analytics-common.ampAnalytics3pNewCreative
+    *          )>} */


I find this preferable to a linter exception to the 80-char line, but if you feel otherwise I am happy to change it.

jonkeller · 2017-06-08T21:21:16Z

extensions/amp-analytics/0.1/cross-domain-iframe-message-queue.js

@@ -0,0 +1,164 @@
+/**


Refactored the message queueing/sending stuff from transport.js to here.

jonkeller · 2017-06-08T21:22:52Z

extensions/amp-analytics/0.1/cross-domain-iframe-message-queue.js

+    const typedMessageObject =
+      /** @type {../../../src/3p-analytics-common.ampAnalytics3pEvent} */
+      (messageObject);
+    return typedMessageObject;


Did it this way because if you do:

return /** @type {../../../src/3p-analytics-common.ampAnalytics3pEvent} */ (messageObject);

...then it interprets the "return" as a complete statement with no semicolon, and the subsequent two lines are unreachable code.

jonkeller · 2017-06-08T21:25:23Z

gulpfile.js

+          includePolyfills: true,
+          checkTypes: true,
+        }),
+      */


Uncomment, or delete?

jonkeller · 2017-06-08T21:27:10Z

src/3p-analytics-common.js

+ * limitations under the License.
+ */
+
+// TODO(jonkeller): Extern this


It's already externed, just a vestigial comment. Removed comment already...will commit/push with next set of changes, if any.

keithwrightbos · 2017-06-10T14:06:22Z

3p/ampanalytics-lib.js

+ * Receives messages bound for this cross-domain iframe, from all creatives
+ */
+class AmpAnalytics3pMessageRouter {
+  /** @param {!Window} win */


Nit: add line here and remove first, empty line in constructor

keithwrightbos · 2017-06-10T14:13:06Z

3p/ampanalytics-lib.js

+      // The sentinel is required by iframe-messaging-client, and is used to
+      // uniquely identify the frame as part of message routing
+      /** @const {string} */
+      this.sentinel_ = frameNameData.sentinel;


I would move this outside of the try/catch and change to this.sentinel_ = frameNameData && frameNameData.sentinel;

keithwrightbos · 2017-06-10T14:13:33Z

3p/ampanalytics-lib.js

+      const frameNameData = JSON.parse(this.win_.name);
+      if (!frameNameData.sentinel) {
+        const ex = {message: 'JSON parsed, but did not include sentinel.'};
+        throw ex;


Should be throw new Error('JSON parsed...');

keithwrightbos · 2017-06-10T14:14:43Z

3p/ampanalytics-lib.js

+      return;
+    }
+
+    /** @private {!Object<string, !AmpAnalytics3pCreativeMessageRouter>} */


Add some documentation explaining the purpose of this object and the value of its keys.

keithwrightbos · 2017-06-10T14:15:21Z

3p/ampanalytics-lib.js

+    this.iframeMessagingClient_.setSentinel(this.sentinel_);
+    this.iframeMessagingClient_.registerCallback(
+      AMP_ANALYTICS_3P_MESSAGE_TYPE.CREATIVES,
+      envelope => {


envelope is an odd variable name IMO. What exactly is the purpose of this object? What are its values?

I meant it in a similar sense to its use in https://docs.oracle.com/cd/E19509-01/820-4408/gguuw/index.html or https://www.tutorialspoint.com/soap/soap_message_structure.htm but somewhat less formal - it's just what I always use to name something whose sole purpose is to contain other thing(s) during transit (since that's the purpose of a real-world envelope). Happy to change if you prefer.

keithwrightbos · 2017-06-10T14:35:51Z

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

@@ -278,6 +298,17 @@ export class AmpAnalytics extends AMP.BaseElement {
  }

  /**
+   * Receives any response that may be sent from the cross-domain iframe
+   * @param {!string} type The type parameter of the cross-domain iframe
+   * @param {Object} response The response message from the iframe


Can the response be typed to a known def?

Done, but to be honest I don't like it. The iframe creates an arbitrary object, which is passed to sendMessageToCreative(), which then wraps that in a AmpAnalytics3pResponse. I liked the symmetry of the amp-analytics tag receiving exactly what the iframe created, with Transport doing the unwrapping. Now that AmpAnalytics.processCrossDomainIframeResponse_() is receiving an AmpAnalytics3pResponse, that symmetry is broken. (We could restore it by requiring the iframe to create the AmpAnalytics3pResponse directly, but I am less in favor of that.)

keithwrightbos · 2017-06-10T14:36:36Z

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

@@ -748,8 +796,8 @@ export class AmpAnalytics extends AMP.BaseElement {
   * Merges two objects. If the value is array or plain object, the values are
   * merged otherwise the value is overwritten.
   *
-   * @param {Object|Array} from Object or array to merge from
-   * @param {Object|Array} to Object or Array to merge into
+   * @param {(Object|Array)} from Object or array to merge from


Why add parens here?

Removed.
(The "why" is that at Taymon's suggestion, I tried this when I was experiencing all those type-checking errors last week in files that I didn't touch. They didn't fix it, and I later found something else which did, but did not remove these parens.)

keithwrightbos · 2017-06-10T14:36:42Z

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

@@ -791,8 +839,8 @@ export class AmpAnalytics extends AMP.BaseElement {
  }

  /**
-   * @param {!Object<string, Object<string, string|Array<string>>>} source1
-   * @param {!Object<string, Object<string, string|Array<string>>>} source2
+   * @param {!Object<string, Object<string, (string|Array<string>)>>} source1


keithwrightbos · 2017-06-10T14:37:14Z

extensions/amp-analytics/0.1/cross-domain-iframe-message-queue.js

+   * contain the queued messages (see 3p-analytics-common)
+   */
+  constructor(iframeMessagingClient, envelopeType) {
+    /** @type {Array<(


keithwrightbos · 2017-06-10T14:39:53Z

extensions/amp-analytics/0.1/cross-domain-iframe-message-queue.js

+    if (this.timerId_) {
+      return; // Timer is already about to fire, no need to set a new one
+    }
+    this.timerId_ = CrossDomainIframeMessageQueue.timer_.delay(() => {


This still suffers from the same problem where message after long inactivity will result in it being unnecessarily delayed. I recommend moving to a setInterval approach which does nothing if the queue is empty. Seems simpler to me and gives the correct behavior of throttling.

I agree that my method unnecessarily delays the first message after a long delay.
But I think that your proposed method is also (albeit less) wasteful, in that you're going to have an interval go off frequently when I bet most of the time there will be no work for it to do.
What about an approach where the first message gets sent immediately, and then we say that no more messages will be sent for the next x milliseconds (we'll queue them and send them once that expires). Something like this pseudocode:

if (door is unlocked) { send the message NOW lock the door setTimeout({ unlock the door; flush the queue; }, 100ms); } else { enqueue the message }

keithwrightbos

Looking good, needs tests and I tend to agree that the abstract queue arrangement seems a bit over engineered.

keithwrightbos · 2017-06-14T13:50:38Z

3p/ampanalytics-lib.js

+      if (!frameNameData.sentinel) {
+        throw new Error('JSON parsed, but did not include sentinel.');
+      }
+      // The sentinel is required by iframe-messaging-client, and is used to


This should be moved to just above the this.sentinel_ line

Oops. Done.

keithwrightbos · 2017-06-14T14:22:07Z

3p/ampanalytics-lib.js

+    let frameNameData = null;
+    try {
+      frameNameData = JSON.parse(this.win_.name);
+      if (!frameNameData.sentinel) {


I would structure this slightly differently:

let frameNameData = null;
try { frameNameData = JSON.parse(this.win_.name); } catch (e) {}
this.sentinel_ = user().assertString(frameNameData && frameNameData.sentinel, 'Invalid/missing sentinel on iframe name attribute', this.win_.name);
if (!this.sentinel_) { return; }

Done. (Note that assertString() only takes 2 args. So changed your last comma to a + )

keithwrightbos · 2017-06-14T14:23:45Z

3p/ampanalytics-lib.js

+    this.iframeMessagingClient_.registerCallback(
+      AMP_ANALYTICS_3P_MESSAGE_TYPE.CREATIVE,
+      messageContainer => {
+        dev().assert(messageContainer.data,


Consider checking empty array case: dev().assert(messageContainer.data && messageContainer.data.length)

Done (Object.entries(messageContainer.data).length since it's an object not an array)

keithwrightbos · 2017-06-14T14:24:26Z

3p/ampanalytics-lib.js

+        Object.entries(messageContainer.data).forEach(entry => {
+          const creativeId = entry[0];
+          const extraData = entry[1];
+          if (this.creativeMessageRouters_.hasOwnProperty(creativeId)) {


Can collapse to: dev().assert(!this.creativeMessageRouters_[creativeId])

keithwrightbos · 2017-06-14T14:25:07Z

3p/ampanalytics-lib.js

+            dev().warn(TAG_, 'Duplicate new creative message for ' + creativeId);
+          }
+          this.creativeMessageRouters_[creativeId] =
+            this.creativeMessageRouters_[creativeId] ||


No need to do || case as this.creativeMessageRouters_[creativeId] should not exist

keithwrightbos · 2017-06-14T14:52:01Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+    this.throttledFlushQueue_ = throttle(this.win_, () => {
+      this.flushQueue_();
+    }, MESSAGE_THROTTLE_TIME_);
+


Nit: remove extra line

keithwrightbos · 2017-06-14T14:52:23Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * @private
+   */
+  flushQueue_() {
+    if (this.isReady_ && Object.keys(this.store_).length > 0) {


greater than 0 is not necessary

keithwrightbos · 2017-06-14T14:53:21Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * iframe
+   */
+  enqueue(senderId, opt_data) {
+    if (this.store_.hasOwnProperty(senderId)) {


This should never happen, right?

Correct. Will change to assert() instead of warn().

keithwrightbos · 2017-06-14T14:53:33Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+      dev().warn(TAG_, 'Replacing existing extra data for: ' + senderId);
+    }
+    this.store_[senderId] = opt_data || '';
+


Nit: remove extra line

keithwrightbos · 2017-06-14T14:55:26Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * @param {string=} opt_data The data to be enqueued and then sent to the
+   * iframe
+   */
+  enqueue(senderId, opt_data) {


It looks like both implement unqueue so this should be part of your abstract class

Intentional. They implement it differently, since for new creatives each entry in .creativeToPendingMessages_ is <string, string> since you can only have one extraData per creative, whereas for events it's <string, Array < string >> since one creative can have any number of events. I originally had just the parent class with no subclasses, and this is actually what made me split it into the current class structure.

keithwrightbos

Looking good. Just needs unit test coverage and try to remove ResponseMap.

keithwrightbos · 2017-06-14T19:21:48Z

3p/ampanalytics-lib.js

+    // The sentinel is required by iframe-messaging-client, and is used to
+    // uniquely identify the frame as part of message routing
+    /** @const {string} */
+    this.sentinel_ = dev().assertString(


Sorry you can further reduce code here by using src/json#tryParseJson:

this.sentinel_ = dev().assertString(tryParseJson(this.win_.name, {}).sentinel, 'Invalid/missing sentinel on iframe name attribute' + this.win_.name);
if (!this.sentinel_) {
return;
}

Cool find! Done.

keithwrightbos · 2017-06-14T19:23:16Z

3p/ampanalytics-lib.js

+      AMP_ANALYTICS_3P_MESSAGE_TYPE.CREATIVE,
+      messageContainer => {
+        dev().assert(
+          messageContainer.data && Object.entries(messageContainer.data).length,


For all I know the JS compiler optimizes this but currently you're converting into an array twice so instead do:
let entries;
dev().assert(messageContainer.data && (entries = Object.entries(messageContainer.data)).length, ...);
entries.forEach(entry =>

keithwrightbos · 2017-06-14T19:28:08Z

3p/ampanalytics-lib.js

+    this.iframeMessagingClient_ = iframeMessagingClient;
+
+    /**
+     * @private {!string}


Nit: make all of these one line to match format of iframeMessageClient_ doc

keithwrightbos · 2017-06-14T19:28:20Z

3p/ampanalytics-lib.js

+    this.extraData_ = opt_extraData;
+
+    /**
+     * private {?function(!Array<!AmpAnalytics3pEvent>)=}


Note that it didn't like the = at the end, so did not include that

keithwrightbos · 2017-06-14T19:30:40Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+    /** @type {!Object<string,!string|!Array<string>>} */
+    this.creativeToPendingMessages_ = {};
+
+    this.throttledFlushQueue_ = throttle(this.win_, () => {


keithwrightbos · 2017-06-14T19:34:24Z

extensions/amp-analytics/0.1/transport.js

+          }
+          opt_processResponse(
+            this.type_,
+            /** @type {!../../../src/3p-analytics-common.AmpAnalytics3pResponse} */ (response)); // eslint-disable-line max-len


Can you just put (response)) on next line so you can remove the disable line length?

No, because the /** */ itself exceeds the limit

keithwrightbos · 2017-06-14T19:34:52Z

extensions/amp-analytics/0.1/transport.js

+  static doneUsingCrossDomainIframe(ampDoc, transportOptions) {
+    const frameUrl = transportOptions['iframe'];
+    const frameData = Transport.getFrameData_(frameUrl);
+    if (!frameData) {


Would this ever occur?

Suppose not. Removed.

keithwrightbos · 2017-06-14T19:39:33Z

extensions/amp-analytics/0.1/transport.js

+    setStyles(frame,
+      {width: 0, height: 0, display: 'none',
+       position: 'absolute', top: 0, left: 0});
+    /** @const {FrameData} */


I believe you want this to be: const frameData = /** @type {!FrameData} */ ({....});

Any reason why you switched to @type instead of @const?

keithwrightbos · 2017-06-14T19:40:54Z

extensions/amp-analytics/0.1/transport.js

 }

+/** @private @const {!Object<string,boolean>} */
+Transport.usedIds_ = {};


This is no longer used

Removed. I wonder if we could make the linter catch this...

keithwrightbos · 2017-06-14T19:42:19Z

build-system/amp.extern.js

@@ -60,6 +60,20 @@ window.AMP_CONFIG.errorReportingUrl;

 window.AMP_CONTEXT_DATA;

+// AMP-Analytics Cross-domain iframes
+/** @enum {string} */
+const AMP_ANALYTICS_3P_MESSAGE_TYPE = {


Since you already include this in a common util class, I don't believe you need it in externs as the compiler will have enforced type check

keithwrightbos

Some initial thoughts. Need to keep going through it. Looks like we're still missing unit test coverage and removing ResponseMap in favor of a closure based solution

keithwrightbos · 2017-06-19T16:48:38Z

3p/ampanalytics-lib.js

+            dev().assert(!this.creativeMessageRouters_[creativeId],
+                'Duplicate new creative message for ' + creativeId);
+            this.creativeMessageRouters_[creativeId] =
+            new AmpAnalytics3pCreativeMessageRouter(


Is this indentation correct? I would have expected "new" to be indented further.

keithwrightbos · 2017-06-19T16:50:24Z

3p/ampanalytics-lib.js

+    this.iframeMessagingClient_.registerCallback(
+        AMP_ANALYTICS_3P_MESSAGE_TYPE.EVENT,
+        messageContainer => {
+          Object.entries(messageContainer.data).forEach(entry => {


Add dev().assert(isArray(messageContainer.data) && messageContainer.data.length); // isArray comes from src/types.js

Done, in the style of lines 70-74 since messageContainer.data is an object, not an array.

keithwrightbos · 2017-06-19T16:52:26Z

3p/ampanalytics-lib.js

+            const messages = entry[1];
+            dev().assert(messages && messages.length,
+                'Received empty events list for' + creativeId);
+            dev().assert(this.creativeMessageRouters_[creativeId],


note that asserts throw so should we consider wrapping the contents in a try/catch to ensure one invalid data item doesn't cause all to be lost?

keithwrightbos · 2017-06-19T18:27:38Z

3p/ampanalytics-lib.js

+    this.eventListener_ = null;
+
+    if (this.win_.onNewAmpAnalyticsInstance) {
+      this.win_.onNewAmpAnalyticsInstance(this);


How do we handle if this throws?

Added a dev().error().

keithwrightbos · 2017-06-19T18:29:02Z

3p/iframe-messaging-client.js

@@ -100,6 +100,17 @@ export class IframeMessagingClient {
   */
  setupEventListener_() {
    listen(this.win_, 'message', event => {
+      try {
+        if (!this.hostWindow_.postMessage && this.hostWindow_.contentWindow) {


I think we can assume all AMP browsers will have postMessage.

That's not what I'm trying to do here. I'm trying to determine whether this.hostWindow_ is a window or a frame. If it's a frame, it won't have .postMessage(), it'll have .contentWindow.postMessage(). But I don't want to go straight for the .contentWindow.postMessage() check, because if it is a cross-domain window then that may throw.
I've split it into a helper method, and added comments, to make this clearer.

keithwrightbos · 2017-06-19T18:29:25Z

3p/iframe-messaging-client.js

+          // contentWindow is now non-null.
+          this.hostWindow_ = this.hostWindow_.contentWindow;
+        }
+      } catch (e) {


This would occur if window is xdomained? Do we want to warn in that case? When would it happen?

Shouldn't happen, given that I'm being careful in the "if" condition above (which is now extracted to another method, but works the same). Removed.

keithwrightbos · 2017-06-19T18:31:13Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   */
+  constructor(win, iframeMessagingClient, messageType) {
+    /** @private {boolean} */
+    this.isReady_ = false;


Nit: I like to put parameter based fields first.

keithwrightbos · 2017-06-19T18:32:07Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   *          ../../../src/3p-analytics-common.AmpAnalytics3pEvent}
+   * @abstract
+   */
+  buildMessage_() {


I usual make the default content an error: throw new Error('Not yet implemented?!');

Heh, I had originally thought of doing this, but https://github.com/google/closure-compiler/wiki/%40abstract-classes-and-methods says it's not allowed (second bullet point under "Rules" section)
Although, it only generates a warning, not an error:
WARNING - Misplaced @abstract annotation. function with a non-empty body cannot be abstract
Ideally we should change the type checking to allow an abstract method to contain a throw.

PS Tried with @Suppress {misplacedTypeAnnotation} but that doesn't squelch the warning. There does not seem to be a reference to misplacedAbstractAnnotation in https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/DiagnosticGroups.java

Surprisingly, Travis will fail if it gets a warning. So, taking this throw back out :(
https://travis-ci.org/ampproject/amphtml/jobs/245496442

I have filed a feature request w/ the Closure compiler team for this.

keithwrightbos · 2017-06-19T18:32:50Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+        type: this.messageType_,
+        data: this.creativeToPendingMessages_,
+      });
+    return message;


Nit: you can put the object constructor right after return, save message variable creation

In general yes, but then you're left with two bad choices. Choice 1:

return /** @type {../../../src/3p-analytics-common.AmpAnalytics3pNewCreative} */ ({ type: this.messageType_, data: this.creativeToPendingMessages_, });

In this case, the JS interpreter in its infinite wisdom thinks that "return" is a complete statement with no semicolon, so you're returning undefined, and what comes after that is unreachable code >:(

Choice 2:

return /** @type {../../../src/3p-analytics-common.AmpAnalytics3pNewCreative} */ ({ type: this.messageType_, data: this.creativeToPendingMessages_, });

Putting the opening paren (and therefore necessarily the casting comment) on a single line will do what we want semantically, but now we'd have to add a comment to tell the linter to ignore the line length violation.
I did it the way I did it for these reasons, though I agree it's stupid that I had to do so.

rsimha · 2017-06-21T21:26:21Z

The Percy failure was due to #10022, which has now been fixed.

… typedefs. Fixes error in which asserts cause error due to no ampdoc

…c flaw when sentinel is missing

…sage format, but new message format resulted in this being over-engineered

… warning makes Travis fail

keithwrightbos

I see test coverage for transport but not any of the new classes you created. Are those in progress?

keithwrightbos · 2017-06-28T13:15:29Z

3p/ampanalytics-lib.js

+              this.creativeMessageRouters_[creativeId]
+                  .sendMessagesToListener(messages);
+            } catch (e) {
+              dev().error(TAG_, 'Failed to send message to listener: ' +


Using the term "send" is a bit misleading here as one may expect it to mean you postMessaged. Perhaps "Failed to execute listener"...?

Re: your above comment which GitHub UI won't let me comment on, I forgot to "git add" the file to test the new Queue classes. Will add that together with addressing your comments here.

And re: the comment I'm actually responding to here, done. Changed to "Failed to pass message to event listener"

keithwrightbos · 2017-06-28T13:15:42Z

3p/ampanalytics-lib.js

+          });
+        });
+    this.iframeMessagingClient_.sendMessage(
+        AMP_ANALYTICS_3P_MESSAGE_TYPE.READY);


Can this fit on one line?

81 characters, unless I rename stuff.

keithwrightbos · 2017-06-28T13:19:06Z

3p/ampanalytics-lib.js

+      try {
+        this.win_.onNewAmpAnalyticsInstance(this);
+      } catch (e) {
+        dev().error(TAG_, 'Exception thrown by onNewAmpAnalyticsInstance in' +


Consider moving onNewAmpAnalyticsInstance check prior to creating AmpAnalytics3pCreativeMessageRouter as AmpAnalytics3pMessageRouter essentially expects it to work properly. In other words, it would call router's sendMessagesToListener function even though client is not actually listening

keithwrightbos · 2017-06-28T13:20:14Z

3p/ampanalytics-lib.js

+   * @param {!Array<AmpAnalytics3pEvent>} messages The message that was received
+   */
+  sendMessagesToListener(messages) {
+    if (!messages.length || !this.eventListener_) {


Consider adding a dev().warn indicating messages are being lost and registerAmpAnalytics3pEventsListener should be called.

We could also try and recover from previous invalid/missing onNewAmpAnalyticsInstance here

Given the fix to your previous comment, it is no longer possible to reach here without implementing onNewAmpAnalyticsInstance, as the enclosing AmpAnalytics3pMessageRouter will now not have been created.
However, this could still happen if they have done the first line here but not the second:

window.onNewAmpAnalyticsInstance = ampAnalytics => { ampAnalytics.registerAmpAnalytics3pEventsListener(events => {

Unfortunately we can't go and create that for them, so I just made the dev().warn message explicitly say "Attempted to send messages when no listener configured. Be sure to first call registerAmpAnalytics3pEventsListener()"

keithwrightbos · 2017-06-28T13:23:12Z

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

+
+      // Load the script specified in the iframe’s name attribute:
+      script = document.createElement('script');
+      script.src = JSON.parse(window.name).scriptSrc;


I fear this opens an XSS exploit for 3p analytic vendors. They shouldn't just trust the script src is to a valid AMP url. Instead we should either provide only the path to the file and hard code the domain or the vendor should have code verifying the domain. See how remote HTML loads the context script: https://github.com/ampproject/amphtml/blob/master/3p/remote.html#L9

This sounds like a good idea.
In Transport.createCrossDomainIframe, it sets the URL like this:

const scriptSrc = getMode().localDev ? '/dist.3p/current/ampanalytics-lib.js' : `${urls.thirdParty}/$internalRuntimeVersion$/ampanalytics-v0.js`;

I just ran it and forced the non-local path, and that gave me:

https://3p.ampproject.net/$internalRuntimeVersion$/ampanalytics-v0.js

So, sure, I could definitely just send everything after "https://3p.ampproject.net", and hard-code that part on the client side.
Though this makes me also wonder if I'm missing something necessary for the substitution of the internal runtime version?

Hmm...though we'll also have to know on that side whether we're local, since I won't want to prepend the "https://3p.ampproject.net" in that case.

keithwrightbos · 2017-06-28T13:26:19Z

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

-    sandbox.stub(Transport, 'sendRequestUsingImage');
+  function setupStubs(crossDomainIframeRetval, imageRetval,
+                      beaconRetval, xhrRetval) {
+    sandbox.stub(transport, 'sendRequestUsingCrossDomainIframe_').returns(


similarly consider making the function public but with @VisibleForTesting

Done, for all uses of private methods/members

keithwrightbos · 2017-06-28T13:28:02Z

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

@@ -48,69 +57,193 @@ describe('amp-analytics.transport', () => {
        'sendRequestUsingImage call count').to.equal(expectedImageCalls);
  }

+  function expectAllUnique(numArray) {
+    if (!numArray) {
+      return true;


Why return true instead of just return?

keithwrightbos · 2017-06-28T13:28:48Z

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

+
+  it('must create xframe before sending message to it', () => {
+    expect(() => {
+      transport.sendRequest(window, 'https://example.com/test', {


test referencing window makes me nervous and implies your test may not be hermetic...

I need to use a window object, to hang iframes on. Also the existing tests need one, to do stuff like win.navigator.sendBeacon(request, '');
But I agree it would be best to somehow reset it, or use some type of "test window" object that lives in the sinon sandbox. Do you know a way to do this?

keithwrightbos · 2017-06-28T13:29:38Z

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

+    sandbox.spy(transport, 'createCrossDomainIframe_');
+    transport.processCrossDomainIframe(window, config);
+
+    assert(transport.useExistingOrCreateCrossDomainIframe_.calledOnce);


why aren't these expect (e.g. expect(transport.useExistingOrCreateCrossDomainIframe_).to.be.calledOnce; )

Hmm. I thought they were equivalent, but just tested with assert(false) and expect(false).to.be.true and I see now that the latter has a more informative message.
Converted these to look like:
expect(transport.createCrossDomainIframe.calledOnce).to.be.true;

keithwrightbos · 2017-06-28T13:31:20Z

src/anchor-click-interceptor.js

@@ -59,6 +60,14 @@ function maybeExpandUrlParams(ampdoc, e) {
    'CLICK_Y': () => {
      return e.pageY;
    },
+    '3PANALYTICS': (frameType, key) => {
+      const responses = ResponseMap.get(ampdoc, frameType,


Why do you need responseMap? Idea was to use closure to keep track of data where message listener would update ampDoc.getAnchorClickListenerBinding() value.

I tried that, but it didn't seem like it was going to work. Though maybe I am missing something - I would be happy to pair on it if you'd like.
As it stands now, ResponseMap is now just a convenience wrapper around ampdoc's binding. I didn't want to spread the implementation details across both anchor-click-interceptor and amp-analytics, especially if they should ever change.

lannka · 2017-06-28T15:22:32Z

3p/iframe-messaging-client.js

@@ -101,6 +101,15 @@ export class IframeMessagingClient {
   */
  setupEventListener_() {
    listen(this.win_, 'message', event => {
+      if (this.hostWindowIsActuallyAnIframe_()) {
+        // this.hostWindow_ can now be set to an iframe, after it has been


why hostWindow can be an iframe?

It's a chicken-and-egg problem. I've created this iframe, and I want to know when it is ready. So I want to say "I am willing to receive messages from this iframe's contentWindow". But the iframe's content window is not defined until it has finished rendering. So basically, if I can only use the window object, then I can't know it's ready unless I can receive messages from it, but I can't receive messages from it until I know it's ready.

should you wait for a handshake initiated from the iframe window?

oh, wait, you wanted to make the iframe client a "host". which should not be a case after you switched to SubscriptionApi.

lannka · 2017-06-28T15:26:34Z

extensions/amp-analytics/0.1/transport.js

+        'Cross-domain frame parameters missing ${this.type_}');
+    const frameData = this.useExistingOrCreateCrossDomainIframe_(
+        win, frameUrl, transportOptions['extraData']);
+    const iframeMessagingClient = frameData.iframeMessagingClient;


should you use SubscriptionApi at host side?

Keith, what do you think? It does seem like this would be better on the AMP side. But AFAICT the 3p side would have to revert to window.addEventListener("message", ...) which loses a lot of what we gain from iframeMessagingClient.

lannka · 2017-06-28T15:32:08Z

3p/ampanalytics-lib.js

+/**
+ * Receives messages bound for this cross-domain iframe, from all creatives
+ */
+class AmpAnalytics3pMessageRouter {


to accelerate this PR, I suggest you split off the code into a 2nd PR. I can see clear boundary between the 2:
PR1: runtime side code
PR2: adding a library for 3p to use.

As long as there're clear API defined, you can easily separate the 2

jonkeller · 2017-06-28T20:33:18Z

At lannka's request, I have split this PR into two:
#10184
#10185
Therefore #9661 is now closed.

lannka · 2017-06-29T21:05:49Z

build-system/dep-check-config.js

@@ -152,6 +152,10 @@ exports.rules = [
  {
    filesMatching: '{src,extensions}/**/*.js',
    mustNotDependOn: '3p/**/*.js',
+    whitelist: [
+      'extensions/amp-analytics/0.1/transport.js->' +
+        '3p/iframe-messaging-client.js',


don't do this

googlebot added the cla: yes label Jun 1, 2017

jonkeller requested a review from keithwrightbos June 1, 2017 14:50

jonkeller commented Jun 1, 2017

View reviewed changes

keithwrightbos requested changes Jun 1, 2017

View reviewed changes

jonkeller commented Jun 4, 2017

View reviewed changes

keithwrightbos requested changes Jun 6, 2017

View reviewed changes

jonkeller commented Jun 7, 2017

View reviewed changes

keithwrightbos requested changes Jun 7, 2017

View reviewed changes

keithwrightbos requested changes Jun 8, 2017

View reviewed changes

jonkeller commented Jun 8, 2017

View reviewed changes

gulpfile.js

includePolyfills: true,

checkTypes: true,

}),

*/

Copy link

Contributor Author

jonkeller Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncomment, or delete?

jonkeller commented Jun 8, 2017

View reviewed changes

jonkeller added the WG: monetization label Jun 9, 2017

keithwrightbos assigned jonkeller Jun 10, 2017

keithwrightbos requested changes Jun 10, 2017

View reviewed changes

keithwrightbos requested changes Jun 14, 2017

View reviewed changes

keithwrightbos requested changes Jun 19, 2017

View reviewed changes

jonkeller requested a review from lannka June 22, 2017 13:31

jonkeller added 23 commits June 27, 2017 18:47

Fixes closure typing

21fb523

Replaces 2 lines accidentally deleted during rebase

6a53ab6

Addresses review feedback, mostly minor

cc245fa

Refactors message queueing from Transport into new class. Adds extern…

b8bd741

… typedefs. Fixes error in which asserts cause error due to no ampdoc

Addresses review feedback re: minor code organization issues and logi…

b7d8665

…c flaw when sentinel is missing

Addresses PR feedback, mostly typing, and format of messages

c25c199

Replaces custom throttling code with src/utils/rate-limit

59170c8

Changes format of postMessage messages

896e9fc

Addresses review feedback, mostly minor code style stuff

10c6cd9

Removes frame-side message queueing which was necessary under old mes…

14e9939

…sage format, but new message format resulted in this being over-engineered

Addresses review feedback

be9c95c

Updates linter compliance

036e2d0

Implements changes from self-review, mostly nits

7d3074b

Updates response handling to new data format

36918a4

Addresses PR feedback, mostly nits

3aeb8de

Puts anchor click listener binding into AmpDoc

591e18c

Fixes Travis issue that check-types is not catching locally

c446a5e

Removes throw statement from abstract method body, since the ensueing…

3c71898

… warning makes Travis fail

Trivial comment change, to force Percy to run again

dd4034b

Adds unit tests

f80268d

Makes typing of frame name more deliberate

e0b9f71

Just modifies comment, to force CI to re-run

c84129a

Fixes typing of frame name, for real this time

94cf287

keithwrightbos requested changes Jun 28, 2017

View reviewed changes

lannka reviewed Jun 28, 2017

View reviewed changes

jonkeller closed this Jun 28, 2017

lannka reviewed Jun 29, 2017

View reviewed changes

jonkeller deleted the amp_analytics_3p_vendors branch July 24, 2017 15:38

Amp analytics 3p vendors #9661

Amp analytics 3p vendors #9661

Conversation

jonkeller commented Jun 1, 2017

jonkeller Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeller Jun 1, 2017 • edited

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

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

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

jonkeller Jun 6, 2017 • edited

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 Jun 7, 2017 • edited

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

jonkeller Jun 1, 2017 •

edited

jonkeller Jun 1, 2017 •

edited

jonkeller Jun 6, 2017 •

edited

jonkeller Jun 7, 2017 •

edited