AMP signal collection frame by keithwrightbos · Pull Request #6830 · ampproject/amphtml

keithwrightbos · 2016-12-28T20:56:46Z

To be included in valid AMP A4A ad content allowing for click url augmentation based on custom javascript execution within a cross domain iframe. Note the following:

Creates new amp-signal-collection-frame extension with priority 1 which creates hidden xdomain iframe
URL to xdomain source is hardcoded within config to ensure only specific custom JS is allowed to be executed
amp-a4a url replacement has been augmented to allow for information postmessaged from the cross domain frames to be included (will need to be ported to amp-inabox functionality as part of A4A Cross Domained AMP Creatives Click URL Does Not Replace X/Y #6385
amp-signal-collection-frame element sends postmessage to hidden iframes with touch based events in 100 ms intervals

…l_collection_frame

dvoytenko · 2017-01-03T22:48:36Z

@keithwrightbos Top-level question: is this mostly the same code as we had https://github.com/ampproject/amphtml/blob/master/src/document-click.js ? I can't find where we removed it and why...

lannka · 2017-01-04T00:14:56Z

extensions/amp-a4a/0.1/amp-a4a.js

      'CLICK_Y': () => {
        return e.pageY;
      },
+      'POSTMESSAGE': (origin, varName) => {


Are we going to make this genetic to any postmessage?

If we are, then there are other security issues to be considered.

If not, I suggest making it BG_SIGNAL(type), which is more specific and reduces chance of mis-use.

Its not really "generic" to any postmessage given the above code restricts populating the postmessage data by messages coming from an iframe within an amp-signal-collection-frame element. I could rename but was just trying to match the i2i #3665 . Thoughts?

lannka · 2017-01-04T05:33:36Z

extensions/amp-signal-collection-frame/0.1/amp-signal-collection-frame.js

+ */
+export const MESSAGE_INTERVAL_MS = 100;
+
+export class AmpSignalCollectionFrame extends AMP.BaseElement {


have we considered having the logic as a part of the doubleclick A4A extension? an extra extension means extra latency.

of course, probably I should ask this first: is it possible for doubleclick A4A to load a creative with AmpSignalCollectionFrame of other type?

This will be shared across AdSense and Doubleclick and potentially other networks that could return a creative from either of those networks so having it in A4A abstract element makes the most sense IMO. Let's discuss offline.

IIRC the resolution of this was to keep separate, but may be thinking of a distinct/similar issue. Confirm please?

lannka · 2017-01-04T05:53:32Z

extensions/amp-a4a/0.1/amp-a4a.js

+      }
+      // Only allow messages from child iframe whose parent is an
+      // amp-signal-collection-frame element.
+      const frames = iframeWin.document


let's cache the results for the sake of performance

The reason not to cache is that attachment of the iframe within amp-signal-collection-frame is done within layoutCallback of that element. Given it will have a lower priority (2) it is likely it will be appended AFTER registerExpandUrlParams_ executes. I also want to allow for the possibility that multiple amp-signal-collection-frame elements exist within the AMP creative so its not sufficient to use the result of querySelectorAll after the first post message. I suppose I could verify the count of iframes = count of total amp signal collection frames and cache at that time... just not sure its worth it. Thoughts?

lannka · 2017-01-04T05:57:58Z

extensions/amp-signal-collection-frame/0.1/amp-signal-collection-frame.js

+      // Place listener on outer document and use capture to ensure data is
+      // always collected.
+      this.unlisteners_.push(
+        listen(this.element.ownerDocument.documentElement, name, e => {


do we allow multiple SignalCollectionFrames in a single creative? If so, should we reuse the listeners for the sake of performance?

lannka · 2017-01-04T06:03:07Z

extensions/amp-signal-collection-frame/0.1/amp-signal-collection-frame.js

+  }
+
+  sendPostMessage(win, message) {
+    win./*REVIEW*/postMessage(message, '*');


for safety, we can use explicit target origin here.

lannka · 2017-01-04T06:11:08Z

extensions/amp-signal-collection-frame/0.1/amp-signal-collection-frame.js

+            JSON.stringify({'collection-events': events}));
+        events = {};
+      }
+    }, MESSAGE_INTERVAL_MS);


did you think of using rate-limit.js ?

lannka · 2017-01-04T06:17:02Z

extensions/amp-a4a/0.1/amp-a4a.js

+    /** @type {Object<string, Object<string, string>>} */
+    const messages = {};
+    // Listen for messages to creative friendly frame.
+    this.unlisteners_.push(listen(iframeWin, 'message', evt => {


if we're eventually having a new extension, i'd prefer all the logic go there.

keithwrightbos · 2017-01-04T16:31:06Z

@dvoytenko responding to Top-level question: is this mostly the same code as we had https://github.com/ampproject/amphtml/blob/master/src/document-click.js?

It was removed due to concerns that anchors not within AMP creatives could be modified. @cramforce isolated it to only the AMP creative iframe within amp-a4a.js and I have an outstanding issue #6385 assigned to @lannka to ensure it is ported to AMP-inabox runtime.

cramforce

Does the current design allow for a single signal collection iframe per page instead of one per ad?

cramforce · 2017-01-04T23:29:40Z

extensions/amp-a4a/0.1/amp-a4a.js

    };
+    // For now we only allow to replace the vars defined items and nothing else.
+    // NOTE: Addition to this whitelist requires additional review.
+    const whitelist = {};


cramforce · 2017-01-04T23:30:43Z

extensions/amp-a4a/0.1/amp-a4a.js

+        try {
+          if (frame.contentWindow == evt.source) {
+            const originUrl = parseUrl(evt.origin);
+            messages[originUrl.hostname.toUpperCase()] =


Lets use the .origin

cramforce · 2017-01-04T23:31:23Z

extensions/amp-a4a/0.1/amp-a4a.js

+    /** @type {Object<string, Object<string, string>>} */
+    const messages = {};
+    // Listen for messages to creative friendly frame.
+    this.unlisteners_.push(listen(iframeWin, 'message', evt => {


Lets factor this into a specific method.

tdrl

Still reviewing, but have gotten preempted by other things today.

tdrl · 2017-01-05T14:25:26Z

extensions/amp-a4a/0.1/amp-a4a.js

        this.xOriginIframeHandler_ = null;
      }
      this.layoutMeasureExecuted_ = false;
+      this.unlisteners_.forEach(unlistener => unlistener);


Sorry if this is JS-naive, but... Not clear what this is doing. Shouldn't it be .foreach(unlistener => unlistener())?

lannka · 2017-05-11T17:53:37Z

design changed. closing this for now.

keithwrightbos added 6 commits December 15, 2016 16:14

initial commit (tests in progress)

e69e8cd

test additions

0f812ee

Merge branch 'master' of github.com:ampproject/amphtml into a4a_signa…

69c28bb

…l_collection_frame

Merge branch 'master' of github.com:ampproject/amphtml into a4a_signa…

920eb1c

…l_collection_frame

Merge branch 'master' of github.com:ampproject/amphtml into a4a_signa…

49f4b00

…l_collection_frame

test coverage

c4f0e90

keithwrightbos assigned cramforce, dvoytenko and tdrl Dec 28, 2016

keithwrightbos added the WG: monetization label Dec 28, 2016

keithwrightbos added 3 commits December 28, 2016 15:58

update md

901ad2f

Fix build failures, update CSS

1394e00

move postmessage listener to creative frame

076f575

lannka suggested changes Jan 4, 2017

View reviewed changes

lannka reviewed Jan 4, 2017

View reviewed changes

lannka self-assigned this Jan 4, 2017

lannka reviewed Jan 4, 2017

View reviewed changes

keithwrightbos mentioned this pull request Jan 4, 2017

Intent to Implement: Anchor Destination Expand on Click #3665

Closed

cramforce requested changes Jan 4, 2017

View reviewed changes

tdrl reviewed Jan 5, 2017

View reviewed changes

clawr mentioned this pull request Mar 30, 2017

Intent to Implement: amp-ad-exit #8515

Closed

lannka closed this May 11, 2017

Conversation

keithwrightbos commented Dec 28, 2016

Uh oh!

dvoytenko commented Jan 3, 2017

Uh oh!

lannka Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonkeller Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Jan 4, 2017

Uh oh!

cramforce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka commented May 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lannka Jan 4, 2017 •

edited

Loading

jonkeller Jan 31, 2017 •

edited

Loading