Track impression on amp landing page #5606

zhouyx · 2016-10-14T01:56:41Z

ALP related. Not fully tested as I only get one ad server url that I can ping to.
I added some tests to test our logic after receiving response from ad server, the coverage should be enough for init impl I think.
PTAL to see I am on the right track.

zhouyx · 2016-10-14T01:57:16Z

build-system/server.js

+    'Cookie': 'Cookie:id=4903ef7b59596f28||t=1451370221|et=730|cs=002213fd48e4405e4579c95dfa',
+  };
+  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',


need to extract this from real url.

cramforce · 2016-10-14T14:46:22Z

build-system/server.js

+  const fakeHeaders = {
+    'Host': 'adclick.g.doubleclick.net',
+    'Origin': 'https://cdn.ampproject.org',
+    'Cookie': 'Cookie:id=4903ef7b59596f28||t=1451370221|et=730|cs=002213fd48e4405e4579c95dfa',


cramforce · 2016-10-14T14:47:59Z

build-system/server.js

+  request(options, (error, response, body) => {
+    if (!error && response.statusCode == 200) {
+      res.send(body);
+    }


You probably want to do something in the else here.

I'd consider to kill the if, though. Should be fine to always return the status code and always return the body.

cramforce · 2016-10-14T14:49:24Z

src/impression.js

-  const clickUrl = viewer.getParam('click');
+  let clickUrl = viewer.getParam('click');
+
+  // One test clickUrl


Please delete. Or add a comment here with the properly encoded click URL. Can be produced by using any example AMP doc and then appending #click={encodeURIComponent('YOUR_URL')}

cramforce · 2016-10-14T14:51:36Z

src/impression.js

+  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');


Not sure where this came frame. Just request the URL with new Image().src = adTrackingUrl

cramforce · 2016-10-14T14:52:08Z

src/impression.js

+    return;
+  }
+
+  // If adLocation is not an amp page, need to redirect to it.


This is not desired. Just delete the branch.

cramforce · 2016-10-14T14:53:19Z

src/impression.js

+  let gclid = response['gclid'];
+  if (!gclid) {
+    if (adLocation.indexOf('gclid=') > 0) {
+      gclid = adLocation.substr(adLocation.indexOf('gclid=') + 6);


The code will go away, but please never parse a URL like that :) Use the utilities in url.js

cramforce · 2016-10-14T14:55:00Z

src/impression.js

+      gclid = adLocation.substr(adLocation.indexOf('gclid=') + 6);
+    }
+  }
+  if (gclid) {


Instead this replacement
https://github.com/ampproject/amphtml/blob/master/src/service/url-replacements-impl.js#L187
should wait for this code to run and then magically discover that there is a second URL (The location) and also provide params from that.

+1
it can be achieved by pushing the gclid as a service:

getService('gclid', gclid);

cramforce · 2016-10-14T14:56:41Z

src/impression.js

+  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');


Above use the location for the image URL if no tracking_url was provided.

avimehta · 2016-10-14T20:08:44Z

Can you also add some tests to ensure that the variables from url-replacements are getting the gclid value. For example, does source url contain the gclid once this code is checked in? What about ampdoc_url etc.

cramforce · 2016-10-15T00:47:02Z

src/impression.js

+    }
+  }
+  if (gclid) {
+    win.location.hash = '#gclid=' + gclid;


Thinking about it more, I think we should create a virtualHref in document-info, use that for SOURCE_URL and QUERY_PARAM substitutions and override it here to be the original location.href with the query string coming from the location

lannka · 2016-10-17T20:31:44Z

src/impression.js

+  if (getMode().localDev && !getMode().test) {
+    clickUrl = 'http://localhost:8000/impression-proxy?url=' + clickUrl;
+  }
+  return xhrFor(win).fetchJson(clickUrl, {
    credentials: 'include',
    requireAmpResponseSourceOrigin: true,
  });
  // TODO(@cramforce): Do something with the result.


Remove the TODO? Since we are doing it outside this function.

lannka · 2016-10-17T20:33:15Z

src/impression.js

+    // TODO(@zhouyx) We need a timeout here?
+    // TODO(@zhouyx) need test with a real response.
+    invoke(win, clickUrl).then(response => {
+      if (!response) {


it will never be null

lannka · 2016-10-17T20:33:42Z

src/impression.js

+ * Send the url to ad server and wait for its response
+ * @param {!Window} win
+ * @param {string} clickUrl
+ * @return {!Promise}


{@Promise<!JSONType>}

lannka · 2016-10-17T20:39:37Z

src/impression.js

+  const adLocation = response['location'];
+
+  // If there's a tracking_url, need to redirect to that url.
+  const adTrackingUrl = response['tracking_url'];


in the design doc, the string literal is 'tracking'

lannka · 2016-10-17T20:43:34Z

src/impression.js

+  // If provided use it, if not try to find in adLocation from response
+  let gclid = response['gclid'];
+  if (!gclid) {
+    if (adLocation.indexOf('gclid=') > 0) {


we'd better assume gclid is already in a separate field if there is one (that's the protocol). we should not try to parse it from URL.

lannka · 2016-10-17T20:45:44Z

src/impression.js

+      gclid = adLocation.substr(adLocation.indexOf('gclid=') + 6);
+    }
+  }
+  if (gclid) {


+1
it can be achieved by pushing the gclid as a service:

getService('gclid', gclid);

zhouyx · 2016-10-18T16:02:47Z

As discussed with @cramforce offline, I tried another approach, PTAL

cramforce · 2016-10-18T16:45:22Z

src/service/url-replacements-impl.js

@@ -163,6 +164,18 @@ export class UrlReplacements {
      return removeFragment(info.sourceUrl);
    }));

+    this.setAsync_('SOURCE_URL', () => {


Please provide sync alternatives to these that do not wait for the promise

This is added. I didn't change the previous this.set_ https://github.com/zhouyx/amphtml/blob/366a535bf7a4c0c0e33125637c1052593a5bd199/src/service/url-replacements-impl.js#L201
it's still there.

cramforce

This looks great overall. Some comment and please add more tests.

cramforce · 2016-10-18T16:45:55Z

src/service/url-replacements-impl.js

@@ -163,6 +164,18 @@ export class UrlReplacements {
      return removeFragment(info.sourceUrl);
    }));

+    this.setAsync_('SOURCE_URL', () => {
+      let promise = trackImpressionPromise;
+      if (!promise) {


Can we dev().assert that this is defined instead?

cramforce · 2016-10-18T16:47:36Z

src/impression.js

+  // 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) {


Honestly I think we can skip this path entirely. The Location should include a gclid param

ok, only replace use location. So i should be promised if there's a gclid, it is inside location

cramforce · 2016-10-18T16:48:01Z

src/impression.js

+  // If there is a tracking_url, need to track it
+  // Otherwise track the location
+  const trackUrl = adTracking || adLocation;
+  if (trackUrl) {


Check if the url looks like a proxyUrl and don't make the call in that case.

cramforce · 2016-10-18T16:48:14Z

src/impression.js

+          '#gclid=' + gclid);
+      return;
+    }
+    win.history.replaceState(null, '', adLocation);


@dvoytenko Should this use HistoryService?

I see a lot of usage of win.history.replaceState. @dvoytenko if I need to use HistoryService how should I use that?

cramforce · 2016-10-18T16:49:16Z

src/impression.js

+          '#gclid=' + gclid);
+      return;
+    }
+    win.history.replaceState(null, '', adLocation);


This will not work. The location should be
currentHref + queryParamsOfAdLocation

changed logic, PTAL

zhouyx · 2016-10-18T16:51:18Z

build-system/server.js

@@ -404,6 +404,18 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) {
    next();
 });

