New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 amp-ad: fixed URL handling on uzou.js #20342
Conversation
@lannka Please check for travis. Maybe occurred timeout error... |
/cc @ampproject/wg-monetization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one comment
@@ -33,6 +32,7 @@ export function uzou(global, data) { | |||
}; | |||
|
|||
const widgetParams = parseJson(data['widgetParams']); | |||
const akamaiHost = widgetParams['akamaiHost'] || 'speee-ad.akamaized.net'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually ask the contributor to wrap url params with #encodeURIComponent()
, but since it is the host, I believe you won't want that. @send to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And simply to make sure the new change is documented your side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only use akamaiHost
param for debugging purpose. And this param is the host part of ad script url. So we don't need wrap this one.
However, uzoInjector.url
is expected encoding based on RFC 3986. So we have fixed it.
LGTM. Merged. |
thanks ! |
* fixed urls * fixed URI encoding * using rfc3986 * fixed lint
We have fixed URL handling on uzou.js for our ad network.
sourceUrl
instead ofcanonicalUrl
akamaiHost
to usedata-widget-params
Could you review our commits?
Regards.