Skip to content

Commit

Permalink
(WIP) Disallow passing ampdoc to urlReplacementsForDoc.
Browse files Browse the repository at this point in the history
  • Loading branch information
William Chou committed Nov 30, 2018
1 parent 731feb1 commit a4f42ec
Show file tree
Hide file tree
Showing 20 changed files with 62 additions and 59 deletions.
8 changes: 6 additions & 2 deletions extensions/amp-a4a/0.1/a4a-variable-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,13 @@ export class A4AVariableSource extends VariableSource {
*/
constructor(ampdoc, embedWin) {
super(ampdoc);

// Use the embed parent's URL replacements service.
const {documentElement} = ampdoc.win.document;
const urlReplacements = Services.urlReplacementsForDoc(documentElement);

/** @private {VariableSource} global variable source for fallback. */
this.globalVariableSource_ = Services.urlReplacementsForDoc(ampdoc)
.getVariableSource();
this.globalVariableSource_ = urlReplacements.getVariableSource();

/** @private {!Window} */
this.win_ = embedWin;
Expand Down
6 changes: 3 additions & 3 deletions extensions/amp-a4a/0.1/real-time-config-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,8 @@ export class RealTimeConfigManager {
ERROR_TYPE: errorType,
HREF: this.win_.location.href,
};
const url = Services.urlReplacementsForDoc(this.ampDoc_).expandUrlSync(
errorReportingUrl, macros, whitelist);
const service = Services.urlReplacementsForDoc(this.a4aElement_.element);
const url = service.expandUrlSync(errorReportingUrl, macros, whitelist);
new this.win_.Image().src = url;
}

Expand Down Expand Up @@ -373,7 +373,7 @@ export class RealTimeConfigManager {
const urlReplacementStartTime = Date.now();
this.promiseArray_.push(Services.timerFor(this.win_).timeoutPromise(
timeoutMillis,
Services.urlReplacementsForDoc(this.ampDoc_).expandUrlAsync(
Services.urlReplacementsForDoc(this.a4aElement_.element).expandUrlAsync(
url, macros, whitelist)).then(url => {
checkStillCurrent();
timeoutMillis -= (urlReplacementStartTime - Date.now());
Expand Down
7 changes: 3 additions & 4 deletions extensions/amp-a4a/0.1/test/test-real-time-config-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ describes.realWin('real-time-config-manager', {amp: true}, env => {
});

describe('sendErrorMessage', () => {
let imageStub, requestUrl, ampDoc;
let imageStub, requestUrl;
let errorType, errorReportingUrl;
let imageMock;

Expand All @@ -776,7 +776,6 @@ describes.realWin('real-time-config-manager', {amp: true}, env => {
sandbox.stub(Xhr.prototype, 'fetch');
imageMock = {};
imageStub = sandbox.stub(env.win, 'Image').returns(imageMock);
ampDoc = a4aElement.getAmpDoc();

errorType = RTC_ERROR_ENUM.TIMEOUT;
errorReportingUrl = 'https://www.example.com?e=ERROR_TYPE&h=HREF';
Expand All @@ -785,12 +784,12 @@ describes.realWin('real-time-config-manager', {amp: true}, env => {
ERROR_TYPE: errorType,
HREF: env.win.location.href,
};
requestUrl = Services.urlReplacementsForDoc(ampDoc).expandUrlSync(
requestUrl = Services.urlReplacementsForDoc(a4aElement).expandUrlSync(
errorReportingUrl, macros, whitelist);
});

it('should send error message pingback to correct url', () => {
sendErrorMessage(errorType, errorReportingUrl, env.win, ampDoc);
sendErrorMessage(errorType, errorReportingUrl);
expect(imageStub).to.be.calledOnce;
expect(imageMock.src).to.equal(requestUrl);
});
Expand Down
8 changes: 1 addition & 7 deletions extensions/amp-access/0.1/amp-access-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,8 @@ export class AccessSource {
/** @const {!AccessTypeAdapterDef} */
this.adapter_ = this.createAdapter_(configJson);

/** @const @private {!string} */
this.pubOrigin_ = getSourceOrigin(ampdoc.win.location);

/** @const @private {!../../../src/service/url-replacements-impl.UrlReplacements} */
this.urlReplacements_ = Services.urlReplacementsForDoc(ampdoc);

/** @private @const {!../../../src/service/viewer-impl.Viewer} */
this.viewer_ = Services.viewerForDoc(ampdoc);
this.urlReplacements_ = Services.urlReplacementsForDoc(accessElement);

/** @private @const {function(string):Promise<string>} */
this.openLoginDialog_ = openLoginDialog.bind(null, ampdoc);
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-ad-exit/0.1/amp-ad-exit.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export class AmpAdExit extends AMP.BaseElement {
'CLICK_X': () => event.clientX,
'CLICK_Y': () => event.clientY,
};
const replacements = Services.urlReplacementsForDoc(this.getAmpDoc());
const replacements = Services.urlReplacementsForDoc(this.element);
const whitelist = {
'RANDOM': true,
'CLICK_X': true,
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-analytics/0.1/amp-analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ export class AmpAnalytics extends AMP.BaseElement {
initializeLinker_() {
const type = this.element.getAttribute('type');
this.linkerManager_ = new LinkerManager(this.getAmpDoc(),
this.config_, type);
this.config_, type, this.element);
this.linkerManager_.init();
}

Expand Down Expand Up @@ -570,7 +570,7 @@ export class AmpAnalytics extends AMP.BaseElement {
}
const expansionOptions = this.expansionOptions_(event, trigger);
expandPostMessage(this.getAmpDoc(), msg, this.config_['extraUrlParams'],
trigger, expansionOptions)
trigger, expansionOptions, this.element)
.then(message => {
if (isIframed(this.win)) {
// Only post message with explict `parentPostMessage` to inabox host
Expand Down
24 changes: 15 additions & 9 deletions extensions/amp-analytics/0.1/linker-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,30 +38,34 @@ export class LinkerManager {
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
* @param {!JsonObject} config
* @param {?string} type
* @param {!Element} element
*/
constructor(ampdoc, config, type) {
/** @private {!../../../src/service/ampdoc-impl.AmpDoc} */
constructor(ampdoc, config, type, element) {
/** @const @private {!../../../src/service/ampdoc-impl.AmpDoc} */
this.ampdoc_ = ampdoc;

/** @private {?JsonObject|undefined} */
this.config_ = config['linkers'];

/** @private {!JsonObject} */
/** @const @private {!JsonObject} */
this.vars_ = config['vars'] || {};

/** @private {?string} */
/** @const @private {?string} */
this.type_ = type;

/** @const @private {!Element} */
this.element_ = element;

/** @private {!Array<Promise>} */
this.allLinkerPromises_ = [];

/** @private {!JsonObject} */
/** @const @private {!JsonObject} */
this.resolvedIds_ = dict();

/** @private {!../../../src/service/url-impl.Url} */
/** @const @private {!../../../src/service/url-impl.Url} */
this.urlService_ = Services.urlForDoc(this.ampdoc_);

/** @private {Promise<../../amp-form/0.1/form-submit-service.FormSubmitService>} */
/** @const @private {Promise<../../amp-form/0.1/form-submit-service.FormSubmitService>} */
this.formSubmitService_ = Services.formSubmitPromiseForDoc(ampdoc);

/** @private {?UnlistenDef} */
Expand Down Expand Up @@ -186,8 +190,10 @@ export class LinkerManager {
expandTemplateWithUrlParams_(template, expansionOptions) {
return variableServiceFor(this.ampdoc_.win)
.expandTemplate(template, expansionOptions)
.then(expanded => Services.urlReplacementsForDoc(
this.ampdoc_).expandUrlAsync(expanded));
.then(expanded => {
const urlReplacements = Services.urlReplacementsForDoc(this.element_);
return urlReplacements.expandUrlAsync(expanded);
});
}


Expand Down
23 changes: 13 additions & 10 deletions extensions/amp-analytics/0.1/requests.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ export class RequestHandler {
const params = Object.assign({}, configParams, trigger['extraUrlParams']);
const timestamp = this.win.Date.now();
const batchSegmentPromise = expandExtraUrlParams(
this.ampdoc_, params, expansionOption, bindings, this.whiteList_)
this.variableService_, this.urlReplacementService_, params,
expansionOption, bindings, this.whiteList_)
.then(params => {
return dict({
'trigger': trigger['on'],
Expand Down Expand Up @@ -299,12 +300,14 @@ export class RequestHandler {
* @param {?JsonObject} configParams
* @param {!JsonObject} trigger
* @param {!./variables.ExpansionOptions} expansionOption
* @param {!Element} element
* @return {Promise<string>}
*/
export function expandPostMessage(
ampdoc, msg, configParams, trigger, expansionOption) {
ampdoc, msg, configParams, trigger, expansionOption, element)
{
const variableService = variableServiceFor(ampdoc.win);
const urlReplacementService = Services.urlReplacementsForDoc(ampdoc);
const urlReplacementService = Services.urlReplacementsForDoc(element);

const bindings = variableService.getMacros();
expansionOption.freezeVar('extraUrlParams');
Expand All @@ -321,7 +324,8 @@ export function expandPostMessage(
return basePromise.then(expandedMsg => {
const params = Object.assign({}, configParams, trigger['extraUrlParams']);
//return base url with the appended extra url params;
return expandExtraUrlParams(ampdoc, params, expansionOption, bindings)
return expandExtraUrlParams(variableService, urlReplacementService, params,
expansionOption, bindings)
.then(extraUrlParams => {
return defaultSerializer(expandedMsg, [
dict({'extraUrlParams': extraUrlParams}),
Expand All @@ -332,19 +336,18 @@ export function expandPostMessage(

/**
* Function that handler extraUrlParams from config and trigger.
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
* @param {!./variables.VariableService} variableService
* @param {!../../../src/service/url-replacements-impl.UrlReplacements} urlReplacements
* @param {!Object} params
* @param {!./variables.ExpansionOptions} expansionOption
* @param {!Object} bindings
* @param {!Object=} opt_whitelist
* @return {!Promise<!Object>}
* @private
*/
function expandExtraUrlParams(
ampdoc, params, expansionOption, bindings, opt_whitelist) {
const variableService = variableServiceFor(ampdoc.win);
const urlReplacements = Services.urlReplacementsForDoc(ampdoc);

function expandExtraUrlParams(variableService, urlReplacements, params,
expansionOption, bindings, opt_whitelist)
{
const requestPromises = [];
// Don't encode param values here,
// as we'll do it later in the getExtraUrlParamsString call.
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-brightcove/0.1/amp-brightcove.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class AmpBrightcove extends AMP.BaseElement {
const ampdoc = this.getAmpDoc();
const deferred = new Deferred();

this.urlReplacements_ = Services.urlReplacementsForDoc(ampdoc);
this.urlReplacements_ = Services.urlReplacementsForDoc(this.element);
this.playerReadyPromise_ = deferred.promise;
this.playerReadyResolver_ = deferred.resolve;

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-call-tracking/0.1/amp-call-tracking.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class AmpCallTracking extends AMP.BaseElement {

/** @override */
layoutCallback() {
return Services.urlReplacementsForDoc(this.getAmpDoc())
return Services.urlReplacementsForDoc(this.element)
.expandUrlAsync(user().assertString(this.configUrl_))
.then(url => fetch_(this.win, url))
.then(data => {
Expand Down
1 change: 0 additions & 1 deletion extensions/amp-list/0.1/amp-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,6 @@ export class AmpList extends AMP.BaseElement {
// Construct the fetch init data that would be called by the viewer
// passed in as the 'originalRequest'.
return requestForBatchFetch(
this.getAmpDoc(),
this.element,
this.getPolicy_(),
refresh).then(r => {
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-powr-player/0.1/amp-powr-player.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class AmpPowrPlayer extends AMP.BaseElement {
const ampdoc = this.getAmpDoc();
const deferred = new Deferred();

this.urlReplacements_ = Services.urlReplacementsForDoc(ampdoc);
this.urlReplacements_ = Services.urlReplacementsForDoc(this.element);
this.playerReadyPromise_ = deferred.promise;
this.playerReadyResolver_ = deferred.resolve;
}
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-social-share/0.1/amp-social-share.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class AmpSocialShare extends AMP.BaseElement {
getDataParamsFromAttributes(element));

const hrefWithVars = addParamsToUrl(this.shareEndpoint_, this.params_);
const urlReplacements = Services.urlReplacementsForDoc(this.getAmpDoc());
const urlReplacements = Services.urlReplacementsForDoc(this.element);
const bindingVars = typeConfig['bindings'];
const bindings = {};
if (bindingVars) {
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-story/0.1/amp-story-request-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export class AmpStoryRequestService {
const opts = {};
opts.requireAmpResponseSourceOrigin = false;

return Services.urlReplacementsForDoc(getAmpdoc(this.storyElement_))
return Services.urlReplacementsForDoc(this.storyElement_)
.expandUrlAsync(user().assertString(rawUrl))
.then(url => this.xhr_.fetchJson(url, opts))
.then(response => {
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-story/1.0/amp-story-request-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export class AmpStoryRequestService {
return Promise.resolve(null);
}

return Services.urlReplacementsForDoc(getAmpdoc(this.storyElement_))
return Services.urlReplacementsForDoc(this.storyElement_)
.expandUrlAsync(user().assertString(rawUrl))
.then(url => this.xhr_.fetchJson(url, opts))
.then(response => {
Expand Down
6 changes: 4 additions & 2 deletions extensions/amp-subscriptions/0.1/url-builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,16 @@ import {getValueForExpr} from '../../../src/json';


export class UrlBuilder {

/**
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
* @param {!Promise<string>} readerIdPromise
*/
constructor(ampdoc, readerIdPromise) {
const {documentElement} = ampdoc.win.document;

/** @private @const {!../../../src/service/url-replacements-impl.UrlReplacements} */
this.urlReplacements_ = Services.urlReplacementsForDoc(ampdoc);
this.urlReplacements_ = Services.urlReplacementsForDoc(documentElement);

/** @private @const {!Promise<string>} */
this.readerIdPromise_ = readerIdPromise;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export class AmpUserNotification extends AMP.BaseElement {
/** @override */
buildCallback() {
const ampdoc = this.getAmpDoc();
this.urlReplacements_ = Services.urlReplacementsForDoc(ampdoc);
this.urlReplacements_ = Services.urlReplacementsForDoc(this.element);
this.storagePromise_ = Services.storageForDoc(ampdoc);

this.elementId_ = user().assert(this.element.id,
Expand Down
7 changes: 3 additions & 4 deletions src/batched-json.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function batchFetchJsonFor(
{
assertHttpsUrl(element.getAttribute('src'), element);
const xhr = Services.batchedXhrFor(ampdoc.win);
return requestForBatchFetch(ampdoc, element, opt_urlReplacement, opt_refresh)
return requestForBatchFetch(element, opt_urlReplacement, opt_refresh)
.then(data => xhr.fetchJson(data.xhrUrl, data.fetchOpt))
.then(res => res.json())
.then(data => {
Expand All @@ -66,18 +66,17 @@ export function batchFetchJsonFor(
/**
* Handles url replacement and constructs the FetchInitJsonDef required for a
* fetch.
* @param {!./service/ampdoc-impl.AmpDoc} ampdoc
* @param {!Element} element
* @param {!UrlReplacementPolicy} replacement If ALL, replaces all URL
* vars. If OPT_IN, replaces whitelisted URL vars. Otherwise, don't expand.
* @param {boolean} refresh Forces refresh of browser cache.
* @return {!Promise<!FetchRequestDef>}
*/
export function requestForBatchFetch(ampdoc, element, replacement, refresh) {
export function requestForBatchFetch(element, replacement, refresh) {
const url = element.getAttribute('src');

// Replace vars in URL if desired.
const urlReplacements = Services.urlReplacementsForDoc(ampdoc);
const urlReplacements = Services.urlReplacementsForDoc(element);
const promise = (replacement >= UrlReplacementPolicy.OPT_IN)
? urlReplacements.expandUrlAsync(url)
: Promise.resolve(url);
Expand Down
2 changes: 1 addition & 1 deletion src/service/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ function maybeExpandUrlParams(ampdoc, e) {
return e.pageY;
},
};
const newHref = Services.urlReplacementsForDoc(ampdoc).expandUrlSync(
const newHref = Services.urlReplacementsForDoc(target).expandUrlSync(
hrefToExpand, vars, undefined, /* opt_whitelist */ {
// For now we only allow to replace the click location vars
// and nothing else.
Expand Down
9 changes: 3 additions & 6 deletions src/services.js
Original file line number Diff line number Diff line change
Expand Up @@ -479,15 +479,12 @@ export class Services {
}

/**
* Unlike most service getters, passing `Node` is necessary for some FIE-scope
* services since sometimes we only have the FIE Document for context.
* @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc
* @param {!Element} element
* @return {!./service/url-replacements-impl.UrlReplacements}
*/
static urlReplacementsForDoc(nodeOrDoc) {
static urlReplacementsForDoc(element) {
return /** @type {!./service/url-replacements-impl.UrlReplacements} */ (
getExistingServiceForDocInEmbedScope(
nodeOrDoc, 'url-replace', /* opt_fallbackToTopWin */ true));
getExistingServiceForDocInEmbedScope(element, 'url-replace'));
}

/**
Expand Down

0 comments on commit a4f42ec

Please sign in to comment.