+
+app.use('/impression-proxy/', function(req, res) {
+  assertCors(req, res, ['GET']);


I thought about this. I don't want to put a fake cookie here to get the ad server response. Is there a way to get the correct cookie here in proxy server ?(I don't think there should be due to security reason). If not I think I will stick with the fake ad response for now.

Don't think you can. You'd need to hardcode one :)

jridgewell · 2016-10-18T19:53:20Z

src/impression.js


+/** A promise that resolve after getting info from ad server


Comment should be on the next line.

jridgewell · 2016-10-18T19:53:36Z

src/impression.js


+/** A promise that resolve after getting info from ad server
+ * {Promise=}


fixed. changed to !Promise

jridgewell · 2016-10-18T20:00:13Z

src/service/url-replacements-impl.js

+        promise = Promise.resolve();
+      }
+      return promise.then(() => {
+        const url = parseUrl(this.ampdoc.win.location.href);


This is exactly the same as the sync version. Let's add a method for this, and call it from both the sync and async code.

I added a getQueryParamData_ function. I think you have your point, it definitely reduce repeated code. but adding an extra method else seems unclear to me actually.

jridgewell · 2016-10-18T20:00:49Z

src/service/url-replacements-impl.js

+      }
+      return promise.then(
+        this.getDocInfoValue_.bind(this, info => {
+          return removeFragment(info.sourceUrl);


Ditto duplication.

Sorry, I don't quite understand. ❓

#5606 (comment)

I see! I don't want to change this though. it's a one line code, no need to add an extra method for it.

jridgewell · 2016-10-18T20:02:59Z

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';


Can we export a function for this? Exporting a modified module value always feels very dirty, unless it's specifically for testing.

changed to getTrackImpressionPromise and add the dev().assert inside this function

zhouyx · 2016-10-18T22:54:58Z

My tests coverage:
for impression.js 1. should change history with a property location back. 2. should do nothing if never receive response back. 3. should do nothing if response is empty.
for url-replacement 1. QUERY_PARAM and SOURCE_URL expandAsync should wait for track impression promise to resolved. 2. For expandSync QUERY_PARAM should not wait for track impression promise, and should always get the current value.
Let me know if you find any other things that I fail to include @avimehta @cramforce

cramforce · 2016-10-18T23:29:12Z

src/service/url-replacements-impl.js

@@ -163,6 +164,14 @@ export class UrlReplacements {
      return removeFragment(info.sourceUrl);
    }));

+    this.setAsync_('SOURCE_URL', () => {
+      return getTrackImpressionPromise().then(
+        this.getDocInfoValue_.bind(this, info => {


I think there is a problem here:

Doc info eagerly reads location.href and never reads it again after.

If that's the case I think I should read getSourceUrl(this.ampdoc.win.location.href) directly.
However I see that what you are suggesting here only happens in AmpDocShadow, but not with AmpDocSingle.
So what would be the case that we are in a shadow-doc mode, and will it happen here?

You need to create a way that

Reads the current value at time of evaluation

Works on shadow mode (even though that is not relevant for your case.

Maybe add an update method to the document-info service?

my questions are:

Not only SOURCE_URL needs the info.sourceUrl, SOURCE_HOST SOURCE_HOSTNAME...all needs that info. When I update the source url, should I make a copy and keep the old value? (cuz SOURCE_HOST SOURCE_HOSTNAME may still needs the old values?)

If we only update source url only for the case SOURCE_URL I don't see why it's necessary to add this update method to document-info. why don't we just call this.ampdoc.win.location.href since no one will ever use that again.

It's interesting that we never to document-info provided url anywhere in url-replaces-impl, we always get from win.location.href directly. Why is that?

I don't have a good answer to the why. It is fine to read this directly inside of UrlReplacements, but you need to handle the shadow case.

SOURCE_URL and QUERY_PARAM should be the only affected, since the change cannot affect other parts of the URL, right?

add a getter on source_url. I also removed url since it's not used anywhere.

cramforce · 2016-10-18T23:30:01Z

src/service/url-replacements-impl.js

+    user().assert(param,
+        'The first argument to QUERY_PARAM, the query string ' +
+        'param is required');
+    const url = parseUrl(this.ampdoc.win.location.href);


This doesn't have the same problem as above, because it reads the .href every time.

cramforce · 2016-10-18T23:30:23Z

src/service/url-replacements-impl.js

+    user().assert(param,
+        'The first argument to QUERY_PARAM, the query string ' +
+        'param is required');
+    const url = parseUrl(this.ampdoc.win.location.href);


@dvoytenko Should this look at the ampdoc?

lannka · 2016-10-18T23:04:01Z

test/functional/test-impression.js

+    const prevHref = window.location.href;
+    maybeTrackImpression(window);
+    return Promise.resolve().then(() => {
+      return Promise.resolve().then(() => {


why is this ?

This is to intentionally create some delay here before I run the test. Basicly to wait until xhr.fetchJson and viewer.whenFirstVisible to resolve.

lannka · 2016-10-18T23:36:06Z

src/impression.js

+    const url = parseUrl(adLocation);
+    const params = parseQueryString(url.search);
+    const newHref = addParamsToUrl(currentHref, params);
+    win.history.replaceState(null, '', newHref);


Imagine the URLs are:
location.href = http://a.com/?a=1&b=2
adLocation = http://a.com/?a=1&b=2

The above logic gives:
newHref = http://a.com/?a=1&b=2&a=1&b=2

That is probably OK I think.

Why don't we just make it the servers responsibility to come up the right URL, and we just blindly replace it?

Not 100% sure what you are suggesting, but we want to minimize server changes.

I meant to make the protocol simple, just completely replace the URL to the location provided in the response if the host matches. That gives server the ability to remove any existing query params if it wants.

Overall, I think client is not the right place to concatenate the URL, because client does not have the full picture as the server does. It will potentially limit future changes at server side.

@lannka would it be the case that location provided in the response and the original href has different query params? If so what should we do?

@lannka persuaded me that blindly replacing URL is the right way to do here on my bus home yesterday 😄 . It will help to keep behaviors same with what we have now w/o ALP. Server most likely will keep all query params and append gclid to the end. If server really want to delete any query params that we have, we should still enable them to do that, instead of keeping it.
@cramforce What's your idea about this?

I think this is fine. I'd avoid the call to replaceState if old and new are the same.

jridgewell · 2016-10-19T18:24:52Z

src/impression.js

+ * @return {!Promise}
+ */
+export function getTrackImpressionPromise() {
+  dev().assert(trackImpressionPromise);


You can return this line.

zhouyx · 2016-10-25T17:49:17Z

PTAL

cramforce

Getting very close. A few more comments.

cramforce · 2016-10-25T23:17:45Z

build-system/server.js

@@ -404,6 +404,18 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) {
    next();
 });

