Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 85 commits into from
Jul 28, 2017
Merged
Show file tree
Hide file tree
Changes from 70 commits
Commits
Show all changes
85 commits
Select commit Hold shift + click to select a range
9a2c05e
Adds 3p iframe to amp analytics. Uses Subscription API. Event message…
jonkeller Jul 13, 2017
c539c66
Fix SubscriptionAPI
jonkeller Jul 14, 2017
2f892c3
Brings in previous chanages to anchor-click-interceptor.js
jonkeller Jul 14, 2017
e754522
Adds unit tests
jonkeller Jul 14, 2017
be880d6
Updates queue unit tests
jonkeller Jul 14, 2017
8c76838
Updates custom types and unit tests
jonkeller Jul 14, 2017
dbf2960
Adds misc small fixes/nits from self-review
jonkeller Jul 14, 2017
7e58280
Changes 2 URLs in example from absolute to relative
jonkeller Jul 14, 2017
5d46a9d
Comments only
jonkeller Jul 14, 2017
b6351f2
Fixes TODO re: unlayoutCallback destroying 3p iframe
jonkeller Jul 14, 2017
d15b9c5
Fixes an 80-char line length violation introduced in previous PR
jonkeller Jul 14, 2017
fb539c8
Implements review feedback
jonkeller Jul 14, 2017
f252fe1
Implements review feedback
jonkeller Jul 14, 2017
ab3b882
Addresses review feedback
jonkeller Jul 14, 2017
8b47fb0
Reverts change to URL calculation, pending additional discussion
jonkeller Jul 14, 2017
b291ea1
Fixes unit test
jonkeller Jul 14, 2017
8373e55
Fixes indentation of 2 lines
jonkeller Jul 14, 2017
e56dbed
Removes extraData, new creative message
jonkeller Jul 15, 2017
83fa626
Fixes transport unit tests
jonkeller Jul 15, 2017
8b5f533
Clarifies transport ID
jonkeller Jul 15, 2017
88d118d
Fixes whitespace
jonkeller Jul 15, 2017
5e4e3f7
Improves example triggers. Fixes issue of delivery of creative response
jonkeller Jul 15, 2017
558339f
Makes indentation consistent in this section
jonkeller Jul 15, 2017
305a84b
Addresses review feedback
jonkeller Jul 17, 2017
421b56e
Fixes a null check
jonkeller Jul 17, 2017
c1b36b5
Keys off of type rather than URL. Fixes issue of null origin, without…
jonkeller Jul 17, 2017
e111811
Minor type annotation corrections
jonkeller Jul 17, 2017
e590d9e
Makes use of new param to SubscriptionAPI c'tor
jonkeller Jul 17, 2017
4c17cbe
Removes allow-same-origin
jonkeller Jul 18, 2017
3ce7c3b
Addresses review feedback, including removing response
jonkeller Jul 19, 2017
fbdb4a8
Fixes unit test
jonkeller Jul 19, 2017
f664d31
Adds a forgotten semicolon :(
jonkeller Jul 19, 2017
fcad58a
Addresses review feedback
jonkeller Jul 20, 2017
4c52b48
Splits crossDomainIframe functionality out from transport.js into ifr…
jonkeller Jul 20, 2017
2920068
Removes a couple blank lines
jonkeller Jul 20, 2017
d5b2dae
Fixes merge conflict
jonkeller Jul 20, 2017
7b12404
Mostly changes comments, removes a small amount of redundant unit tes…
jonkeller Jul 20, 2017
28c7b7a
Changes enum to const since all but one enum value no longer used. Re…
jonkeller Jul 21, 2017
409ac7c
Renames things using 3P in name, changes wire format from map to array
jonkeller Jul 21, 2017
ee822cf
Renamed files/classes to get rid of '3p'
jonkeller Jul 24, 2017
f4f5048
Adds 3p iframe to amp analytics. Uses Subscription API. Event message…
jonkeller Jul 13, 2017
0203122
Implements review feedback
jonkeller Jul 14, 2017
bff17d9
Removes extraData, new creative message
jonkeller Jul 15, 2017
0d64916
Amp 3p analytics using SubscriptionAPI, 3p side
jonkeller Jul 14, 2017
0e54a38
Fixes ampanalytics-lib tests
jonkeller Jul 14, 2017
3d7dbd6
Fixes linter issues
jonkeller Jul 14, 2017
4889860
Removes extraData, new creative message
jonkeller Jul 15, 2017
21d272f
Stash
jonkeller Jul 15, 2017
f363bf1
Changes creative ID to transport ID
jonkeller Jul 15, 2017
284d363
Includes transport ID in example logging, response
jonkeller Jul 15, 2017
c7898d7
Makes example ad click URL match parent branch
jonkeller Jul 15, 2017
352f1f8
Addresses review feedback
jonkeller Jul 17, 2017
904148d
Addresses further review feedback
jonkeller Jul 17, 2017
56cc6a6
extensions/amp-analytics/0.1/amp-analytics.js
jonkeller Jul 17, 2017
2cccfc3
Corrects rebase issue
jonkeller Jul 17, 2017
cfa8c33
Add XSS check to example HTML
jonkeller Jul 18, 2017
d8dde0f
Changes XSS message from log to warning
jonkeller Jul 18, 2017
98249eb
Addresses review nits
jonkeller Jul 18, 2017
78098c5
Addresses review feedback including removing response temporarily
jonkeller Jul 19, 2017
fa9c3f3
Clean up unit tests, rename event datatype
jonkeller Jul 20, 2017
ac5bdca
Resolves rebase issue
jonkeller Jul 20, 2017
d96b644
Intentionally adding trivial change, will remove in a minute
jonkeller Jul 20, 2017
0f03774
Removing trival change added a minute ago. This is because Git UI is …
jonkeller Jul 20, 2017
3ff0ae4
Changes an enum to a const, since all but one enum value have been re…
jonkeller Jul 21, 2017
3092770
Renames things using 3P in name, changes wire format from map to array
jonkeller Jul 21, 2017
ac08ff3
Fixes unit test
jonkeller Jul 21, 2017
8e4a0a3
Renamed files/classes to get rid of '3p'
jonkeller Jul 24, 2017
29e94e5
Fixes some merge issues
jonkeller Jul 24, 2017
ab3db5f
Found typo in method name, which led to renaming that and a few other…
jonkeller Jul 24, 2017
a7f05ea
Adds missing space in error msg
jonkeller Jul 24, 2017
2f9bec5
Addresses review feedback re: renaming files/class, and object field …
jonkeller Jul 26, 2017
66d0cc4
Switch to IframeMessagingClient, add clarifying comment
jonkeller Jul 26, 2017
545ac7b
Now uses IframeMessagingClient
jonkeller Jul 26, 2017
c7b6c88
Greatly simplified iframe-transport-client
jonkeller Jul 26, 2017
88b6c4c
Updates unit tests
jonkeller Jul 26, 2017
015c48a
Changes whitespace only
jonkeller Jul 26, 2017
ef8fb8d
Fixes a broken unit test
jonkeller Jul 26, 2017
e7c0311
Changes comment only
jonkeller Jul 26, 2017
6ca88ee
Makes lib architecture more similar to ampcontext
jonkeller Jul 26, 2017
1e8595d
Adds 3p/iframe-transport-client-lib.js
jonkeller Jul 26, 2017
3349b6b
Differentiates message name, fixes possible null exception
jonkeller Jul 27, 2017
bd9a9eb
Trivial change to re-run Percy, which was flakey last time
jonkeller Jul 27, 2017
39164c5
Splits request/response message types better
jonkeller Jul 28, 2017
7b0a03b
Moves message type declarations into 3p-frame-messaging.js
jonkeller Jul 28, 2017
d265980
Addresses a few nits
jonkeller Jul 28, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
264 changes: 264 additions & 0 deletions 3p/ampanalytics-lib.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

how about iframe-transport-client.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea. Done.

* Copyright 2017 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import './polyfills';
import {tryParseJson} from '../src/json';
import {dev, user, initLogConstructor, setReportError} from '../src/log';
import {IFRAME_TRANSPORT_EVENTS_TYPE} from '../src/iframe-transport-common';
import {getData} from '../src/event-helper';

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

/** @private @const {string} */
const TAG_ = 'ampanalytics-lib';

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

Choose a reason for hiding this comment

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

IframeTransportClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


/** @param {!Window} win */
constructor(win) {
/** @private {!Window} */
this.win_ = win;

/** @const {string} */
this.sentinel_ = user().assertString(
tryParseJson(this.win_.name)['sentinel'],
'Invalid/missing sentinel on iframe name attribute' + this.win_.name);
if (!this.sentinel_) {
return;
}

/**
* Multiple creatives on a page may wish to use the same type of
* amp-analytics tag. This object provides a mapping between the
* IDs which identify which amp-analytics tag a message is to/from,
* with each ID's corresponding CreativeEventRouter,
* which is an object that handles messages to/from a particular creative.
* @private {!Object<string, !CreativeEventRouter>}
*/
this.creativeEventRouters_ = {};

this.win_.addEventListener('message', event => {
const messageContainer = this.extractMessage_(event);
if (this.sentinel_ != messageContainer['sentinel']) {
return;
}
user().assert(messageContainer['type'],
'Received message with missing type in ' + this.win_.location.href);
user().assert(messageContainer['events'],
'Received empty message in ' + this.win_.location.href);
user().assert(
messageContainer['type'] == IFRAME_TRANSPORT_EVENTS_TYPE,
'Received unrecognized message type ' + messageContainer['type'] +
' in ' + this.win_.location.href);
this.processEventsMessage_(
/**
* @type {!Array<../src/iframe-transport-common.IframeTransportEvent>}
*/
(messageContainer['events']));
}, false);

this.subscribeTo(IFRAME_TRANSPORT_EVENTS_TYPE);
}

/**
* Sends a message to the parent frame, requesting to subscribe to a
* particular message type
* @param messageType The type of message to subscribe to
* @VisibleForTesting
*/
subscribeTo(messageType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should you use IframeMessagingClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

this.win_.parent./*OK*/postMessage(/** @type {JsonObject} */ ({
sentinel: this.sentinel_,
type: messageType,
}), '*');
}

/**
* Handle receipt of a message indicating that creative(s) have sent
* event(s) to this frame
* @param {!Array<!../src/iframe-transport-common.IframeTransportEvent>}
* events An array of events
* @private
*/
processEventsMessage_(events) {
user().assert(events && events.length,
'Received empty events list in ' + this.win_.location.href);
this.win_.onNewAmpAnalyticsInstance =
this.win_.onNewAmpAnalyticsInstance || null;
user().assert(this.win_.onNewAmpAnalyticsInstance,
'Must implement onNewAmpAnalyticsInstance in ' +
this.win_.location.href);
events.forEach(event => {
const transportId = event['transportId'];
Copy link
Contributor

Choose a reason for hiding this comment

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

to leverage the typecheck : event.transportid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea - will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const message = event['message'];
try {
if (!this.creativeEventRouters_[transportId]) {
this.creativeEventRouters_[transportId] =
new CreativeEventRouter(
this.win_, this.sentinel_, transportId);
try {
this.win_.onNewAmpAnalyticsInstance(
this.creativeEventRouters_[transportId]);
} catch (e) {
user().error(TAG_, 'Caught exception in' +
' onNewAmpAnalyticsInstance: ' + e.message);
throw e;
}
}
this.creativeEventRouters_[transportId]
.sendMessageToListener(message);
} catch (e) {
user().error(TAG_, 'Failed to pass message to event listener: ' +
e.message);
}
});
}

/**
* Test method to ensure sentinel set correctly
* @returns {string}
* @VisibleForTesting
*/
getSentinel() {
return this.sentinel_;
}

/**
* Gets the mapping of creative senderId to
* CreativeEventRouter
* @returns {!Object.<string, !CreativeEventRouter>}
* @VisibleForTesting
*/
getCreativeEventRouters() {
return this.creativeEventRouters_;
}

/**
* Gets rid of the mapping to CreativeEventRouter
* @VisibleForTesting
*/
reset() {
this.creativeEventRouters_ = {};
}

/**
* Takes the raw postMessage event, and extracts from it the actual data
* payload
* @param event
* @returns {JsonObject}
* @private
*/
extractMessage_(event) {
user().assert(event, 'Received null event in ' + this.win_.name);
const data = String(getData(event));
user().assert(data, 'Received empty event in ' + this.win_.name);
let startIndex;
user().assert((startIndex = data.indexOf('-') + 1) > 0,
'Received truncated events message in ' + this.win_.name);
return tryParseJson(data.substr(startIndex)) || null;
}
}

if (!window.AMP_TEST) {
try {
new EventRouter(window);
} catch (e) {
user().error(TAG_, 'Failed to construct EventRouter: ' +
e.message);
}
}

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

Choose a reason for hiding this comment

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

why is this class necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

/**
* @param {!Window} win The enclosing window object
* @param {!string} sentinel The communication sentinel of this iframe
* @param {!string} transportId The ID of the creative to route messages
* to/from
*/
constructor(win, sentinel, transportId) {
/** @private {!Window} */
this.win_ = win;

/** @private {!string} */
this.sentinel_ = sentinel;

/** @private {!string} */
this.transportId_ = transportId;

/** @private
* {?function(!Array<!string>)} */
this.eventListener_ = null;
}

/**
* Registers a callback function to be called when AMP Analytics events occur.
* There may only be one listener. If another function has previously been
* registered as a listener, it will no longer receive events.
* @param {!function(!string)}
* listener A function that takes an event string, and does something with
* it.
*/
registerCreativeEventListener(listener) {
if (this.eventListener_) {
dev().warn(TAG_, 'Replacing existing eventListener for ' +
this.transportId_);
}
this.eventListener_ = listener;
}

/**
* Receives message(s) from a creative for the cross-domain iframe
* and passes them to that iframe's listener, if a listener has been
* registered
* @param {!string} message The event message that was received
*/
sendMessageToListener(message) {
if (!this.eventListener_) {
dev().warn(TAG_, 'Attempted to send message when no listener' +
' configured. Sentinel=' + this.sentinel_ + ', TransportID=' +
this.transportId_ + '. Be sure to' +
' call registerCreativeEventListener() within' +
' onNewAmpAnalyticsInstance()!');
return;
}
try {
this.eventListener_(message);
} catch (e) {
user().error(TAG_, 'Caught exception executing listener for ' +
this.transportId_ + ': ' + e.message);
}
}

