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
Changes from 76 commits
9a2c05e
c539c66
2f892c3
e754522
be880d6
8c76838
dbf2960
7e58280
5d46a9d
b6351f2
d15b9c5
fb539c8
f252fe1
ab3b882
8b47fb0
b291ea1
8373e55
e56dbed
83fa626
8b5f533
88d118d
5e4e3f7
558339f
305a84b
421b56e
c1b36b5
e111811
e590d9e
4c17cbe
3ce7c3b
fbdb4a8
f664d31
fcad58a
4c52b48
2920068
d5b2dae
7b12404
28c7b7a
409ac7c
ee822cf
f4f5048
0203122
bff17d9
0d64916
0e54a38
3d7dbd6
4889860
21d272f
f363bf1
284d363
c7898d7
352f1f8
904148d
56cc6a6
2cccfc3
cfa8c33
d8dde0f
98249eb
78098c5
fa9c3f3
ac5bdca
d96b644
0f03774
3ff0ae4
3092770
ac08ff3
8e4a0a3
29e94e5
ab3db5f
a7f05ea
2f9bec5
66d0cc4
545ac7b
c7b6c88
88b6c4c
015c48a
ef8fb8d
e7c0311
6ca88ee
1e8595d
3349b6b
bd9a9eb
39164c5
7b0a03b
d265980
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
/** | ||
* 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 {IframeMessagingClient} from './iframe-messaging-client'; | ||
|
||
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_ = 'iframe-transport-client'; | ||
|
||
/** | ||
* Receives event messages bound for this cross-domain iframe, from all | ||
* creatives | ||
*/ | ||
export class IframeTransportClient { | ||
|
||
/** @param {!Window} win */ | ||
constructor(win) { | ||
/** @private {!Window} */ | ||
this.win_ = win; | ||
|
||
// Necessary, or else check-types will complain "Property | ||
// processAmpAnalyticsEvent never defined on Window" | ||
this.win_.processAmpAnalyticsEvent = | ||
this.win_.processAmpAnalyticsEvent || null; | ||
|
||
/** @protected {!IframeMessagingClient} */ | ||
this.client_ = new IframeMessagingClient(win); | ||
this.client_.setHostWindow(this.win_.parent); | ||
this.client_.setSentinel(dev().assertString( | ||
tryParseJson(this.win_.name)['sentinel'], | ||
'Invalid/missing sentinel on iframe name attribute' + this.win_.name)); | ||
this.client_.makeRequest( | ||
IFRAME_TRANSPORT_EVENTS_TYPE, | ||
IFRAME_TRANSPORT_EVENTS_TYPE, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. back to my previous comment, i think it make sense to have 2 different message type
naming is after 3p-frame-messaging.js There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed by creating constants with different names, but same value. See: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That does not sound right. Can you investigate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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_ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All the current usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, to resemble code in provided link |
||
eventData => { | ||
const events = | ||
/** | ||
* @type | ||
* {!Array<../src/iframe-transport-common.IframeTransportEvent>} | ||
*/ | ||
(eventData['events']); | ||
user().assert(events, | ||
'Received malformed events list in ' + this.win_.location.href); | ||
dev().assert(events.length, | ||
'Received empty events list in ' + this.win_.location.href); | ||
user().assert(this.win_.processAmpAnalyticsEvent, | ||
'Must implement processAmpAnalyticsEvent in ' + | ||
this.win_.location.href); | ||
events.forEach(event => { | ||
try { | ||
this.win_.processAmpAnalyticsEvent(event.message, | ||
event.transportId); | ||
} catch (e) { | ||
user().error(TAG_, | ||
'Exception in processAmpAnalyticsEvent: ' + e.message); | ||
} | ||
}); | ||
}); | ||
} | ||
|
||
/** | ||
* Gets the IframeMessagingClient | ||
* @returns {!IframeMessagingClient} | ||
* @VisibleForTesting | ||
*/ | ||
getClient() { | ||
return this.client_; | ||
} | ||
} | ||
|
||
if (!window.AMP_TEST) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can follow ampcontext-lib.js to split this piece out of this file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
try { | ||
new IframeTransportClient(window); | ||
} catch (e) { | ||
user().error(TAG_, 'Failed to construct IframeTransportClient: ' + | ||
e.message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
<!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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.processAmpAnalyticsEvent = (event, transportId) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for simplifying. but this is a bit different from what I proposed.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, back to you. |
||
// 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: " + | ||
transportId); | ||
}; | ||
|
||
// 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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
/** | ||
* 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 { | ||
IFRAME_TRANSPORT_EVENT_MESSAGES_TYPE, | ||
} from '../../../../src/iframe-transport-common'; | ||
import { | ||
IframeTransportClient, | ||
CreativeEventRouter, | ||
} from '../../../../3p/iframe-transport-client'; | ||
import {dev, user} from '../../../../src/log'; | ||
import {adopt} from '../../../../src/runtime'; | ||
import * as sinon from 'sinon'; | ||
|
||
adopt(window); | ||
|
||
let nextId = 5000; | ||
function createUniqueId() { | ||
return String(++(nextId)); | ||
} | ||
|
||
describe('iframe-transport-client', () => { | ||
let sandbox; | ||
let badAssertsCounterStub; | ||
let router; | ||
let sentinel; | ||
|
||
beforeEach(() => { | ||
sandbox = sinon.sandbox.create(); | ||
badAssertsCounterStub = sandbox.stub(); | ||
sentinel = createUniqueId(); | ||
window.name = '{"sentinel": "' + sentinel + '"}'; | ||
router = new IframeTransportClient(window); | ||
sandbox.stub(dev(), 'assert', (condition, msg) => { | ||
if (!condition) { | ||
badAssertsCounterStub(msg); | ||
} | ||
}); | ||
sandbox.stub(user(), 'assert', (condition, msg) => { | ||
if (!condition) { | ||
badAssertsCounterStub(msg); | ||
} | ||
}); | ||
}); | ||
|
||
afterEach(() => { | ||
sandbox.restore(); | ||
}); | ||
|
||
/** | ||
* Sends a message from the current window to itself | ||
* @param {string} type Type of the message. | ||
* @param {!JsonObject} object Message payload. | ||
*/ | ||
function send(type, data) { | ||
const object = {}; | ||
object['type'] = type; | ||
object['sentinel'] = sentinel; | ||
if (data['events']) { | ||
object['events'] = data['events']; | ||
} else { | ||
object['data'] = data; | ||
} | ||
const payload = 'amp-' + JSON.stringify(object); | ||
window./*OK*/postMessage(payload, '*'); | ||
} | ||
|
||
it('fails to create router if no window.name ', () => { | ||
const oldWindowName = window.name; | ||
expect(() => { | ||
window.name = ''; | ||
new IframeTransportClient(window); | ||
}).to.throw(/Cannot read property 'sentinel' of undefined/); | ||
window.name = oldWindowName; | ||
}); | ||
|
||
it('sets sentinel from window.name.sentinel ', () => { | ||
expect(router.getClient().sentinel_).to.equal(sentinel); | ||
}); | ||
|
||
it('receives an event message ', () => { | ||
window.processAmpAnalyticsEvent = (event, transportId) => { | ||
expect(ampAnalytics.getTransportId()).to.equal('101'); | ||
expect(event).to.equal('hello, world!'); | ||
}; | ||
send(IFRAME_TRANSPORT_EVENT_MESSAGES_TYPE, /** @type {!JsonObject} */ ({ | ||
events: [ | ||
{transportId: '101', message: 'hello, world!'}, | ||
]})); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be user() instead of dev()? If it fails, I assume we cannot recover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We should probably change it at https://github.com/ampproject/amphtml/blob/master/3p/ampcontext.js#L103 also