From 487d029842e1356fa159ede64f4ee287987e8040 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Tue, 11 Oct 2016 08:49:21 -0700 Subject: [PATCH 01/13] wip --- build-system/server.js | 34 +++++++++++++++++++++++++++++++++- src/impression.js | 14 +++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index f00563830fd5..232dab1380b0 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -208,6 +208,7 @@ app.use('/share-tracking/get-outgoing-fragment', function(req, res) { function proxyToAmpProxy(req, res, minify) { var url = 'https://cdn.ampproject.org/c' + req.url; var localUrlPrefix = getUrlPrefix(req); + console.log('request request'); request(url, function (error, response, body) { body = body // Unversion URLs. @@ -222,7 +223,7 @@ function proxyToAmpProxy(req, res, minify) { localUrlPrefix + '/dist/v0/'); if (minify) { body = body.replace(/\.max\.js/g, '.js') - .replace('/dist/amp.js', '/dist/v0.js'); + .replace('/dist/amp.js', '/dist/v1.js'); } res.status(response.statusCode).send(body); }); @@ -404,6 +405,37 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) { next(); }); + +app.use('/impression-proxy/', function(req, res) { + //http.get + //console.log('url is ', req.get('impression-proxy')); + //const xhr = + + var fakeHeaders = { + 'Host': 'adclick.g.doubleclick.net', + 'Origin': 'https://cdn.ampproject.org', + 'Cookie': 'Cookie:id=4903ef7b59596f28||t=1451370221|et=730|cs=002213fd48e4405e4579c95dfa', + }; + + var options = { + url: 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236', + headers: fakeHeaders, + }; + +// function callback(error, response, body) { +// if (!error && response.statusCode == 200) { +// console.log(body); +// } +// } + + request(options, (error, response, body) => { + if (!error && response.statusCode == 200) { + console.log(body); + return body; + } + }); +}); + // Proxy with unminified JS. // Example: // http://localhost:8000/max/s/www.washingtonpost.com/amphtml/news/post-politics/wp/2016/02/21/bernie-sanders-says-lower-turnout-contributed-to-his-nevada-loss-to-hillary-clinton/ diff --git a/src/impression.js b/src/impression.js index 47418b423d4f..bf36138229be 100644 --- a/src/impression.js +++ b/src/impression.js @@ -18,6 +18,7 @@ import {user} from './log'; import {isExperimentOn} from './experiments'; import {viewerForDoc} from './viewer'; import {xhrFor} from './xhr'; +import {getMode} from './mode' /** @@ -29,9 +30,16 @@ export function maybeTrackImpression(win) { if (!isExperimentOn(win, 'alp')) { return; } + console.log('aaaaaaa'); const viewer = viewerForDoc(win.document); /** @const {string|undefined} */ - const clickUrl = viewer.getParam('click'); + let clickUrl = viewer.getParam('click'); + + //clickUrl = 'http://localhost:8000/local-proxy?url=https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; + clickUrl = 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; + //clickUrl = 'https://0.cat2.eventfe.cafe.content-ads-owners.pc.borg.google.com/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; + //clickUrl = "https%3A%2F%2Fadclick.g.doubleclick.net%2Fpcs%2Fclick%3Famp%3D1%26xai%3DAKAOjsv-m1m4UWV9Ma-712CSL1GAlk8LqQBnsR7qaiMk9RHySNoeynJlw-hKVfH6tWAQSc4_ksp0eIKEpASP9Hg52B1ZQfwZebVkFSw_leOH5LpYrdI8CcKVzdDDahoDwi9BWpxj1hBELNiD2yuq4yKHGqhwmwYAtPLBQSzRoyPJyP5WYjp4YdKMGjDLT_mUmL7zCwbX9FgKNupSIkGoghqLtTIPEdPPd0y-CBN04s3sH9vuUnBD4OdAQ9i0Ty1Lf94KEYo%26sig%3DCg0ArKJSzG321ESes8LKEAE%26urlfix%3D1%26adurl%3Dhttps%3A%2F%2Fcdn.ampproject.org%2Fc%2Fs%2Fwww.buzzfeed.com%2Famphtml%2Fstephaniemcneal%2Fyou-can-use-this-website-to-make-president-obama-say-whateve"; + //clickUrl = 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=abc…&sig=xyz…&adurl=https://cdn.ampproject.org/c/events.latimes.com/festivalofbooks/&nm=3&nx=139&ny=132&mb=33&clkt=2'; if (!clickUrl) { return; } @@ -47,6 +55,9 @@ export function maybeTrackImpression(win) { // avoid duplicate tracking. win.location.hash = ''; } + if (getMode().localDev) { + clickUrl = 'http://localhost:8000/impression-proxy?url=' + clickUrl; + } viewer.whenFirstVisible().then(() => { invoke(win, clickUrl); }); @@ -57,5 +68,6 @@ function invoke(win, clickUrl) { credentials: 'include', requireAmpResponseSourceOrigin: true, }); + console.log('invoke'); // TODO(@cramforce): Do something with the result. } From fb805e42f3989692d1871b9316f1b68b82b152ef Mon Sep 17 00:00:00 2001 From: zhouyx Date: Tue, 11 Oct 2016 11:13:36 -0700 Subject: [PATCH 02/13] get response back from proxy server --- build-system/server.js | 5 ++++- src/impression.js | 3 +++ src/service/xhr-impl.js | 3 ++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index 232dab1380b0..2e77a8744382 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -410,7 +410,7 @@ app.use('/impression-proxy/', function(req, res) { //http.get //console.log('url is ', req.get('impression-proxy')); //const xhr = - + assertCors(req, res, ['GET']); var fakeHeaders = { 'Host': 'adclick.g.doubleclick.net', 'Origin': 'https://cdn.ampproject.org', @@ -431,7 +431,10 @@ app.use('/impression-proxy/', function(req, res) { request(options, (error, response, body) => { if (!error && response.statusCode == 200) { console.log(body); + res.send(body); return body; + } else { + console.log(response.statusCode); } }); }); diff --git a/src/impression.js b/src/impression.js index bf36138229be..ccf186b55f6b 100644 --- a/src/impression.js +++ b/src/impression.js @@ -67,6 +67,9 @@ function invoke(win, clickUrl) { xhrFor(win).fetchJson(clickUrl, { credentials: 'include', requireAmpResponseSourceOrigin: true, + }).then(a => { + console.log('fetchJson return'); + console.log(a); }); console.log('invoke'); // TODO(@cramforce): Do something with the result. diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js index 0d9ce3d69d1b..03de15fc22ab 100644 --- a/src/service/xhr-impl.js +++ b/src/service/xhr-impl.js @@ -162,8 +162,9 @@ export class Xhr { const init = opt_init || {}; init.method = normalizeMethod_(init.method); setupJson_(init); - + console.log('in fetchJson'); return this.fetchAmpCors_(input, init).then(response => { + console.log('fetch response is ', response); return assertSuccess(response); }).then(response => response.json()); } From 9cc6b0b72504e41735b38aab3c91c47cfaed1d20 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 13 Oct 2016 18:49:46 -0700 Subject: [PATCH 03/13] first impl --- build-system/server.js | 21 +------ src/impression.js | 78 ++++++++++++++++++++----- src/service/xhr-impl.js | 2 - test/functional/test-impression.js | 92 ++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 35 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index 2e77a8744382..fe16d30eaf84 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -208,7 +208,6 @@ app.use('/share-tracking/get-outgoing-fragment', function(req, res) { function proxyToAmpProxy(req, res, minify) { var url = 'https://cdn.ampproject.org/c' + req.url; var localUrlPrefix = getUrlPrefix(req); - console.log('request request'); request(url, function (error, response, body) { body = body // Unversion URLs. @@ -223,7 +222,7 @@ function proxyToAmpProxy(req, res, minify) { localUrlPrefix + '/dist/v0/'); if (minify) { body = body.replace(/\.max\.js/g, '.js') - .replace('/dist/amp.js', '/dist/v1.js'); + .replace('/dist/amp.js', '/dist/v0.js'); } res.status(response.statusCode).send(body); }); @@ -407,34 +406,20 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) { app.use('/impression-proxy/', function(req, res) { - //http.get - //console.log('url is ', req.get('impression-proxy')); - //const xhr = assertCors(req, res, ['GET']); - var fakeHeaders = { + const fakeHeaders = { 'Host': 'adclick.g.doubleclick.net', 'Origin': 'https://cdn.ampproject.org', 'Cookie': 'Cookie:id=4903ef7b59596f28||t=1451370221|et=730|cs=002213fd48e4405e4579c95dfa', }; - - var options = { + const options = { url: 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236', headers: fakeHeaders, }; -// function callback(error, response, body) { -// if (!error && response.statusCode == 200) { -// console.log(body); -// } -// } - request(options, (error, response, body) => { if (!error && response.statusCode == 200) { - console.log(body); res.send(body); - return body; - } else { - console.log(response.statusCode); } }); }); diff --git a/src/impression.js b/src/impression.js index ccf186b55f6b..bffc0894f720 100644 --- a/src/impression.js +++ b/src/impression.js @@ -18,7 +18,8 @@ import {user} from './log'; import {isExperimentOn} from './experiments'; import {viewerForDoc} from './viewer'; import {xhrFor} from './xhr'; -import {getMode} from './mode' +import {getMode} from './mode'; +import {openWindowDialog} from './dom'; /** @@ -30,16 +31,13 @@ export function maybeTrackImpression(win) { if (!isExperimentOn(win, 'alp')) { return; } - console.log('aaaaaaa'); + const viewer = viewerForDoc(win.document); /** @const {string|undefined} */ let clickUrl = viewer.getParam('click'); - //clickUrl = 'http://localhost:8000/local-proxy?url=https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; + // One test clickUrl clickUrl = 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; - //clickUrl = 'https://0.cat2.eventfe.cafe.content-ads-owners.pc.borg.google.com/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; - //clickUrl = "https%3A%2F%2Fadclick.g.doubleclick.net%2Fpcs%2Fclick%3Famp%3D1%26xai%3DAKAOjsv-m1m4UWV9Ma-712CSL1GAlk8LqQBnsR7qaiMk9RHySNoeynJlw-hKVfH6tWAQSc4_ksp0eIKEpASP9Hg52B1ZQfwZebVkFSw_leOH5LpYrdI8CcKVzdDDahoDwi9BWpxj1hBELNiD2yuq4yKHGqhwmwYAtPLBQSzRoyPJyP5WYjp4YdKMGjDLT_mUmL7zCwbX9FgKNupSIkGoghqLtTIPEdPPd0y-CBN04s3sH9vuUnBD4OdAQ9i0Ty1Lf94KEYo%26sig%3DCg0ArKJSzG321ESes8LKEAE%26urlfix%3D1%26adurl%3Dhttps%3A%2F%2Fcdn.ampproject.org%2Fc%2Fs%2Fwww.buzzfeed.com%2Famphtml%2Fstephaniemcneal%2Fyou-can-use-this-website-to-make-president-obama-say-whateve"; - //clickUrl = 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=abc…&sig=xyz…&adurl=https://cdn.ampproject.org/c/events.latimes.com/festivalofbooks/&nm=3&nx=139&ny=132&mb=33&clkt=2'; if (!clickUrl) { return; } @@ -55,22 +53,72 @@ export function maybeTrackImpression(win) { // avoid duplicate tracking. win.location.hash = ''; } - if (getMode().localDev) { - clickUrl = 'http://localhost:8000/impression-proxy?url=' + clickUrl; - } + viewer.whenFirstVisible().then(() => { - invoke(win, clickUrl); + // TODO(@zhouyx) We need a timeout here? + // TODO(@zhouyx) need test with a real response. + invoke(win, clickUrl).then(response => { + if (!response) { + return; + } + applyResponse(win, viewer, response); + }); }); } +/** + * Send the url to ad server and wait for its response + * @param {!Window} win + * @param {string} clickUrl + * @return {!Promise} + */ function invoke(win, clickUrl) { - xhrFor(win).fetchJson(clickUrl, { + if (getMode().localDev && !getMode().test) { + clickUrl = 'http://localhost:8000/impression-proxy?url=' + clickUrl; + } + return xhrFor(win).fetchJson(clickUrl, { credentials: 'include', requireAmpResponseSourceOrigin: true, - }).then(a => { - console.log('fetchJson return'); - console.log(a); }); - console.log('invoke'); // TODO(@cramforce): Do something with the result. } + +/** + * parse the response back from ad server + * Set for analytics purposes + * @param {!Window} win + * @param {!Object} response + */ +function applyResponse(win, viewer, response) { + const adLocation = response['location']; + + // If there's a tracking_url, need to redirect to that url. + const adTrackingUrl = response['tracking_url']; + if (adTrackingUrl) { + // TODO(@zhouyx) Confirm we assume the tracking_url is not + // a cdn.ampproject page, and we don't do viewer redirect + openWindowDialog(win, adTrackingUrl, '_top'); + return; + } + + // If adLocation is not an amp page, need to redirect to it. + if (adLocation && adLocation.indexOf('https://cdn.ampproject.org') != 0) { + // TODO(@zhouyx) Confirm we assume the tracking_url is not + // a cdn.ampproject page, and we don't do viewer redirect + openWindowDialog(win, adLocation, '_top'); + return; + } + + // If have gclid, or location contains gclid. add it to location hash + // Set gclid. + // If provided use it, if not try to find in adLocation from response + let gclid = response['gclid']; + if (!gclid) { + if (adLocation.indexOf('gclid=') > 0) { + gclid = adLocation.substr(adLocation.indexOf('gclid=') + 6); + } + } + if (gclid) { + win.location.hash = '#gclid=' + gclid; + } +} diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js index 03de15fc22ab..6058fdfe4794 100644 --- a/src/service/xhr-impl.js +++ b/src/service/xhr-impl.js @@ -162,9 +162,7 @@ export class Xhr { const init = opt_init || {}; init.method = normalizeMethod_(init.method); setupJson_(init); - console.log('in fetchJson'); return this.fetchAmpCors_(input, init).then(response => { - console.log('fetch response is ', response); return assertSuccess(response); }).then(response => response.json()); } diff --git a/test/functional/test-impression.js b/test/functional/test-impression.js index 50581b5e47ec..74d419d4b37b 100644 --- a/test/functional/test-impression.js +++ b/test/functional/test-impression.js @@ -18,6 +18,7 @@ import {maybeTrackImpression} from '../../src/impression'; import {toggleExperiment} from '../../src/experiments'; import {viewerForDoc} from '../../src/viewer'; import {xhrFor} from '../../src/xhr'; +import * as dom from '../../src/dom'; import * as sinon from 'sinon'; describe('impression', () => { @@ -80,4 +81,95 @@ describe('impression', () => { }); }); }); + + it('should redirect if xhr return tracking url', () => { + toggleExperiment(window, 'alp', true); + viewer.getParam.withArgs('click').returns('https://www.example.com'); + + const openWindowDialogSpy = sandbox.stub(dom, 'openWindowDialog', + (win, url, target) => { + expect(url).to.equal('test_tracking_url'); + expect(target).to.equal('_top'); + }); + + xhr.fetchJson = () => { + return Promise.resolve({ + 'location': 'test_location', + 'tracking_url': 'test_tracking_url', + }); + }; + + maybeTrackImpression(window); + return Promise.resolve().then(() => { + return Promise.resolve().then(() => { + expect(openWindowDialogSpy).to.be.calledOnce; + }); + }); + }); + + it('should redirect if location is some other url', () => { + toggleExperiment(window, 'alp', true); + viewer.getParam.withArgs('click').returns('https://www.example.com'); + + const openWindowDialogSpy = sandbox.stub(dom, 'openWindowDialog', + (win, url, target) => { + expect(url).to.equal('test_location'); + expect(target).to.equal('_top'); + }); + + xhr.fetchJson = () => { + return Promise.resolve({ + 'location': 'test_location', + }); + }; + + maybeTrackImpression(window); + return Promise.resolve().then(() => { + return Promise.resolve().then(() => { + expect(openWindowDialogSpy).to.be.calledOnce; + }); + }); + }); + + it('should not redirect if location is valid', () => { + toggleExperiment(window, 'alp', true); + viewer.getParam.withArgs('click').returns('https://www.example.com'); + + const openWindowDialogSpy = sandbox.spy(dom, 'openWindowDialog'); + + xhr.fetchJson = () => { + return Promise.resolve({ + 'location': 'https://cdn.ampproject.org/c/test/?gclid=654321', + }); + }; + + maybeTrackImpression(window); + return Promise.resolve().then(() => { + return Promise.resolve().then(() => { + expect(openWindowDialogSpy).to.not.be.called; + expect(window.location.hash).to.equal('#gclid=654321'); + }); + }); + }); + + it('should set gclid to location hash', () => { + toggleExperiment(window, 'alp', true); + viewer.getParam.withArgs('click').returns('https://www.example.com'); + + const openWindowDialogSpy = sandbox.spy(dom, 'openWindowDialog'); + + xhr.fetchJson = () => { + return Promise.resolve({ + 'gclid': '123456', + }); + }; + + maybeTrackImpression(window); + return Promise.resolve().then(() => { + return Promise.resolve().then(() => { + expect(openWindowDialogSpy).to.not.be.called; + expect(window.location.hash).to.be.equal('#gclid=123456'); + }); + }); + }); }); From dcb8cb25d23dc6389eeee90b7350f8434502d19f Mon Sep 17 00:00:00 2001 From: zhouyx Date: Mon, 17 Oct 2016 10:48:48 -0700 Subject: [PATCH 04/13] another approach --- build-system/server.js | 20 ++---- src/impression.js | 69 +++++++++---------- src/service/url-replacements-impl.js | 31 +++++++++ test/functional/test-impression.js | 87 +++--------------------- test/functional/test-url-replacements.js | 34 ++++++++- 5 files changed, 113 insertions(+), 128 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index fe16d30eaf84..a791c672bc14 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -407,21 +407,13 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) { app.use('/impression-proxy/', function(req, res) { assertCors(req, res, ['GET']); - const fakeHeaders = { - 'Host': 'adclick.g.doubleclick.net', - 'Origin': 'https://cdn.ampproject.org', - 'Cookie': 'Cookie:id=4903ef7b59596f28||t=1451370221|et=730|cs=002213fd48e4405e4579c95dfa', + // TODO(@zhouyx): How to request real ad server response with cookie set + const body = { + 'location': 'localhost:8000/examples/#gclid=1234', + 'gclid': '1234', + 'tracking': 'tracking_url', }; - const options = { - url: 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236', - headers: fakeHeaders, - }; - - request(options, (error, response, body) => { - if (!error && response.statusCode == 200) { - res.send(body); - } - }); + res.send(body); }); // Proxy with unminified JS. diff --git a/src/impression.js b/src/impression.js index bffc0894f720..254ab6a82ece 100644 --- a/src/impression.js +++ b/src/impression.js @@ -14,13 +14,16 @@ * limitations under the License. */ -import {user} from './log'; +import {dev, user} from './log'; import {isExperimentOn} from './experiments'; import {viewerForDoc} from './viewer'; import {xhrFor} from './xhr'; import {getMode} from './mode'; -import {openWindowDialog} from './dom'; +/** A promise that resolve after getting info from ad server + * {Promise=} + */ +export let trackImpressionPromise; /** * Emit a HTTP request to a destination defined on the incoming URL. @@ -28,23 +31,30 @@ import {openWindowDialog} from './dom'; * @param {!Window} win */ export function maybeTrackImpression(win) { + let resolveImpression; + + trackImpressionPromise = new Promise(resolve => { + resolveImpression = resolve; + }); + if (!isExperimentOn(win, 'alp')) { + resolveImpression(); return; } const viewer = viewerForDoc(win.document); /** @const {string|undefined} */ - let clickUrl = viewer.getParam('click'); + const clickUrl = viewer.getParam('click'); - // One test clickUrl - clickUrl = 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; if (!clickUrl) { + resolveImpression(); return; } if (clickUrl.indexOf('https://') != 0) { user().warn('Impression', 'click fragment param should start with https://. Found ', clickUrl); + resolveImpression(); return; } if (win.location.hash) { @@ -57,11 +67,9 @@ export function maybeTrackImpression(win) { viewer.whenFirstVisible().then(() => { // TODO(@zhouyx) We need a timeout here? // TODO(@zhouyx) need test with a real response. - invoke(win, clickUrl).then(response => { - if (!response) { - return; - } + invoke(win, dev().assertString(clickUrl)).then(response => { applyResponse(win, viewer, response); + resolveImpression(); }); }); } @@ -70,7 +78,7 @@ export function maybeTrackImpression(win) { * Send the url to ad server and wait for its response * @param {!Window} win * @param {string} clickUrl - * @return {!Promise} + * @return {!Promise} */ function invoke(win, clickUrl) { if (getMode().localDev && !getMode().test) { @@ -80,7 +88,6 @@ function invoke(win, clickUrl) { credentials: 'include', requireAmpResponseSourceOrigin: true, }); - // TODO(@cramforce): Do something with the result. } /** @@ -91,34 +98,24 @@ function invoke(win, clickUrl) { */ function applyResponse(win, viewer, response) { const adLocation = response['location']; + const adTracking = response['tracking']; - // If there's a tracking_url, need to redirect to that url. - const adTrackingUrl = response['tracking_url']; - if (adTrackingUrl) { - // TODO(@zhouyx) Confirm we assume the tracking_url is not - // a cdn.ampproject page, and we don't do viewer redirect - openWindowDialog(win, adTrackingUrl, '_top'); - return; + // If there is a tracking_url, need to track it + // Otherwise track the location + const trackUrl = adTracking || adLocation; + if (trackUrl) { + new Image().src = trackUrl; } - // If adLocation is not an amp page, need to redirect to it. - if (adLocation && adLocation.indexOf('https://cdn.ampproject.org') != 0) { - // TODO(@zhouyx) Confirm we assume the tracking_url is not - // a cdn.ampproject page, and we don't do viewer redirect - openWindowDialog(win, adLocation, '_top'); - return; - } - - // If have gclid, or location contains gclid. add it to location hash - // Set gclid. - // If provided use it, if not try to find in adLocation from response - let gclid = response['gclid']; - if (!gclid) { - if (adLocation.indexOf('gclid=') > 0) { - gclid = adLocation.substr(adLocation.indexOf('gclid=') + 6); + // If have we have gclid replace the location href with new location we get. + // TODO(@zhouyx) should we replaceState directly w/o checking gclid? + const gclid = response['gclid']; + if (gclid && adLocation) { + if (getMode().localDev && !getMode().test) { + win.history.replaceState(null, '', win.location.href + + '#gclid=' + gclid); + return; } - } - if (gclid) { - win.location.hash = '#gclid=' + gclid; + win.history.replaceState(null, '', adLocation); } } diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 341bf295f2b4..7318819a0b91 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -29,6 +29,7 @@ import {viewportForDoc} from '../viewport'; import {userNotificationManagerFor} from '../user-notification'; import {activityFor} from '../activity'; import {isExperimentOn} from '../experiments'; +import {trackImpressionPromise} from '../impression.js'; /** @private @const {string} */ @@ -163,6 +164,18 @@ export class UrlReplacements { return removeFragment(info.sourceUrl); })); + this.setAsync_('SOURCE_URL', () => { + let promise = trackImpressionPromise; + if (!promise) { + promise = Promise.resolve(); + } + return promise.then( + this.getDocInfoValue_.bind(this, info => { + return removeFragment(info.sourceUrl); + }) + ); + }); + // Returns the host of the Source URL for this AMP document. this.set_('SOURCE_HOST', this.getDocInfoValue_.bind(this, info => { return parseUrl(info.sourceUrl).host; @@ -197,6 +210,24 @@ export class UrlReplacements { defaultValue; }); + this.setAsync_('QUERY_PARAM', (param, defaultValue = '') => { + user().assert(param, + 'The first argument to QUERY_PARAM, the query string ' + + 'param is required'); + let promise = trackImpressionPromise; + if (!promise) { + promise = Promise.resolve(); + } + return promise.then(() => { + const url = parseUrl(this.ampdoc.win.location.href); + const params = parseQueryString(url.search); + + return (typeof params[param] !== 'undefined') ? + params[param] : + defaultValue; + }); + }); + /** * Stores client ids that were generated during this page view * indexed by scope. diff --git a/test/functional/test-impression.js b/test/functional/test-impression.js index 74d419d4b37b..cda05f0cbae8 100644 --- a/test/functional/test-impression.js +++ b/test/functional/test-impression.js @@ -14,11 +14,13 @@ * limitations under the License. */ -import {maybeTrackImpression} from '../../src/impression'; +import { + trackImpressionPromise, + maybeTrackImpression, +} from '../../src/impression'; import {toggleExperiment} from '../../src/experiments'; import {viewerForDoc} from '../../src/viewer'; import {xhrFor} from '../../src/xhr'; -import * as dom from '../../src/dom'; import * as sinon from 'sinon'; describe('impression', () => { @@ -49,6 +51,7 @@ describe('impression', () => { it('should do nothing if the experiment is off', () => { viewer.getParam.throws(new Error('Should not be called')); maybeTrackImpression(window); + return trackImpressionPromise.should.be.fulfilled; }); it('should do nothing if there is no click arg', () => { @@ -56,6 +59,7 @@ describe('impression', () => { viewer.getParam.withArgs('click').returns(''); maybeTrackImpression(window); expect(xhr.fetchJson.callCount).to.equal(0); + return trackImpressionPromise.should.be.fulfilled; }); it('should do nothing if there is the click arg is http', () => { @@ -63,6 +67,7 @@ describe('impression', () => { viewer.getParam.withArgs('click').returns('http://www.example.com'); maybeTrackImpression(window); expect(xhr.fetchJson.callCount).to.equal(0); + return trackImpressionPromise.should.be.fulfilled; }); it('should invoke URL', () => { @@ -82,84 +87,12 @@ describe('impression', () => { }); }); - it('should redirect if xhr return tracking url', () => { + it('should replace location href', () => { toggleExperiment(window, 'alp', true); viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.stub(dom, 'openWindowDialog', - (win, url, target) => { - expect(url).to.equal('test_tracking_url'); - expect(target).to.equal('_top'); - }); - - xhr.fetchJson = () => { - return Promise.resolve({ - 'location': 'test_location', - 'tracking_url': 'test_tracking_url', - }); - }; - - maybeTrackImpression(window); - return Promise.resolve().then(() => { - return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.be.calledOnce; - }); - }); - }); - - it('should redirect if location is some other url', () => { - toggleExperiment(window, 'alp', true); - viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.stub(dom, 'openWindowDialog', - (win, url, target) => { - expect(url).to.equal('test_location'); - expect(target).to.equal('_top'); - }); - xhr.fetchJson = () => { return Promise.resolve({ 'location': 'test_location', - }); - }; - - maybeTrackImpression(window); - return Promise.resolve().then(() => { - return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.be.calledOnce; - }); - }); - }); - - it('should not redirect if location is valid', () => { - toggleExperiment(window, 'alp', true); - viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.spy(dom, 'openWindowDialog'); - - xhr.fetchJson = () => { - return Promise.resolve({ - 'location': 'https://cdn.ampproject.org/c/test/?gclid=654321', - }); - }; - - maybeTrackImpression(window); - return Promise.resolve().then(() => { - return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.not.be.called; - expect(window.location.hash).to.equal('#gclid=654321'); - }); - }); - }); - - it('should set gclid to location hash', () => { - toggleExperiment(window, 'alp', true); - viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.spy(dom, 'openWindowDialog'); - - xhr.fetchJson = () => { - return Promise.resolve({ 'gclid': '123456', }); }; @@ -167,8 +100,8 @@ describe('impression', () => { maybeTrackImpression(window); return Promise.resolve().then(() => { return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.not.be.called; - expect(window.location.hash).to.be.equal('#gclid=123456'); + expect(window.location.href).to.contain('test_location'); + return trackImpressionPromise.should.be.fulfilled; }); }); }); diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index c972b3ec7177..54fff06797fb 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -33,6 +33,7 @@ import {setCookie} from '../../src/cookies'; import {parseUrl} from '../../src/url'; import {toggleExperiment} from '../../src/experiments'; import {viewerForDoc} from '../../src/viewer'; +import * as trackPromise from '../../src/impression'; import * as sinon from 'sinon'; @@ -198,6 +199,7 @@ describe('UrlReplacements', () => { }); it('should replace SOURCE_URL and _HOST', () => { + sandbox.stub(trackPromise, 'trackImpressionPromise', Promise.resolve()); return expandAsync('?url=SOURCE_URL&host=SOURCE_HOST').then(res => { expect(res).to.not.match(/SOURCE_URL/); expect(res).to.not.match(/SOURCE_HOST/); @@ -205,12 +207,32 @@ describe('UrlReplacements', () => { }); it('should replace SOURCE_URL and _HOSTNAME', () => { + sandbox.stub(trackPromise, 'trackImpressionPromise', Promise.resolve()); return expandAsync('?url=SOURCE_URL&host=SOURCE_HOSTNAME').then(res => { expect(res).to.not.match(/SOURCE_URL/); expect(res).to.not.match(/SOURCE_HOSTNAME/); }); }); + it('should update SOURCE_URL after track impression', () => { + const win = getFakeWindow(); + win.location = parseUrl('https://wrong.com'); + const clock = sandbox.useFakeTimers(); + sandbox.stub(trackPromise, 'trackImpressionPromise', + new Promise(resolve => { + setTimeout(() => { + win.location = parseUrl('https://example.com#gclid=123456'); + resolve(); + }, 1000); + })); + clock.tick(1001); + return installUrlReplacementsServiceForDoc(win.ampdoc) + .expandAsync('?url=SOURCE_URL') + .then(res => { + expect(res).to.contain('example.com'); + }); + }); + it('should replace SOURCE_PATH', () => { return expandAsync('?path=SOURCE_PATH').then(res => { expect(res).to.not.match(/SOURCE_PATH/); @@ -727,7 +749,17 @@ describe('UrlReplacements', () => { it('should replace QUERY_PARAM with foo', () => { const win = getFakeWindow(); - win.location = parseUrl('https://example.com?query_string_param1=foo'); + win.location = parseUrl('https://example.com?query_string_param1=wrong'); + const clock = sandbox.useFakeTimers(); + sandbox.stub(trackPromise, 'trackImpressionPromise', + new Promise(resolve => { + setTimeout(() => { + win.location = + parseUrl('https://example.com?query_string_param1=foo'); + resolve(); + }, 1000); + })); + clock.tick(1001); return installUrlReplacementsServiceForDoc(win.ampdoc) .expandAsync('?sh=QUERY_PARAM(query_string_param1)&s') .then(res => { From 281e0a2298c452bf4c4e6b7b1f887954111be4bb Mon Sep 17 00:00:00 2001 From: zhouyx Date: Tue, 18 Oct 2016 15:44:58 -0700 Subject: [PATCH 05/13] fix --- src/impression.js | 38 ++++++++------ src/service/url-replacements-impl.js | 45 ++++++----------- test/functional/test-impression.js | 63 ++++++++++++++++++++---- test/functional/test-url-replacements.js | 51 +++++++++++-------- 4 files changed, 123 insertions(+), 74 deletions(-) diff --git a/src/impression.js b/src/impression.js index 254ab6a82ece..9526a21d6b05 100644 --- a/src/impression.js +++ b/src/impression.js @@ -18,12 +18,24 @@ import {dev, user} from './log'; import {isExperimentOn} from './experiments'; import {viewerForDoc} from './viewer'; import {xhrFor} from './xhr'; +import { + isProxyOrigin, + parseUrl, + parseQueryString, + addParamsToUrl, +} from './url'; import {getMode} from './mode'; -/** A promise that resolve after getting info from ad server - * {Promise=} +let trackImpressionPromise = null; + +/** + * A function to get the trackImpressionPromise; + * @return {!Promise} */ -export let trackImpressionPromise; +export function getTrackImpressionPromise() { + dev().assert(trackImpressionPromise); + return trackImpressionPromise; +} /** * Emit a HTTP request to a destination defined on the incoming URL. @@ -103,19 +115,17 @@ function applyResponse(win, viewer, response) { // If there is a tracking_url, need to track it // Otherwise track the location const trackUrl = adTracking || adLocation; - if (trackUrl) { + + if (trackUrl && !isProxyOrigin(trackUrl)) { new Image().src = trackUrl; } - // If have we have gclid replace the location href with new location we get. - // TODO(@zhouyx) should we replaceState directly w/o checking gclid? - const gclid = response['gclid']; - if (gclid && adLocation) { - if (getMode().localDev && !getMode().test) { - win.history.replaceState(null, '', win.location.href + - '#gclid=' + gclid); - return; - } - win.history.replaceState(null, '', adLocation); + // Replace the location href params with new location params we get. + if (adLocation) { + const currentHref = win.location.href; + const url = parseUrl(adLocation); + const params = parseQueryString(url.search); + const newHref = addParamsToUrl(currentHref, params); + win.history.replaceState(null, '', newHref); } } diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 7318819a0b91..0917c187a510 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -29,7 +29,7 @@ import {viewportForDoc} from '../viewport'; import {userNotificationManagerFor} from '../user-notification'; import {activityFor} from '../activity'; import {isExperimentOn} from '../experiments'; -import {trackImpressionPromise} from '../impression.js'; +import {getTrackImpressionPromise} from '../impression.js'; /** @private @const {string} */ @@ -165,11 +165,7 @@ export class UrlReplacements { })); this.setAsync_('SOURCE_URL', () => { - let promise = trackImpressionPromise; - if (!promise) { - promise = Promise.resolve(); - } - return promise.then( + return getTrackImpressionPromise().then( this.getDocInfoValue_.bind(this, info => { return removeFragment(info.sourceUrl); }) @@ -199,32 +195,12 @@ export class UrlReplacements { })); this.set_('QUERY_PARAM', (param, defaultValue = '') => { - user().assert(param, - 'The first argument to QUERY_PARAM, the query string ' + - 'param is required'); - const url = parseUrl(this.ampdoc.win.location.href); - const params = parseQueryString(url.search); - - return (typeof params[param] !== 'undefined') ? - params[param] : - defaultValue; + return this.getQueryParamData_(param, defaultValue, /* opt_sync */ true); }); this.setAsync_('QUERY_PARAM', (param, defaultValue = '') => { - user().assert(param, - 'The first argument to QUERY_PARAM, the query string ' + - 'param is required'); - let promise = trackImpressionPromise; - if (!promise) { - promise = Promise.resolve(); - } - return promise.then(() => { - const url = parseUrl(this.ampdoc.win.location.href); - const params = parseQueryString(url.search); - - return (typeof params[param] !== 'undefined') ? - params[param] : - defaultValue; + return getTrackImpressionPromise().then(() => { + return this.getQueryParamData_(param, defaultValue); }); }); @@ -571,6 +547,17 @@ export class UrlReplacements { return navigationInfo[attribute]; } + getQueryParamData_(param, defaultValue, opt_sync) { + user().assert(param, + 'The first argument to QUERY_PARAM, the query string ' + + 'param is required'); + const url = parseUrl(this.ampdoc.win.location.href); + const params = parseQueryString(url.search); + return (typeof params[param] !== 'undefined') ? + params[param] : + defaultValue; + } + /** * * Sets a synchronous value resolver for the variable with the specified name. diff --git a/test/functional/test-impression.js b/test/functional/test-impression.js index cda05f0cbae8..b12e8a061fb4 100644 --- a/test/functional/test-impression.js +++ b/test/functional/test-impression.js @@ -15,7 +15,7 @@ */ import { - trackImpressionPromise, + getTrackImpressionPromise, maybeTrackImpression, } from '../../src/impression'; import {toggleExperiment} from '../../src/experiments'; @@ -51,7 +51,7 @@ describe('impression', () => { it('should do nothing if the experiment is off', () => { viewer.getParam.throws(new Error('Should not be called')); maybeTrackImpression(window); - return trackImpressionPromise.should.be.fulfilled; + return getTrackImpressionPromise().should.be.fulfilled; }); it('should do nothing if there is no click arg', () => { @@ -59,7 +59,7 @@ describe('impression', () => { viewer.getParam.withArgs('click').returns(''); maybeTrackImpression(window); expect(xhr.fetchJson.callCount).to.equal(0); - return trackImpressionPromise.should.be.fulfilled; + return getTrackImpressionPromise().should.be.fulfilled; }); it('should do nothing if there is the click arg is http', () => { @@ -67,7 +67,7 @@ describe('impression', () => { viewer.getParam.withArgs('click').returns('http://www.example.com'); maybeTrackImpression(window); expect(xhr.fetchJson.callCount).to.equal(0); - return trackImpressionPromise.should.be.fulfilled; + return getTrackImpressionPromise().should.be.fulfilled; }); it('should invoke URL', () => { @@ -87,21 +87,64 @@ describe('impression', () => { }); }); - it('should replace location href', () => { + it('should do nothing if response is not received', () => { toggleExperiment(window, 'alp', true); viewer.getParam.withArgs('click').returns('https://www.example.com'); + xhr.fetchJson = () => { + setTimeout(() => { + return Promise.resolve({ + 'location': 'test_location?gclid=654321', + }); + }, 5000); + }; + const clock = sandbox.useFakeTimers(); + const promise = new Promise(resolve => { + setTimeout(() => { + resolve(); + }, 2000); + }); + clock.tick(2001); + return promise.then(() => { + expect(window.location.href).to.not.contain('gclid=654321'); + xhr.fetchJson = () => { + return Promise.resolve(); + }; + }); + }); + + it('should do nothing if get empty response', () => { + toggleExperiment(window, 'alo', true); + viewer.getParam.withArgs('click').returns('https://www.example.com'); + const prevHref = window.location.href; + maybeTrackImpression(window); + return Promise.resolve().then(() => { + return Promise.resolve().then(() => { + expect(window.location.href).to.equal(prevHref); + return getTrackImpressionPromise().should.be.fulfilled; + }); + }); + }); + + it('should replace location href only with query params', () => { + toggleExperiment(window, 'alp', true); + viewer.getParam.withArgs('click').returns('https://www.example.com'); + xhr.fetchJson = () => { return Promise.resolve({ - 'location': 'test_location', - 'gclid': '123456', + 'location': 'test_location?gclid=123456', }); }; - + const prevHref = window.location.href; maybeTrackImpression(window); return Promise.resolve().then(() => { return Promise.resolve().then(() => { - expect(window.location.href).to.contain('test_location'); - return trackImpressionPromise.should.be.fulfilled; + expect(window.location.href).to.contain('gclid=123456'); + expect(window.location.href).to.not.contain('test_location'); + xhr.fetchJson = () => { + return Promise.resolve(); + }; + window.history.replaceState(null, '', prevHref); + return getTrackImpressionPromise().should.be.fulfilled; }); }); }); diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index 54fff06797fb..36f538d0e182 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -199,7 +199,9 @@ describe('UrlReplacements', () => { }); it('should replace SOURCE_URL and _HOST', () => { - sandbox.stub(trackPromise, 'trackImpressionPromise', Promise.resolve()); + sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { + return Promise.resolve(); + }); return expandAsync('?url=SOURCE_URL&host=SOURCE_HOST').then(res => { expect(res).to.not.match(/SOURCE_URL/); expect(res).to.not.match(/SOURCE_HOST/); @@ -207,7 +209,9 @@ describe('UrlReplacements', () => { }); it('should replace SOURCE_URL and _HOSTNAME', () => { - sandbox.stub(trackPromise, 'trackImpressionPromise', Promise.resolve()); + sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { + return Promise.resolve(); + }); return expandAsync('?url=SOURCE_URL&host=SOURCE_HOSTNAME').then(res => { expect(res).to.not.match(/SOURCE_URL/); expect(res).to.not.match(/SOURCE_HOSTNAME/); @@ -217,15 +221,12 @@ describe('UrlReplacements', () => { it('should update SOURCE_URL after track impression', () => { const win = getFakeWindow(); win.location = parseUrl('https://wrong.com'); - const clock = sandbox.useFakeTimers(); - sandbox.stub(trackPromise, 'trackImpressionPromise', - new Promise(resolve => { - setTimeout(() => { - win.location = parseUrl('https://example.com#gclid=123456'); - resolve(); - }, 1000); - })); - clock.tick(1001); + sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { + return new Promise(resolve => { + win.location = parseUrl('https://example.com#gclid=123456'); + resolve(); + }); + }); return installUrlReplacementsServiceForDoc(win.ampdoc) .expandAsync('?url=SOURCE_URL') .then(res => { @@ -750,19 +751,18 @@ describe('UrlReplacements', () => { it('should replace QUERY_PARAM with foo', () => { const win = getFakeWindow(); win.location = parseUrl('https://example.com?query_string_param1=wrong'); - const clock = sandbox.useFakeTimers(); - sandbox.stub(trackPromise, 'trackImpressionPromise', - new Promise(resolve => { - setTimeout(() => { - win.location = - parseUrl('https://example.com?query_string_param1=foo'); - resolve(); - }, 1000); - })); - clock.tick(1001); + sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { + return new Promise(resolve => { + win.location = + parseUrl('https://example.com?query_string_param1=foo'); + resolve(); + console.log('promise resolve'); + }); + }); return installUrlReplacementsServiceForDoc(win.ampdoc) .expandAsync('?sh=QUERY_PARAM(query_string_param1)&s') .then(res => { + console.log('compare happend', res); expect(res).to.match(/sh=foo&s/); }); }); @@ -770,6 +770,9 @@ describe('UrlReplacements', () => { it('should replace QUERY_PARAM with ""', () => { const win = getFakeWindow(); win.location = parseUrl('https://example.com'); + sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { + return Promise.resolve(); + }); return installUrlReplacementsServiceForDoc(win.ampdoc) .expandAsync('?sh=QUERY_PARAM(query_string_param1)&s') .then(res => { @@ -780,6 +783,9 @@ describe('UrlReplacements', () => { it('should replace QUERY_PARAM with default_value', () => { const win = getFakeWindow(); win.location = parseUrl('https://example.com'); + sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { + return Promise.resolve(); + }); return installUrlReplacementsServiceForDoc(win.ampdoc) .expandAsync('?sh=QUERY_PARAM(query_string_param1,default_value)&s') .then(res => { @@ -790,6 +796,9 @@ describe('UrlReplacements', () => { it('should collect vars', () => { const win = getFakeWindow(); win.location = parseUrl('https://example.com?p1=foo'); + sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { + return Promise.resolve(); + }); return installUrlReplacementsServiceForDoc(win.ampdoc) .collectVars('?SOURCE_HOST&QUERY_PARAM(p1)&SIMPLE&FUNC&PROMISE', { 'SIMPLE': 21, From 5b9c451a38a1a046fe75c05a014a7363c6791267 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 20 Oct 2016 12:37:15 -0700 Subject: [PATCH 06/13] fix type check --- src/impression.js | 3 +-- src/service/url-replacements-impl.js | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/impression.js b/src/impression.js index 9526a21d6b05..3bd6b997021b 100644 --- a/src/impression.js +++ b/src/impression.js @@ -33,8 +33,7 @@ let trackImpressionPromise = null; * @return {!Promise} */ export function getTrackImpressionPromise() { - dev().assert(trackImpressionPromise); - return trackImpressionPromise; + return dev().assert(trackImpressionPromise); } /** diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 0917c187a510..47d48da9416c 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -547,10 +547,18 @@ export class UrlReplacements { return navigationInfo[attribute]; } + /** + * Return the QUERY_PARAM from the current location href + * @param {*} param + * @param {string} defaultValue + * @param {boolean=} opt_sync + * @return {string} + */ getQueryParamData_(param, defaultValue, opt_sync) { user().assert(param, 'The first argument to QUERY_PARAM, the query string ' + 'param is required'); + user().assert(typeof param == 'string', 'param should be a string'); const url = parseUrl(this.ampdoc.win.location.href); const params = parseQueryString(url.search); return (typeof params[param] !== 'undefined') ? From 6972a4f8847af6198c354e1a163f3672cc41fc64 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Mon, 24 Oct 2016 10:30:36 -0700 Subject: [PATCH 07/13] wip --- src/document-info.js | 4 ++-- src/service/ampdoc-impl.js | 12 ++++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/document-info.js b/src/document-info.js index 14807df712fd..ffa239f9c1e2 100644 --- a/src/document-info.js +++ b/src/document-info.js @@ -41,10 +41,10 @@ export let DocumentInfoDef; * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc * @return {!DocumentInfoDef} Info about the doc */ -export function documentInfoForDoc(nodeOrDoc) { +export function documentInfoForDoc(nodeOrDoc, opt_force) { return /** @type {!DocumentInfoDef} */ (getServiceForDoc(nodeOrDoc, 'documentInfo', ampdoc => { - const url = ampdoc.getUrl(); + const url = ampdoc.getUrl(opt_force); const sourceUrl = getSourceUrl(url); const rootNode = ampdoc.getRootNode(); let canonicalUrl = rootNode && rootNode.AMP diff --git a/src/service/ampdoc-impl.js b/src/service/ampdoc-impl.js index 86b5f445376f..662a4d16a8d8 100644 --- a/src/service/ampdoc-impl.js +++ b/src/service/ampdoc-impl.js @@ -265,9 +265,10 @@ export class AmpDoc { /** * Returns the URL from which the document was loaded. + * @param {boolean=} opt_force * @return {string} */ - getUrl() { + getUrl(opt_force) { return dev().assertString(null, 'not implemented'); } @@ -328,7 +329,7 @@ export class AmpDocSingle extends AmpDoc { } /** @override */ - getUrl() { + getUrl(opt_force) { return this.win.location.href; } @@ -372,7 +373,7 @@ export class AmpDocShadow extends AmpDoc { */ constructor(win, url, shadowRoot) { super(win); - /** @private @const {string} */ + /** @private {string} */ this.url_ = url; /** @private @const {!ShadowRoot} */ this.shadowRoot_ = shadowRoot; @@ -411,7 +412,10 @@ export class AmpDocShadow extends AmpDoc { } /** @override */ - getUrl() { + getUrl(opt_force) { + if (opt_force) { + + } return this.url_; } From e94029dd8c87919ad1a330ecf8c8142fe4292fb0 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Mon, 24 Oct 2016 13:41:06 -0700 Subject: [PATCH 08/13] add getter --- src/document-info.js | 13 ++++++++++--- src/service/ampdoc-impl.js | 12 ++++-------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/document-info.js b/src/document-info.js index ffa239f9c1e2..72eb7c1ce3fb 100644 --- a/src/document-info.js +++ b/src/document-info.js @@ -41,10 +41,10 @@ export let DocumentInfoDef; * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc * @return {!DocumentInfoDef} Info about the doc */ -export function documentInfoForDoc(nodeOrDoc, opt_force) { +export function documentInfoForDoc(nodeOrDoc) { return /** @type {!DocumentInfoDef} */ (getServiceForDoc(nodeOrDoc, 'documentInfo', ampdoc => { - const url = ampdoc.getUrl(opt_force); + const url = ampdoc.getUrl(); const sourceUrl = getSourceUrl(url); const rootNode = ampdoc.getRootNode(); let canonicalUrl = rootNode && rootNode.AMP @@ -56,11 +56,18 @@ export function documentInfoForDoc(nodeOrDoc, opt_force) { canonicalUrl = parseUrl(canonicalTag.href).href; } const pageViewId = getPageViewId(ampdoc.win); - return {url, sourceUrl, canonicalUrl, pageViewId}; + const res = {url, sourceUrl, canonicalUrl, pageViewId}; + Object.defineProperties(res, 'sourceUrl', { + get: () => { + return getSourceUrl(ampdoc.getUrl()); + }, + }); + return res; })); } + /** * Returns a relatively low entropy random string. * This should be called once per window and then cached for subsequent diff --git a/src/service/ampdoc-impl.js b/src/service/ampdoc-impl.js index 662a4d16a8d8..86b5f445376f 100644 --- a/src/service/ampdoc-impl.js +++ b/src/service/ampdoc-impl.js @@ -265,10 +265,9 @@ export class AmpDoc { /** * Returns the URL from which the document was loaded. - * @param {boolean=} opt_force * @return {string} */ - getUrl(opt_force) { + getUrl() { return dev().assertString(null, 'not implemented'); } @@ -329,7 +328,7 @@ export class AmpDocSingle extends AmpDoc { } /** @override */ - getUrl(opt_force) { + getUrl() { return this.win.location.href; } @@ -373,7 +372,7 @@ export class AmpDocShadow extends AmpDoc { */ constructor(win, url, shadowRoot) { super(win); - /** @private {string} */ + /** @private @const {string} */ this.url_ = url; /** @private @const {!ShadowRoot} */ this.shadowRoot_ = shadowRoot; @@ -412,10 +411,7 @@ export class AmpDocShadow extends AmpDoc { } /** @override */ - getUrl(opt_force) { - if (opt_force) { - - } + getUrl() { return this.url_; } From b521f9a5f3f50ebdc93ae015e2fc00b9b231a70c Mon Sep 17 00:00:00 2001 From: zhouyx Date: Mon, 24 Oct 2016 15:48:52 -0700 Subject: [PATCH 09/13] add getter to sourceUrl --- src/document-info.js | 8 +++----- test/functional/test-document-info.js | 22 ++++++++++++++++++++-- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/document-info.js b/src/document-info.js index 72eb7c1ce3fb..e9bf939dbcc3 100644 --- a/src/document-info.js +++ b/src/document-info.js @@ -56,11 +56,9 @@ export function documentInfoForDoc(nodeOrDoc) { canonicalUrl = parseUrl(canonicalTag.href).href; } const pageViewId = getPageViewId(ampdoc.win); - const res = {url, sourceUrl, canonicalUrl, pageViewId}; - Object.defineProperties(res, 'sourceUrl', { - get: () => { - return getSourceUrl(ampdoc.getUrl()); - }, + const res = {sourceUrl, canonicalUrl, pageViewId}; + Object.defineProperty(res, 'sourceUrl', { + get: () => getSourceUrl(ampdoc.getUrl()), }); return res; })); diff --git a/test/functional/test-document-info.js b/test/functional/test-document-info.js index 437544d336db..6cf81e023134 100644 --- a/test/functional/test-document-info.js +++ b/test/functional/test-document-info.js @@ -63,12 +63,30 @@ describe('document-info', () => { }; win.document.defaultView = win; installDocService(win, true); - expect(documentInfoForDoc(win.document).url).to.equal( - 'https://cdn.ampproject.org/v/www.origin.com/foo/?f=0'); expect(documentInfoForDoc(win.document).sourceUrl).to.equal( 'http://www.origin.com/foo/?f=0'); }); + it('should provide the updated sourceUrl', () => { + const win = { + document: { + nodeType: /* document */ 9, + querySelector() { return 'http://www.origin.com/foo/?f=0'; }, + }, + Math: {random() { return 0.123456789; }}, + location: { + href: 'https://cdn.ampproject.org/v/www.origin.com/foo/?f=0', + }, + }; + win.document.defaultView = win; + installDocService(win, true); + expect(documentInfoForDoc(win.document).sourceUrl).to.equal( + 'http://www.origin.com/foo/?f=0'); + win.location.href = 'https://cdn.ampproject.org/v/www.origin.com/foo/?f=1'; + expect(documentInfoForDoc(win.document).sourceUrl).to.equal( + 'http://www.origin.com/foo/?f=1'); + }); + it('should provide the pageViewId', () => { return getWin('https://twitter.com/').then(win => { sandbox.stub(win.Math, 'random', () => 0.123456789); From a88a0157c4485eeeef9d0895a6b22a139ce3d831 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Wed, 26 Oct 2016 11:00:31 -0700 Subject: [PATCH 10/13] address comments, add timeout test --- build-system/server.js | 9 ++++++--- src/impression.js | 28 ++++++++++++++++++++++++---- src/service/url-replacements-impl.js | 5 ++--- test/functional/test-impression.js | 24 ++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 12 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index a791c672bc14..c06eba967ebf 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -407,11 +407,14 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) { app.use('/impression-proxy/', function(req, res) { assertCors(req, res, ['GET']); - // TODO(@zhouyx): How to request real ad server response with cookie set + // Fake response with the following optional fields: + // location: The Url the that server would have sent redirect to w/o ALP + // tracking_url: URL that should be requested to track click + // gclid: The conversion tracking value const body = { - 'location': 'localhost:8000/examples/#gclid=1234', + 'location': 'localhost:8000/examples/#gclid=1234?gclid=RANDOM&r=1&url=SOURCE_URL', + 'tracking_url': 'tracking_url', 'gclid': '1234', - 'tracking': 'tracking_url', }; res.send(body); }); diff --git a/src/impression.js b/src/impression.js index 3bd6b997021b..672841020e8b 100644 --- a/src/impression.js +++ b/src/impression.js @@ -24,8 +24,11 @@ import { parseQueryString, addParamsToUrl, } from './url'; +import {timerFor} from './timer'; import {getMode} from './mode'; +const TIMEOUT_VALUE = 8000; + let trackImpressionPromise = null; /** @@ -36,6 +39,14 @@ export function getTrackImpressionPromise() { return dev().assert(trackImpressionPromise); } +/** + * Function that reset the trackImpressionPromise only for testing + * @visibleForTesting + */ +export function resetTrackImpressionPromiseForTesting() { + trackImpressionPromise = null; +} + /** * Emit a HTTP request to a destination defined on the incoming URL. * Protected by experiment. @@ -76,12 +87,17 @@ export function maybeTrackImpression(win) { } viewer.whenFirstVisible().then(() => { - // TODO(@zhouyx) We need a timeout here? // TODO(@zhouyx) need test with a real response. - invoke(win, dev().assertString(clickUrl)).then(response => { + const promise = invoke(win, dev().assertString(clickUrl)).then(response => { applyResponse(win, viewer, response); - resolveImpression(); }); + + // Timeout invoke promise after 8s and resolve trackImpressionPromise. + timerFor(win).timeoutPromise(TIMEOUT_VALUE, promise, + 'timeout waiting for ad server response').catch(() => { + }).then(() => { + resolveImpression(); + }); }); } @@ -109,18 +125,22 @@ function invoke(win, clickUrl) { */ function applyResponse(win, viewer, response) { const adLocation = response['location']; - const adTracking = response['tracking']; + const adTracking = response['tracking_url']; // If there is a tracking_url, need to track it // Otherwise track the location const trackUrl = adTracking || adLocation; if (trackUrl && !isProxyOrigin(trackUrl)) { + // To request the provided trackUrl for tracking purposes. new Image().src = trackUrl; } // Replace the location href params with new location params we get. if (adLocation) { + if (!win.history.replaceState) { + return; + } const currentHref = win.location.href; const url = parseUrl(adLocation); const params = parseQueryString(url.search); diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 47d48da9416c..64f7f08db9a7 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -561,9 +561,8 @@ export class UrlReplacements { user().assert(typeof param == 'string', 'param should be a string'); const url = parseUrl(this.ampdoc.win.location.href); const params = parseQueryString(url.search); - return (typeof params[param] !== 'undefined') ? - params[param] : - defaultValue; + return (typeof params[param] !== 'undefined') + ? params[param] : defaultValue; } /** diff --git a/test/functional/test-impression.js b/test/functional/test-impression.js index b12e8a061fb4..21d9eb7ea13b 100644 --- a/test/functional/test-impression.js +++ b/test/functional/test-impression.js @@ -17,6 +17,7 @@ import { getTrackImpressionPromise, maybeTrackImpression, + resetTrackImpressionPromiseForTesting, } from '../../src/impression'; import {toggleExperiment} from '../../src/experiments'; import {viewerForDoc} from '../../src/viewer'; @@ -40,6 +41,7 @@ describe('impression', () => { }; sandbox.spy(xhr, 'fetchJson'); sandbox.stub(viewer, 'whenFirstVisible').returns(Promise.resolve()); + resetTrackImpressionPromiseForTesting(); }); afterEach(() => { @@ -47,7 +49,6 @@ describe('impression', () => { sandbox.restore(); }); - it('should do nothing if the experiment is off', () => { viewer.getParam.throws(new Error('Should not be called')); maybeTrackImpression(window); @@ -106,14 +107,33 @@ describe('impression', () => { clock.tick(2001); return promise.then(() => { expect(window.location.href).to.not.contain('gclid=654321'); + // Reset xhr.fetchJson = () => { return Promise.resolve(); }; }); }); + it('should resolve trackImpressionPromise after timeout', () => { + toggleExperiment(window, 'alp', true); + viewer.getParam.withArgs('click').returns('https://www.example.com'); + xhr.fetchJson = () => { + return new Promise(resolve => { + setTimeout(() => { + resolve(); + }, 10000); + }); + }; + const clock = sandbox.useFakeTimers(); + maybeTrackImpression(window); + return Promise.resolve().then(() => { + clock.tick(8001); + return getTrackImpressionPromise().should.be.fulfilled; + }); + }); + it('should do nothing if get empty response', () => { - toggleExperiment(window, 'alo', true); + toggleExperiment(window, 'alp', true); viewer.getParam.withArgs('click').returns('https://www.example.com'); const prevHref = window.location.href; maybeTrackImpression(window); From 63bfd3f1c003f916e3a368087cd629f6df57ef58 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 27 Oct 2016 10:58:12 -0700 Subject: [PATCH 11/13] test fix --- build-system/server.js | 2 +- test/functional/test-impression.js | 9 ++++++--- test/functional/test-url-replacements.js | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index c06eba967ebf..5a70d37681ca 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -412,7 +412,7 @@ app.use('/impression-proxy/', function(req, res) { // tracking_url: URL that should be requested to track click // gclid: The conversion tracking value const body = { - 'location': 'localhost:8000/examples/#gclid=1234?gclid=RANDOM&r=1&url=SOURCE_URL', + 'location': 'localhost:8000/examples/?gclid=1234&foo=bar&example=123', 'tracking_url': 'tracking_url', 'gclid': '1234', }; diff --git a/test/functional/test-impression.js b/test/functional/test-impression.js index 21d9eb7ea13b..5174784a533b 100644 --- a/test/functional/test-impression.js +++ b/test/functional/test-impression.js @@ -151,15 +151,18 @@ describe('impression', () => { xhr.fetchJson = () => { return Promise.resolve({ - 'location': 'test_location?gclid=123456', + 'location': 'test_location?gclid=123456&foo=bar&example=123', }); }; const prevHref = window.location.href; + console.log(prevHref); + window.history.replaceState(null, '', prevHref + '?bar=foo&test=4321'); + console.log(window.location.href); maybeTrackImpression(window); return Promise.resolve().then(() => { return Promise.resolve().then(() => { - expect(window.location.href).to.contain('gclid=123456'); - expect(window.location.href).to.not.contain('test_location'); + expect(window.location.href).to.equal('http://localhost:9876/context.html' + + '?bar=foo&test=4321&gclid=123456&foo=bar&example=123'); xhr.fetchJson = () => { return Promise.resolve(); }; diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index 36f538d0e182..41ab3f61d5fa 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -223,7 +223,7 @@ describe('UrlReplacements', () => { win.location = parseUrl('https://wrong.com'); sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => { return new Promise(resolve => { - win.location = parseUrl('https://example.com#gclid=123456'); + win.location = parseUrl('https://example.com?gclid=123456'); resolve(); }); }); From 38df3d5b60f9cf1b6b1813a53e90b7adfc2a3781 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 27 Oct 2016 15:09:08 -0700 Subject: [PATCH 12/13] small fix --- src/impression.js | 7 ++----- src/service/url-replacements-impl.js | 11 +++++------ 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/impression.js b/src/impression.js index 672841020e8b..a44b11756282 100644 --- a/src/impression.js +++ b/src/impression.js @@ -93,11 +93,8 @@ export function maybeTrackImpression(win) { }); // Timeout invoke promise after 8s and resolve trackImpressionPromise. - timerFor(win).timeoutPromise(TIMEOUT_VALUE, promise, - 'timeout waiting for ad server response').catch(() => { - }).then(() => { - resolveImpression(); - }); + resolveImpression(timerFor(win).timeoutPromise(TIMEOUT_VALUE, promise, + 'timeout waiting for ad server response').catch(() => {})); }); } diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 64f7f08db9a7..7e75d0c486b4 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -165,11 +165,11 @@ export class UrlReplacements { })); this.setAsync_('SOURCE_URL', () => { - return getTrackImpressionPromise().then( - this.getDocInfoValue_.bind(this, info => { + return getTrackImpressionPromise().then(() => { + return this.getDocInfoValue_(info => { return removeFragment(info.sourceUrl); - }) - ); + }); + }); }); // Returns the host of the Source URL for this AMP document. @@ -551,10 +551,9 @@ export class UrlReplacements { * Return the QUERY_PARAM from the current location href * @param {*} param * @param {string} defaultValue - * @param {boolean=} opt_sync * @return {string} */ - getQueryParamData_(param, defaultValue, opt_sync) { + getQueryParamData_(param, defaultValue) { user().assert(param, 'The first argument to QUERY_PARAM, the query string ' + 'param is required'); From 2d2432153dceaea37318f221d5469432a982b4f6 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 27 Oct 2016 16:23:55 -0700 Subject: [PATCH 13/13] use getter method --- src/document-info.js | 13 +++++++------ src/service/url-replacements-impl.js | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/document-info.js b/src/document-info.js index e9bf939dbcc3..3bf8b8b44afb 100644 --- a/src/document-info.js +++ b/src/document-info.js @@ -44,8 +44,6 @@ export let DocumentInfoDef; export function documentInfoForDoc(nodeOrDoc) { return /** @type {!DocumentInfoDef} */ (getServiceForDoc(nodeOrDoc, 'documentInfo', ampdoc => { - const url = ampdoc.getUrl(); - const sourceUrl = getSourceUrl(url); const rootNode = ampdoc.getRootNode(); let canonicalUrl = rootNode && rootNode.AMP && rootNode.AMP.canonicalUrl; @@ -56,10 +54,13 @@ export function documentInfoForDoc(nodeOrDoc) { canonicalUrl = parseUrl(canonicalTag.href).href; } const pageViewId = getPageViewId(ampdoc.win); - const res = {sourceUrl, canonicalUrl, pageViewId}; - Object.defineProperty(res, 'sourceUrl', { - get: () => getSourceUrl(ampdoc.getUrl()), - }); + const res = { + get sourceUrl() { + return getSourceUrl(ampdoc.getUrl()); + }, + canonicalUrl, + pageViewId, + }; return res; })); } diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 7e75d0c486b4..4607a7e76e9b 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -195,7 +195,7 @@ export class UrlReplacements { })); this.set_('QUERY_PARAM', (param, defaultValue = '') => { - return this.getQueryParamData_(param, defaultValue, /* opt_sync */ true); + return this.getQueryParamData_(param, defaultValue); }); this.setAsync_('QUERY_PARAM', (param, defaultValue = '') => {