Implement outgoing link URL replacements. #5628

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

Only exposes QUERY_PARAM and CLIENT_ID
Requires opt-in per <a> tag.
Only done for destinations going to the page's source or canonical origin.

cramforce · 2016-10-15T00:40:34Z

@zhouyx please take a look
CC @rudygalfi I went ahead and implemented this :)

zhouyx · 2016-10-17T19:19:47Z

One thing I don't understand. We still do lazy initialization. What I see here is that this.setAsync_('CLIENT_ID... here is called no earlier than this.set_('CLIENT_ID.... Would this work as expected?

cramforce · 2016-10-17T19:21:10Z

@zhouyx Could you rephrase your question? I'm not sure I understand.

This is intended to only provide a CLIENT_ID into a link if there was another mechanism that previously requested it.

zhouyx · 2016-10-17T19:34:53Z

I see, I thought we would need to wait and replace the URL in all cases.
So we are not replacing CLIENT_ID if we don't have it at that moment, would it be normal that url replacement will fail because we never wait for anything?

cramforce · 2016-10-17T19:42:34Z

Replacement happens on click, while the other mechanisms (analytics) load on page view, so they will probably have run by the time of the click.

zhouyx · 2016-10-17T19:48:19Z

src/service/url-replacements-impl.js

+      if (!clientIds) {
+        return null;
+      }
+      return clientIds[dev().assertString(scope)];


don't quite understand what's the scope here. Is it always the same as what we set clientIds[scope above?
Would it be possible to getclientIds[scope] = null here?

scope is the param passed of CLIENT_ID(someScope). If the scope is the same, the client ID is the same, which should be what is achieved here. The return value of this statement is undefined if no async request for the client id was made.

zhouyx · 2016-10-17T19:57:59Z

src/service/url-replacements-impl.js

+      return;
+    }
+    if (element[ORIGINAL_HREF_PROPERTY] == null) {
+      element[ORIGINAL_HREF_PROPERTY] = href;


where is this used?

Line 647. I'll add a comment as to what this is for.

- Only exposes `QUERY_PARAM` and `CLIENT_ID` - Requires opt-in per `<a>` tag. - Only done for destinations going to the page's source or canonical origin. Fixes ampproject#4078

zhouyx · 2016-10-17T20:15:57Z

lgtm

jridgewell · 2016-10-17T21:23:49Z

src/service/url-replacements-impl.js

@@ -213,8 +221,22 @@ export class UrlReplacements {
          scope: dev().assertString(scope),
          createCookieIfNotPresent: true,
        }, consent);
+      }).then(cid => {
+        if (!clientIds) {


Why not just initialize it above? It's just one object.

Please let me have my premature optimizations :)

jridgewell · 2016-10-17T21:30:37Z

src/service/url-replacements-impl.js

+      'CLIENT_ID': true,
+      'QUERY_PARAM': true,
+    };
+    const requestedReplacements = {};


We could reuse the same supportedReplacements by settings its values to false.

Yeah, it also isn't actually used like that, i think. I don't care about allocation on click :)

…mp_reddit_extension * 'master' of https://github.com/ampproject/amphtml: (63 commits) Position:relative on body (ampproject#5665) Fix for ampproject#5663 (ampproject#5664) Nokta Ad Server is added as an ad type (ampproject#5550) RFC: Separate the load phase of AMP into multiple chunks. (ampproject#5536) Add OWNERS files for A4A. (ampproject#5651) Update DEVELOPING.md Call original event add/remove via interceptor (ampproject#5650) Fix wording on confusing steps to protect against CSRF. (ampproject#5646) Install runtime CSS for all amp tests (ampproject#5642) Implement outgoing link URL replacements. (ampproject#5628) Wait for window to load before installing ServiceWorker. (ampproject#5638) iOS wrapper viewport implementation (ampproject#5629) Do not use AMP Version as RTV Versions (ampproject#5474) Purch Ad Support for Amp-Ads (ampproject#5464) A11Y fix for sticky ad close button. (ampproject#5640) Propagate ARIA attributes to real-elements (ampproject#5590) Ibillboard integration (ampproject#5392) Add some keywords to the NPM description of the validator. (ampproject#5633) PWA: messaging and broadcast (ampproject#5588) Carousel swipe not working well on Android Firefox (ampproject#5626) ...

- Only exposes `QUERY_PARAM` and `CLIENT_ID` - Requires opt-in per `<a>` tag. - Only done for destinations going to the page's source or canonical origin. Implements ampproject#4078

cramforce assigned zhouyx Oct 15, 2016

zhouyx suggested changes Oct 17, 2016

View reviewed changes

cramforce added 2 commits October 17, 2016 13:08

Implement outgoing link URL replacements.

24e63e2

- Only exposes `QUERY_PARAM` and `CLIENT_ID` - Requires opt-in per `<a>` tag. - Only done for destinations going to the page's source or canonical origin. Fixes ampproject#4078

Add comment

7a674bf

cramforce force-pushed the link-url-replacements branch from eb9bc6f to 7a674bf Compare October 17, 2016 20:09

zhouyx approved these changes Oct 17, 2016

View reviewed changes

jridgewell reviewed Oct 17, 2016

View reviewed changes

erwinmombay mentioned this pull request Oct 17, 2016

Meeting Notes 2016-10-17 #5645

Closed

cramforce merged commit 156c8ee into ampproject:master Oct 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement outgoing link URL replacements. #5628

Implement outgoing link URL replacements. #5628

cramforce commented Oct 15, 2016

cramforce commented Oct 15, 2016

zhouyx commented Oct 17, 2016

cramforce commented Oct 17, 2016

zhouyx commented Oct 17, 2016

cramforce commented Oct 17, 2016

zhouyx Oct 17, 2016

cramforce Oct 17, 2016

zhouyx Oct 17, 2016

cramforce Oct 17, 2016

zhouyx commented Oct 17, 2016

jridgewell Oct 17, 2016

cramforce Oct 17, 2016

jridgewell Oct 17, 2016

cramforce Oct 17, 2016

Implement outgoing link URL replacements. #5628

Implement outgoing link URL replacements. #5628

Conversation

cramforce commented Oct 15, 2016

cramforce commented Oct 15, 2016

zhouyx commented Oct 17, 2016

cramforce commented Oct 17, 2016

zhouyx commented Oct 17, 2016

cramforce commented Oct 17, 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

zhouyx commented Oct 17, 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