Introduce iframe transport to amp-analytics - 3p side #10596

jonkeller · 2017-07-24T17:54:39Z

This is the 3p-side companion to the amp analytics impl PR at #10442 and thus a simplified version of #10184

It is the result of rebasing the code that was at https://github.com/google/amphtml/pull/20 over to ampproject/amphtml

… queue much simpler than previous PR. Response functionality is included, but extraData is currently commented out - will add again before PR

… relying on omitting sandbox allow-same-origin

lannka · 2017-07-24T23:50:08Z

3p/ampanalytics-lib.js

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


how about iframe-transport-client.js?

Can do, if you think this will be more meaningful to customers. (Note that I am taking a vacation day Tuesday 7/25.)

this name is internal in our repo only, so it's a good idea to keep it consistent with other naming.
we can definitely use a different name for the compiled library (maybe including ampanalytics in the name).

I like this idea. Done.

lannka · 2017-07-24T23:51:31Z

3p/ampanalytics-lib.js

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


IframeTransportClient

lannka · 2017-07-24T23:54:33Z

3p/ampanalytics-lib.js

+   * @param messageType The type of message to subscribe to
+   * @VisibleForTesting
+   */
+  subscribeTo(messageType) {


should you use IframeMessagingClient?

I'm a bit confused here. You said to change from IframeMessagingClient to SubscriptionAPI. Do you mean to have one side of the conversation using IframeMessagingClient, and the other side using SubscriptionAPI? I can research whether that will work, but want to make sure I understand you correctly.

sorry if i didn't make it clear.
SubscriptionApi and IframeMessagingClient is a pair. the former is installed in host page, the latter is for the iframe client.

that's how it works for our 3P ads iframe

lannka · 2017-07-25T00:04:10Z

3p/ampanalytics-lib.js

+        'Must implement onNewAmpAnalyticsInstance in ' +
+        this.win_.location.href);
+    events.forEach(event => {
+      const transportId = event['transportId'];


to leverage the typecheck : event.transportid

Good idea - will do.

lannka · 2017-07-25T00:28:59Z

examples/analytics-iframe-transport-remote-frame.html

+          console.log("The page at: " + window.location.href +
+            " has received an event: " + event +
+            " from the creative with transport ID: " +
+            ampAnalytics.getTransportId());


so this onNewAmpAnalyticsInstance will be called for every transport?
I propose a simpler API:

function processAnalyticsEvent(event) { // vendor code goes here ... } const url = JSON.parse(window.name).scriptSrc; if (url && url.startsWith('https://3p.ampproject.net/')) { loadScript(url).then(() => { // see 3p.js window.context.onAnalyticsEvent(processAnalyticsEvent.bind(null)); }); }

Yes, it contains the transportId as state. This way, if 2 creatives use the same vendor, we will not confuse events from one creative with events from the other. I fear that your proposal would force the vendor to do the work of keeping events from different creatives separated.
Can't call loadScript() in 3p from here: to do so, we would have to compile it into the JS lib, and this is the code to load that lib, so it would be a chicken-and-egg problem. But, my code to createElement(), set src, and then appendChild() is identical to what is in loadScript(), minus the callbacks which I don't have a use for currently.
Also, window context API is not being used.

would force the vendor to do the work of keeping events from different creatives separated

I don't feel we should do that much for the vendor. The reason we have this client lib with sacrificing of speed is that we don't want to expose the raw postMessage API to them (which is hard to migrate once vendors start to depend on it). So the only job of the client lib is to convert raw message to a js friendly object.

I want to keep the API simple and flexible. We simply flush the events to the client, each event has the transport info. And vendors can have their own design about how to passing the creative IDs along with the event. And it's their job how to group them / process them.

Can't call loadScript() in 3p from here

I was not talking about import the code here. you can copy-paste the impl here.

Ok, I will work to simplify this. If you don't mind, I'll still leave my assert()s though.
Re: loadScript(), my current implementation is essentially the same, minus the callbacks. loadScript():

export function loadScript(win, url, opt_cb, opt_errorCb) { /** @const {!Element} */ const s = win.document.createElement('script'); s.src = url; if (opt_cb) { s.onload = opt_cb; } if (opt_errorCb) { s.onerror = opt_errorCb; } win.document.body.appendChild(s); }

Mine:

script = document.createElement('script'); script.src = url; document.head.appendChild(script);

I'll switch that last line from document.head to document.body, though.

Simplification is done. Did not end up changing from "document.head" to "document.body" since the remote frame has no content and thus no body.

lannka · 2017-07-25T00:29:33Z

3p/ampanalytics-lib.js

+ * Receives messages bound for this cross-domain iframe, from a particular
+ * creative.
+ */
+export class CreativeEventRouter {


why is this class necessary?

Currently CreativeEventRouter provides an association between a creative (transport) and the customer-provided listener function, and EventRouter handles communication with the parent frame (for any/all transports). Therefore the function passed to registerCreativeEventListener() is specific to one creative.

did we normalized all events into one type at host side already?

…access. Have not yet addressed the comments about IframeMessagingClient nor simplifying to eliminate CreativeEventRouter.

jonkeller · 2017-07-26T18:57:59Z

examples/analytics-iframe-transport-remote-frame.html

+    <title>Requests Frame</title>
+    <script>
+      /**
+       * To receive analytics events in a third-party frame, you must


Comment is outdated

lannka · 2017-07-26T19:10:56Z

examples/analytics-iframe-transport-remote-frame.html

+       * @param ampAnalytics  Call registerAmpAnalyticsEventListener() on this,
+       * passing your function which will receive the analytics events.
+       */
+      window.processAmpAnalyticsEvent = (event, transportId) => {


thanks for simplifying. but this is a bit different from what I proposed.
i was thinking of exposing the API as window.context.onAnalyticsEvent(callback)

more consistent with our ampcontext api

this gives 3p code to decide when to call the api, as they might be loading something necessary asynchronously.

Done, back to you.

lannka · 2017-07-26T19:13:47Z

3p/iframe-transport-client.js

+  }
+}
+
+if (!window.AMP_TEST) {


we can follow ampcontext-lib.js to split this piece out of this file.

jonkeller · 2017-07-26T20:57:30Z

examples/analytics-iframe-transport-remote-frame.html

+    <meta charset="UTF-8">
+    <title>Requests Frame</title>
+    <script>
+      window.addEventListener('amp-iframeTransportClientCreated', function() {


Changed to resemble:

amphtml/examples/ampcontext-creative.html

Line 12 in d98d8e1

window.addEventListener('amp-windowContextCreated', function(){

jonkeller · 2017-07-26T20:58:13Z

3p/iframe-transport-client-lib.js

+ *  assign it to window.iframeTransportClient, to provide the creative with
+ *  all the required functionality.
+ */
+try {


Modelled after https://github.com/ampproject/amphtml/blob/ea4b3f60b0c6293d4fb31f100886b666cb1f398b/3p/ampcontext-lib.js

lannka · 2017-07-27T19:01:52Z

3p/iframe-transport-client.js

+        'Invalid/missing sentinel on iframe name attribute' + this.win_.name));
+    this.client_.makeRequest(
+        IFRAME_TRANSPORT_EVENTS_TYPE,
+        IFRAME_TRANSPORT_EVENTS_TYPE,


back to my previous comment, i think it make sense to have 2 different message type

SEND_IFRAME_TRANSPORT_EVENTS, IFRAME_TRANSPORT_EVENTS

naming is after 3p-frame-messaging.js

Fixed by creating constants with different names, but same value. See:
https://github.com/ampproject/amphtml/blob/master/src/iframe-helper.js#L217
https://github.com/ampproject/amphtml/blob/master/src/iframe-helper.js#L265
...for why values must be the same.
Just to be safe: Tested with same values, works. Tested with different values, does not work.

Tested with different values, does not work.

That does not sound right. Can you investigate?

I believe this makes sense. In the class at https://github.com/ampproject/amphtml/blob/master/src/iframe-helper.js#L412, subscribers send a message of a certain type. That is stored in this.clientWindows_
Later, when send() is called (line 446), it looks at this.clientWindows_ to see who wants messages of that type, and sends it to them.
I don't see how it could work with the request/response message types being different. If you send a subscription request for message type X, it wouldn't/shouldn't know to send you message type Y.

All the current usage of IframeMessagingClient#makeRequest takes different type: https://github.com/ampproject/amphtml/blob/master/3p/ampcontext.js#L158

Fixed, to resemble code in provided link

lannka · 2017-07-27T19:03:25Z

3p/iframe-transport-client.js

+              'Must call onAnalyticsEvent in ' + this.win_.location.href);
+          events.forEach(event => {
+            try {
+              this.listener_(event.message, event.transportId);


listener_ can be null

lannka · 2017-07-28T20:00:47Z

src/iframe-transport-common.js

@@ -19,9 +19,16 @@
 * This is the type of message that will be sent to the 3p frame.
 * The message will contain an array of the typedef declared below.
 */
-export const IFRAME_TRANSPORT_EVENTS_TYPE = 'IframeTransportEvents';
+export const IFRAME_TRANSPORT_EVENTS = 'iframe-transport-events';


actually, it probably makes sense to merge this whole file into 3p-frame-messaging.js now

Ok, I can do that. Thanks!

keithwrightbos · 2017-07-28T20:50:19Z

3p/iframe-transport-client.js

+    /** @protected {!IframeMessagingClient} */
+    this.client_ = new IframeMessagingClient(win);
+    this.client_.setHostWindow(this.win_.parent);
+    this.client_.setSentinel(dev().assertString(


Should this be user() instead of dev()? If it fails, I assume we cannot recover?

I agree. We should probably change it at https://github.com/ampproject/amphtml/blob/master/3p/ampcontext.js#L103 also

keithwrightbos · 2017-07-28T20:51:33Z

examples/analytics-iframe-transport-remote-frame.html

+    <meta charset="UTF-8">
+    <title>Requests Frame</title>
+    <script>
+      window.addEventListener('amp-iframeTransportClientCreated', function() {


Any reason not to use big arrow notation? (e.g. () => {...})

Agree with this, too. Will leave myself a note to also change

amphtml/examples/ampcontext-creative.html

Line 12 in d98d8e1

window.addEventListener('amp-windowContextCreated', function(){

after fixit week.

keithwrightbos · 2017-07-28T20:51:45Z

examples/analytics-iframe-transport-remote-frame.html

+        window.iframeTransportClient.onAnalyticsEvent(
+            (event, transportId) => {
+              // Now, do something meaningful with the AMP Analytics event
+              console.log("The page at: " + window.location.href +


Single quotes?

lannka

LGTM
thanks for the change.

jonkeller added 30 commits July 24, 2017 11:44

Adds 3p iframe to amp analytics. Uses Subscription API. Event message…

9a2c05e

… queue much simpler than previous PR. Response functionality is included, but extraData is currently commented out - will add again before PR

Fix SubscriptionAPI

c539c66

Brings in previous chanages to anchor-click-interceptor.js

2f892c3

Adds unit tests

e754522

Updates queue unit tests

be880d6

Updates custom types and unit tests

8c76838

Adds misc small fixes/nits from self-review

dbf2960

Changes 2 URLs in example from absolute to relative

7e58280

Comments only

5d46a9d

Fixes TODO re: unlayoutCallback destroying 3p iframe

b6351f2

Fixes an 80-char line length violation introduced in previous PR

d15b9c5

Implements review feedback

fb539c8

Implements review feedback

f252fe1

Addresses review feedback

ab3b882

Reverts change to URL calculation, pending additional discussion

8b47fb0

Fixes unit test

b291ea1

Fixes indentation of 2 lines

8373e55

Removes extraData, new creative message

e56dbed

Fixes transport unit tests

83fa626

Clarifies transport ID

8b5f533

Fixes whitespace

88d118d

Improves example triggers. Fixes issue of delivery of creative response

5e4e3f7

Makes indentation consistent in this section

558339f

Addresses review feedback

305a84b

Fixes a null check

421b56e

Keys off of type rather than URL. Fixes issue of null origin, without…

c1b36b5

… relying on omitting sandbox allow-same-origin

Minor type annotation corrections

e111811

Makes use of new param to SubscriptionAPI c'tor

e590d9e

Removes allow-same-origin

4c17cbe

Addresses review feedback, including removing response

3ce7c3b

lannka requested changes Jul 25, 2017

View reviewed changes

jonkeller added 7 commits July 26, 2017 11:01

Addresses review feedback re: renaming files/class, and object field …

2f9bec5

…access. Have not yet addressed the comments about IframeMessagingClient nor simplifying to eliminate CreativeEventRouter.

Switch to IframeMessagingClient, add clarifying comment

66d0cc4

Now uses IframeMessagingClient

545ac7b

Greatly simplified iframe-transport-client

c7b6c88

Updates unit tests

88b6c4c

Changes whitespace only

015c48a

Fixes a broken unit test

ef8fb8d

jonkeller commented Jul 26, 2017

View reviewed changes

Changes comment only

e7c0311

lannka reviewed Jul 26, 2017

View reviewed changes

jonkeller added 2 commits July 26, 2017 16:15

Makes lib architecture more similar to ampcontext

6ca88ee

Adds 3p/iframe-transport-client-lib.js

1e8595d

jonkeller commented Jul 26, 2017

View reviewed changes

lannka requested changes Jul 27, 2017

View reviewed changes

jonkeller added 3 commits July 27, 2017 16:25

Differentiates message name, fixes possible null exception

3349b6b

Trivial change to re-run Percy, which was flakey last time

bd9a9eb

Splits request/response message types better

39164c5

lannka approved these changes Jul 28, 2017

View reviewed changes

Moves message type declarations into 3p-frame-messaging.js

7b0a03b

keithwrightbos approved these changes Jul 28, 2017

View reviewed changes

Addresses a few nits

d265980

lannka approved these changes Jul 28, 2017

View reviewed changes

keithwrightbos merged commit 53925ed into ampproject:master Jul 28, 2017

jonkeller deleted the amp_analytics_3p_1 branch July 29, 2017 16:57

jonkeller mentioned this pull request Jul 31, 2017

Fixes minor code style issues #10715

Merged

ampprojectbot added the PR use: In Beta / Experimental label Aug 2, 2017

ampprojectbot added the PR use: In Stable label Aug 10, 2017

jonkeller mentioned this pull request Oct 20, 2017

AMP Analytics 3P Obfuscation #9433

Closed

Introduce iframe transport to amp-analytics - 3p side #10596

Introduce iframe transport to amp-analytics - 3p side #10596

Conversation

jonkeller commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 Jul 28, 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

lannka left a comment

Choose a reason for hiding this comment

jonkeller Jul 28, 2017 •

edited