+
+app.use('/impression-proxy/', function(req, res) {
+  assertCors(req, res, ['GET']);


Don't think you can. You'd need to hardcode one :)

cramforce · 2016-10-25T23:17:56Z

build-system/server.js

+  assertCors(req, res, ['GET']);
+  // TODO(@zhouyx): How to request real ad server response with cookie set
+  const body = {
+    'location': 'localhost:8000/examples/#gclid=1234',


Add schema and a few URL params.

added. but not sure this is what you want? Please let me know

cramforce · 2016-10-25T23:19:41Z

src/impression.js


 /**
 * Emit a HTTP request to a destination defined on the incoming URL.
 * Protected by experiment.
 * @param {!Window} win
 */
 export function maybeTrackImpression(win) {
+  let resolveImpression;
+
+  trackImpressionPromise = new Promise(resolve => {


Maybe add a ForTesting function that sets it back to null?

ForTesting function added. But why can't we just export resolveImpression for testing?

cramforce · 2016-10-25T23:20:05Z

src/impression.js

  viewer.whenFirstVisible().then(() => {
-    invoke(win, clickUrl);
+    // TODO(@zhouyx) We need a timeout here?


yes, please. 8s?

i remembered you said no. 😄 😈

cramforce · 2016-10-25T23:21:17Z

src/impression.js

+  const trackUrl = adTracking || adLocation;
+
+  if (trackUrl && !isProxyOrigin(trackUrl)) {
+    new Image().src = trackUrl;


Add comment as to why and what this does.

cramforce · 2016-10-25T23:21:59Z

src/impression.js

+    const url = parseUrl(adLocation);
+    const params = parseQueryString(url.search);
+    const newHref = addParamsToUrl(currentHref, params);
+    win.history.replaceState(null, '', newHref);


I think replaceState is optional for us. Check if it exists and just don't call it if it does not.

hmmm, add the check for if(!win.history.replaceState). But when would it be null?

This is not supported by some browsers that otherwise work with AMP.

I see. we just do nothing at all for this case? I mean we can probably use other way to attach the params to location.hash

cramforce · 2016-10-25T23:23:13Z

src/service/url-replacements-impl.js

+    const url = parseUrl(this.ampdoc.win.location.href);
+    const params = parseQueryString(url.search);
+    return (typeof params[param] !== 'undefined') ?
+      params[param] :


Nit: Indent +2
I also prefer the ? and : on the newline

+! I also like to write it in this format:

const x = isConditionMatched(state) ? calculateA(value) : calculateB(value);

I also prefer the ? and : on the newline

Ick.

changed. @lannka since everything can fit in the new line, i follow the format of

const x = isCondiftionMatched(state) ? A : B

cramforce · 2016-10-26T00:08:21Z

LOL :)

On Tue, Oct 25, 2016 at 4:47 PM, Yuxuan Zhou notifications@github.com
wrote:

@zhouyx commented on this pull request.

In src/impression.js #5606:

viewer.whenFirstVisible().then(() => {

invoke(win, clickUrl);

// TODO(@zhouyx) We need a timeout here?

i remembered you said no. 😄 😈

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5606, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeTyKh6TrNoxj7LEnPVJxBwcqz9GBOks5q3pT7gaJpZM4KWj_w
.

zhouyx · 2016-10-26T21:18:27Z

Addressed comments.

cramforce · 2016-10-27T00:07:41Z

build-system/server.js

+  // 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',


Please add an actual querystring, not a fragment

Those should be concrete values, not stuff like RANDOM, right?

I honestly don't know how to write a real querystring. 😢 Can you give me an example or lead me to a certain doc i can take a look at?

i always thought querystring is part of the location.hash. big mistake : (

cramforce · 2016-10-27T00:09:42Z

test/functional/test-impression.js

+    maybeTrackImpression(window);
+    return Promise.resolve().then(() => {
+      return Promise.resolve().then(() => {
+        expect(window.location.href).to.contain('gclid=123456');


Make this an assertEqual test to make sure the URL looks good.

cramforce · 2016-10-27T00:10:01Z

test/functional/test-impression.js

+        'location': 'test_location?gclid=123456',
+      });
+    };
+    const prevHref = window.location.href;


Can you spice this one up a bit with a few query params.

cramforce · 2016-10-27T00:10:12Z

test/functional/test-impression.js

+
+    xhr.fetchJson = () => {
+      return Promise.resolve({
+        'location': 'test_location?gclid=123456',


Add a few more query params.

cramforce · 2016-10-27T00:10:46Z

test/functional/test-url-replacements.js

+    win.location = parseUrl('https://wrong.com');
+    sandbox.stub(trackPromise, 'getTrackImpressionPromise', () => {
+      return new Promise(resolve => {
+        win.location = parseUrl('https://example.com#gclid=123456');


Why the # part?

my bad, changed to ?

cramforce · 2016-10-27T01:06:32Z

Just something like ?gclid=1234&foo=bar&example=123
and similar in the test suite.

On Wed, Oct 26, 2016 at 5:17 PM, Yuxuan Zhou notifications@github.com
wrote:

@zhouyx commented on this pull request.

In build-system/server.js
#5606:

@@ -404,6 +404,21 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) {
next();
});

+app.use('/impression-proxy/', function(req, res) {

assertCors(req, res, ['GET']);

// 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?gclid=RANDOM&r=1&url=SOURCE_URL',

I honestly don't know how to write a real querystring. Can you give me an
example or lead to a certain doc i can take a look at?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5606, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT9ejq9lWq_9kU1ix0K1BA8_SyTpVks5q3-2LgaJpZM4KWj_w
.

cramforce · 2016-10-27T15:35:42Z

The Google JS style guide is on your side, but it is wrong :)

On Wed, Oct 26, 2016 at 10:38 AM, Justin Ridgewell <notifications@github.com

wrote:

@jridgewell commented on this pull request.

In src/service/url-replacements-impl.js
#5606:

@@ -541,6 +548,25 @@ 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) {

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') ?
 params[param] :
I also prefer the ? and : on the newline

Ick.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5606, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT-NrpUcbg-BI339lwKPLkpdsc50lks5q35AXgaJpZM4KWj_w
.

cramforce

LGTM

cramforce · 2016-10-27T18:26:56Z

src/impression.js

+    const url = parseUrl(adLocation);
+    const params = parseQueryString(url.search);
+    const newHref = addParamsToUrl(currentHref, params);
+    win.history.replaceState(null, '', newHref);


I think this is fine. I'd avoid the call to replaceState if old and new are the same.

jridgewell · 2016-10-27T19:44:21Z

src/document-info.js

@@ -56,11 +56,16 @@ export function documentInfoForDoc(nodeOrDoc) {
          canonicalUrl = parseUrl(canonicalTag.href).href;
        }
        const pageViewId = getPageViewId(ampdoc.win);
-        return {url, sourceUrl, canonicalUrl, pageViewId};
+        const res = {sourceUrl, canonicalUrl, pageViewId};
+        Object.defineProperty(res, 'sourceUrl', {


Is this really necessary?

It's necessary, because of this comment

Let's add a getter method instead (of a getter property).

I think the best solution would be to have our History class update this field when the URL changes. But that can wait for another time.

jridgewell · 2016-10-27T19:46:52Z

src/impression.js

+    // Timeout invoke promise after 8s and resolve trackImpressionPromise.
+    timerFor(win).timeoutPromise(TIMEOUT_VALUE, promise,
+        'timeout waiting for ad server response').catch(() => {
+        }).then(() => {


This can be done with resolveImpression(timerFor()...), which will skip the deferment from the #then block.

eh I don't quite get, resolveImpression() is a simple resolve function that don't receive any param? you mean make the trackImpressionPromise resolve after 8s

trackImpressionPromise = new Promise(resolve => { const id = timerFor(win).delay(resolve, 8000); resolveImpression = () => { timerFor(win).cancel(id); resolve(); } });

I don't see this one is much better than the current approach.

resolveImpression is the resolve (I hate that that's what everyone calls it, it should be fulfill) given to you by the Promise constructor. It takes a single param, and if that param is a Promise itself, it will adopt the param's state (will resolve when the param resolves, with whatever the param resolves to).

jridgewell · 2016-10-27T19:49:22Z

src/impression.js

+    }
+    const currentHref = win.location.href;
+    const url = parseUrl(adLocation);
+    const params = parseQueryString(url.search);


Can't you just parseQueryString(adLocation)?

parseQueryString expects the input to be a query string either ?a=1&b=1 or a=1&b=1

jridgewell · 2016-10-27T19:51:33Z

src/service/url-replacements-impl.js

@@ -163,6 +164,14 @@ export class UrlReplacements {
      return removeFragment(info.sourceUrl);
    }));

+    this.setAsync_('SOURCE_URL', () => {
+      return getTrackImpressionPromise().then(
+        this.getDocInfoValue_.bind(this, info => {


This #bind is totally unnecessary.

jridgewell · 2016-10-27T19:51:59Z

src/service/url-replacements-impl.js

+        '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);


jridgewell · 2016-10-27T19:52:18Z

src/service/url-replacements-impl.js

+   * Return the QUERY_PARAM from the current location href
+   * @param {*} param
+   * @param {string} defaultValue
+   * @param {boolean=} opt_sync


Unused param.

zhouyx · 2016-10-27T22:09:48Z

@lannka PTAL

zhouyx · 2016-10-27T23:25:03Z

@jridgewell ptal

jridgewell · 2016-10-28T00:09:24Z

src/document-info.js

-          get: () => getSourceUrl(ampdoc.getUrl()),
-        });
+        const res = {
+          get sourceUrl() {


This is still a getter property. Remove the get part, can it turns into a getter method. Maybe call it getSourceUrl.

you want user to call data.getSourceUrl(), instead of data.sourceUrl? I think this get makes sure they can access sourceUrl just like before and that's what we want.

cramforce · 2016-10-28T15:04:01Z

I think we need to go with the getter property for backward compat.
Switching to a method would be better in the long term.

On Thu, Oct 27, 2016 at 5:15 PM, Yuxuan Zhou notifications@github.com
wrote:

@zhouyx commented on this pull request.

In src/document-info.js #5606:
@@ -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() {
you want user to call data.getSourceUrl(), instead of data.sourceUrl? I
think this get makes sure they can access sourceUrl just like before and
that's what we want.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5606, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT7pcB4eY18DeYK5I0Iu3X3UVj-P0ks5q4T6jgaJpZM4KWj_w
.

…mp_reddit_extension * 'master' of https://github.com/ampproject/amphtml: (121 commits) `#setOwner` should rewrite `resource.owner_` value if it exist (ampproject#5898) Defer full upgrade until the element is connected (ampproject#5908) Skeleton for an amp-animation component (ampproject#5891) Use upgrade callback to pick the correct carousel class. (ampproject#5899) Add alert role to user-notification by default (ampproject#5896) Fix test-iframe-createIframeWithMessageStub failures on older browsers (ampproject#5895) Make screen readers announce changes in the slides carousel. (ampproject#5892) Separate out implementaton of document-info and remove dependency of … (ampproject#5864) Fixed assertion in key fetch function. (ampproject#5854) Track impression on amp landing page (ampproject#5606) Backward compatible way of stringifying Uint8Array (ampproject#5881) Ensure that a friendly-iframe embed cancels any boilerplate when ready (ampproject#5863) Fix amp-forms broken and flakey tests. (ampproject#5835) Make SW test properly skip when Request == undefined (ampproject#5876) Add Preamble section to AMP Cache Guidelines (ampproject#5873) Adding Adverline to amp-ad (ampproject#5829) Clarify cache guidelines (ampproject#5874) Consider it as non-viewer mode if there is no "origin" in hash param. (ampproject#5867) [amp-youtube] autoplay and test suite to run across all video players that implement the video API (ampproject#5765) Update Forms Docs to reflect availability. (ampproject#5815) ...

* wip * get response back from proxy server * first impl * another approach * fix * fix type check * wip * add getter * add getter to sourceUrl * address comments, add timeout test * test fix * small fix * use getter method

zhouyx added the WG: monetization label Oct 14, 2016

zhouyx assigned cramforce and lannka Oct 14, 2016

zhouyx commented Oct 14, 2016

View reviewed changes

cramforce requested changes Oct 14, 2016

View reviewed changes

cramforce reviewed Oct 15, 2016

View reviewed changes

lannka requested changes Oct 17, 2016

View reviewed changes

zhouyx force-pushed the alp-impression branch from 0dc02e7 to 366a535 Compare October 18, 2016 04:25

cramforce reviewed Oct 18, 2016

View reviewed changes

cramforce requested changes Oct 18, 2016

View reviewed changes

zhouyx commented Oct 18, 2016

View reviewed changes

jridgewell reviewed Oct 18, 2016

View reviewed changes

cramforce requested changes Oct 18, 2016

View reviewed changes

lannka requested changes Oct 18, 2016

View reviewed changes

jridgewell reviewed Oct 19, 2016

View reviewed changes

zhouyx added 9 commits October 24, 2016 13:41

wip

487d029

get response back from proxy server

fb805e4

first impl

9cc6b0b

another approach

dcb8cb2

fix

281e0a2

fix type check

5b9c451

wip

6972a4f

add getter

e94029d

add getter to sourceUrl

b521f9a

zhouyx force-pushed the alp-impression branch from b67900b to b521f9a Compare October 24, 2016 22:49

cramforce requested changes Oct 25, 2016

View reviewed changes

address comments, add timeout test

a88a015

cramforce reviewed Oct 27, 2016

View reviewed changes

test fix

63bfd3f

cramforce approved these changes Oct 27, 2016

View reviewed changes

jridgewell reviewed Oct 27, 2016

View reviewed changes

small fix

38df3d5

jridgewell mentioned this pull request Oct 27, 2016

Separate out implementaton of document-info and remove dependency of … #5864

Merged

use getter method

2d24321

jridgewell reviewed Oct 28, 2016

View reviewed changes

lannka approved these changes Oct 28, 2016

View reviewed changes

zhouyx merged commit aa205dc into ampproject:master Oct 28, 2016

zhouyx deleted the alp-impression branch October 28, 2016 19:44


		/** A promise that resolve after getting info from ad server


		/** A promise that resolve after getting info from ad server
		* {Promise=}

Track impression on amp landing page #5606

Track impression on amp landing page #5606

Conversation

zhouyx commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimehta commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Oct 25, 2016

cramforce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment