Allow for white listed replacement of URL placements and use it in A4A. by cramforce · Pull Request #5275 · ampproject/amphtml

cramforce · 2016-09-28T01:15:15Z

A more limited (to A4A) exposure of the work in #5053.

Implements ability to provide a white list of replacements for a given operation.
Moves the click handler from a global position to A4A specific code.
Switches to simpler click position implementation (just pageX/Y). This is currently incorrect, but will be correct after A4A moves to friendly iframes from ShadowDOM.

Should help with #5205 and #4078. Related to #4891.

keithwrightbos · 2016-09-28T17:39:29Z

extensions/amp-a4a/0.1/amp-a4a.js

+      // based rendering, in which case the document relative positions are
+      // really ad-relative.
+      'CLICK_X': () => {
+        return e.pageX;


Given that we're not handling ad offset correctly here in the shadow DOM case, I'm concerned that we may inadvertently trip spam filters due to invalid click x/y values being sent. I propose we either wait for PR #5196 before submitting (will need to handle conflicts) or update the X/Y to use the shadow DOM offset as I had done previously and then I will remove as part of PR #5196

I'd prefer to wait

keithwrightbos · 2016-09-28T17:40:32Z

extensions/amp-a4a/0.1/amp-a4a.js

+        hrefToExpand, vars, undefined, /* opt_whitelist */ {
+          // For now we only allow to replace the click location vars
+          // and nothing else.
+          'CLICK_X': true,


There would never be a reason to pass false here, right? You really want a set so you used Object? Just confirming...

Yeah, sigh. I was considering an array, since O(small N) is likely faster than more complicated O(1), but this is slightly more idiomatic.

keithwrightbos · 2016-09-28T17:42:01Z

extensions/amp-a4a/0.1/amp-a4a.js

+      }
+      target.setAttribute('href', newHref);
+    }
+    return newHref;


No need to return href right? Signature does not reflect this

avimehta · 2016-09-29T18:23:24Z

/cc I am interested in how amp-analytics will work in this as well.

cramforce · 2016-09-29T19:59:56Z

@avimehta Currently no change, but we need to do something similar there for rolling out A4A to more ad sources.

A more limited (to A4A) exposure of the work in ampproject#5053. - Implements ability to provide a white list of replacements for a given operation. - Moves the click handler from a global position to A4A specific code. - Switches to simpler click position implementation (just `pageX/Y`). This is currently incorrect, but will be correct after A4A moves to friendly iframes from ShadowDOM. Should help with ampproject#5205 and ampproject#4078. Related to ampproject#4891.

cramforce · 2016-10-07T22:28:54Z

PTAL

Added @zhouyx since she just wrote similar code.

keithwrightbos · 2016-10-07T23:59:35Z

OOTO until Tuesday. If this cannot wait, please ask tdrl@ to review.
Thank you

On Oct 7, 2016 6:29 PM, "Malte Ubl" notifications@github.com wrote:

PTAL

Added @zhouyx https://github.com/zhouyx since she just wrote similar
code.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#5275 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABr0L8_qqA4oLF5ZvqtVdZdilnOtD4JYks5qxse5gaJpZM4KIUYZ
.

zhouyx · 2016-10-08T00:55:48Z

extensions/amp-a4a/0.1/amp-a4a.js

+   * @param {!Window} iframeWin
+   */
+  registerExpandUrlParams_(iframeWin) {
+    iframeWin.document.documentElement.addEventListener('click',


One thing I worry here is registerAlpHandler_ also listen to the same click event. Would it be the case that it redirect before expanding url params?

I added some comments to this respect. We do need the 2 handlers for correctness, because one is a capture phase handler and thus reliably runs first.

zhouyx · 2016-10-08T00:58:36Z

extensions/amp-a4a/0.1/amp-a4a.js

+      },
+    };
+    const newHref = urlReplacementsForDoc(this.getAmpDoc()).expandSync(
+        hrefToExpand, vars, undefined, /* opt_whitelist */ {


should we keep a default whitelist separately?

I don't think we will use this whitelist elsewhere and I like this inline for readability.

dvoytenko · 2016-10-10T17:53:03Z

src/service/url-replacements-impl.js

   * @private
   */
-  expand_(url, opt_bindings, opt_collectVars, opt_sync) {
+  expand_(url, opt_bindings, opt_collectVars, opt_sync, opt_whiteList) {


This is probably ok as a first start, but I think the final API will be drastically different when we need to support the whole table of enabled/disabled/overriden vars. In the meantime, we could even simply call string.replace('CLIENT_X', value) to get us past this point and closer to correct implementation.

Our current plans wouldn't go much further than this.

@dvoytenko I think the overall design is moving towards not having any overridden variables. Variables will be either enabled or disabled for amp-analytics. This design works for that case(with the change to figure out how whitelist can be converted to blacklist.).

What about performance metrics? It seems very likely we'd completely override their meaning for ads. No?

cramforce · 2016-10-10T22:40:54Z

Friendly ping :)

keithwrightbos · 2016-10-11T13:59:50Z

Apologies for the delay. LGTM. I am definitely interested in the long term design decision on how to allow greater expansion. The original i2i included the ability for a hidden iframe to send post message information to be included in the click url. We are hoping to add this functionality in Q1.

zhouyx · 2016-10-11T18:01:28Z

LGTM

…A. (ampproject#5275) A more limited (to A4A) exposure of the work in ampproject#5053. - Implements ability to provide a white list of replacements for a given operation. - Moves the click handler from a global position to A4A specific code. - Switches to simpler click position implementation (just `pageX/Y`).

cramforce added NEEDS REVIEW WG: monetization labels Sep 28, 2016

cramforce assigned keithwrightbos Sep 28, 2016

keithwrightbos reviewed Sep 28, 2016

View reviewed changes

cramforce added 3 commits October 7, 2016 13:01

Update after merge.

71b3662

Remove console.log

19f3a1c

cramforce force-pushed the a4a-url-replace branch from 1159d3e to 19f3a1c Compare October 7, 2016 22:28

cramforce assigned zhouyx Oct 7, 2016

zhouyx reviewed Oct 8, 2016

View reviewed changes

Add more comments.

ef5b69e

dvoytenko reviewed Oct 10, 2016

View reviewed changes

jridgewell mentioned this pull request Oct 11, 2016

Revert "Rollback: anchor href expand on document click (#5197)" #5204

Closed

zhouyx approved these changes Oct 11, 2016

View reviewed changes

cramforce merged commit bc5ac0d into ampproject:master Oct 11, 2016

cramforce deleted the a4a-url-replace branch October 11, 2016 18:10

Conversation

cramforce commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avimehta commented Sep 29, 2016

Uh oh!

cramforce commented Sep 29, 2016

Uh oh!

cramforce commented Oct 7, 2016

Uh oh!

keithwrightbos commented Oct 7, 2016

Uh oh!

zhouyx Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cramforce commented Oct 10, 2016

Uh oh!

keithwrightbos commented Oct 11, 2016

Uh oh!

zhouyx commented Oct 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhouyx Oct 8, 2016 •

edited

Loading