/**
* @returns {!string}
* @VisibleForTesting
*/
getTransportId() {
return this.transportId_;
}
}

1 change: 1 addition & 0 deletions build-system/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ module.exports = {
'!{node_modules,build,dist,dist.tools,' +
'dist.3p/[0-9]*,dist.3p/current-min}/**/*.*',
'!dist.3p/current/**/ampcontext-lib.js',
'!dist.3p/current/**/ampanalytics-lib.js',
'!validator/dist/**/*.*',
'!validator/node_modules/**/*.*',
'!validator/nodejs/node_modules/**/*.*',
Expand Down
1 change: 1 addition & 0 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ var forbiddenTerms = {
whitelist: [
'3p/integration.js',
'3p/ampcontext-lib.js',
'3p/ampanalytics-lib.js',
'ads/alp/install-alp.js',
'ads/inabox/inabox-host.js',
'dist.3p/current/integration.js',
Expand Down
40 changes: 40 additions & 0 deletions examples/analytics-iframe-transport-remote-frame.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Requests Frame</title>
<script>
/**
* To receive analytics events in a third-party frame, you must
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment is outdated

* implement this method, with this signature. Within that method, you
* must create a function which processes the analytics events, and
* pass that function to the registerAmpAnalytics3pEventsListener() method
* of the object which is passed as a parameter to
* onNewAmpAnalyticsInstance().
* @param ampAnalytics Call registerAmpAnalyticsEventListener() on this,
* passing your function which will receive the analytics events.
*/
window.onNewAmpAnalyticsInstance = ampAnalytics => {
ampAnalytics.registerCreativeEventListener(event => {
// Now, do something meaningful with the AMP Analytics event
console.log("The page at: " + window.location.href +
" has received an event: " + event +
" from the creative with transport ID: " +
ampAnalytics.getTransportId());
Copy link
Contributor

Choose a reason for hiding this comment

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

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));
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

});
};

// Load the script specified in the iframe’s name attribute:
const url = JSON.parse(window.name).scriptSrc;
if (url && url.startsWith('https://3p.ampproject.net/')) {
script = document.createElement('script');
script.src = url;
document.head.appendChild(script);
// The script will be loaded, and will call onNewAmpAnalyticsInstance()
} else {
console.warn('Received invalid URL - risk of XSS! ' + url);
}
</script>
</head>
<!-- The frame will not be visible, so there is no need for a body tag. -->
</html>
2 changes: 1 addition & 1 deletion extensions/amp-analytics/0.1/amp-analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ export class AmpAnalytics extends AMP.BaseElement {
// TODO(zhouyx, #7096) Track overwrite percentage. Prevent transport overwriting
if (inlineConfig['transport'] || this.remoteConfig_['transport']) {
const TAG = this.getName_();
user().error(TAG, 'Inline or remote config should not' +
user().error(TAG, 'Inline or remote config should not ' +
'overwrite vendor transport settings');
}
}
Expand Down
Loading