Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow passing ampdoc to Services.urlReplacementsForDoc() #19560

Merged
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 parent URL replacements service for fallback.
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
20 changes: 14 additions & 6 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 @@ -45,8 +45,10 @@ describes.realWin('real-time-config-manager', {amp: true}, env => {

beforeEach(() => {
sandbox = env.sandbox;
// ensures window location == AMP cache passes

// Ensures window location == AMP cache passes.
env.win.AMP_MODE.test = true;

const doc = env.win.document;
element = createElementWithAttributes(env.win.document, 'amp-ad', {
'width': '200',
Expand All @@ -58,6 +60,12 @@ describes.realWin('real-time-config-manager', {amp: true}, env => {
fetchJsonStub = sandbox.stub(Xhr.prototype, 'fetchJson');
a4aElement = new AmpA4A(element);

// RealTimeConfigManager uses the UrlReplacements service scoped to the A4A
// (FIE), but for testing stub in the parent service for simplicity.
const urlReplacements = Services.urlReplacementsForDoc(env.ampdoc);
sandbox.stub(Services, 'urlReplacementsForDoc')
.withArgs(a4aElement.element).returns(urlReplacements);

rtc = new RealTimeConfigManager(a4aElement);
maybeExecuteRealTimeConfig_ = rtc.maybeExecuteRealTimeConfig.bind(rtc);
getCalloutParam_ = rtc.getCalloutParam_.bind(rtc);
Expand Down Expand Up @@ -765,7 +773,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 +784,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 +792,13 @@ describes.realWin('real-time-config-manager', {amp: true}, env => {
ERROR_TYPE: errorType,
HREF: env.win.location.href,
};
requestUrl = Services.urlReplacementsForDoc(ampDoc).expandUrlSync(
errorReportingUrl, macros, whitelist);

requestUrl = Services.urlReplacementsForDoc(a4aElement.element)
.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
9 changes: 1 addition & 8 deletions extensions/amp-access/0.1/amp-access-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {Deferred} from '../../../src/utils/promise';
import {Services} from '../../../src/services';
import {
assertHttpsUrl,
getSourceOrigin,
parseQueryString,
} from '../../../src/url';
import {dev, user} from '../../../src/log';
Expand Down Expand Up @@ -106,14 +105,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
34 changes: 26 additions & 8 deletions extensions/amp-access/0.1/test/test-amp-access-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ describes.fakeWin('AccessSource', {
let ampdoc;
let element;

// Undefined initialization params for AccessSource (unused in tests so far).
let readerIdFn, scheduleViewFn, onReauthorizeFn;

beforeEach(() => {
win = env.win;
ampdoc = env.ampdoc;
Expand All @@ -57,7 +60,8 @@ describes.fakeWin('AccessSource', {
});

function expectSourceType(ampdoc, config, type, adapter) {
const source = new AccessSource(ampdoc, config, null);
const source = new AccessSource(ampdoc, config, readerIdFn, scheduleViewFn,
onReauthorizeFn, element);
expect(source.type_).to.equal(type);
expect(source.adapter_).to.be.instanceOf(adapter);
}
Expand All @@ -72,7 +76,8 @@ describes.fakeWin('AccessSource', {
},
};

const service = new AccessSource(ampdoc, config);
const service = new AccessSource(ampdoc, config, readerIdFn, scheduleViewFn,
onReauthorizeFn, element);
expect(service.loginConfig_).to.deep.equal({
'login1': 'https://acme.com/l1',
'login2': 'https://acme.com/l2',
Expand Down Expand Up @@ -130,7 +135,8 @@ describes.fakeWin('AccessSource', {
type: 'vendor',
vendor: 'vendor1',
};
const source = new AccessSource(ampdoc, config);
const source = new AccessSource(ampdoc, config, readerIdFn, scheduleViewFn,
onReauthorizeFn, element);
sandbox.stub(source.adapter_, 'getConfig');
source.getAdapterConfig();
expect(source.adapter_.getConfig.called).to.be.true;
Expand Down Expand Up @@ -182,7 +188,8 @@ describes.fakeWin('AccessSource', {
'login': 'https://acme.com/l',
'authorizationFallbackResponse': {'error': true},
};
const service = new AccessSource(ampdoc, config);
const service = new AccessSource(ampdoc, config, readerIdFn, scheduleViewFn,
onReauthorizeFn, element);
expect(service.authorizationFallbackResponse_).to.deep.equal(
{'error': true});
});
Expand All @@ -192,7 +199,8 @@ describes.fakeWin('AccessSource', {
type: 'vendor',
vendor: 'vendor1',
};
const source = new AccessSource(ampdoc, config);
const source = new AccessSource(ampdoc, config, readerIdFn, scheduleViewFn,
onReauthorizeFn, element);
const sourceMock = sandbox.mock(source);
sourceMock.expects('login_')
.withExactArgs('https://url', '')
Expand All @@ -212,6 +220,9 @@ describes.fakeWin('AccessSource adapter context', {
let source;
let context;

// Undefined initialization params for AccessSource (unused in tests so far).
let scheduleViewFn, onReauthorizeFn;

beforeEach(() => {
win = env.win;
ampdoc = env.ampdoc;
Expand All @@ -233,8 +244,9 @@ describes.fakeWin('AccessSource adapter context', {
configElement.textContent = JSON.stringify(config);
document.body.appendChild(configElement);

source = new AccessSource(ampdoc, config,
() => Promise.resolve('reader1'));
const readerIdFn = () => Promise.resolve('reader1');
source = new AccessSource(ampdoc, config, readerIdFn, scheduleViewFn,
onReauthorizeFn, configElement);
context = source.adapter_.context_;
});

Expand Down Expand Up @@ -299,6 +311,9 @@ describes.fakeWin('AccessSource authorization', {
let adapterMock;
let source;

// Undefined initialization params for AccessSource (unused in tests so far).
let scheduleViewFn, onReauthorizeFn;

beforeEach(() => {
win = env.win;
ampdoc = env.ampdoc;
Expand All @@ -313,7 +328,10 @@ describes.fakeWin('AccessSource authorization', {
'pingback': 'https://acme.com/p?rid=READER_ID',
'login': 'https://acme.com/l?rid=READER_ID',
};
source = new AccessSource(ampdoc, config, () => Promise.resolve('reader1'));
const readerIdFn = () => Promise.resolve('reader1');
source = new AccessSource(ampdoc, config, readerIdFn, scheduleViewFn,
onReauthorizeFn, win.document.documentElement);

const adapter = {
getConfig: () => {
},
Expand Down
10 changes: 5 additions & 5 deletions extensions/amp-access/0.1/test/test-amp-access.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ import {AccessClientAdapter} from '../amp-access-client';
import {AccessService} from '../amp-access';
import {AmpEvents} from '../../../../src/amp-events';
import {Observable} from '../../../../src/observable';
import {Services} from '../../../../src/services';
import {cidServiceForDocForTesting} from
'../../../../src/service/cid-impl';
import {installPerformanceService} from
'../../../../src/service/performance-impl';
import {toggleExperiment} from '../../../../src/experiments';


describes.fakeWin('AccessService', {
amp: true,
location: 'https://pub.com/doc1',
Expand Down Expand Up @@ -1695,8 +1695,8 @@ describes.fakeWin('AccessService multiple sources', {
const authorizationStub =
sandbox.stub(sourceBeer, 'runAuthorization').callsFake(
() => Promise.resolve());
const broadcastStub = sandbox.stub(sourceBeer.viewer_,
'broadcast');
const viewer = Services.viewerForDoc(ampdoc);
const broadcastStub = sandbox.stub(viewer, 'broadcast');
const sourceBeerMock = sandbox.mock(sourceBeer);
sourceBeerMock.expects('openLoginDialog_')
.withExactArgs('https://acme.com/l?rid=reader1')
Expand Down Expand Up @@ -1727,8 +1727,8 @@ describes.fakeWin('AccessService multiple sources', {
const authorizationStub =
sandbox.stub(sourceDonuts, 'runAuthorization').callsFake(
() => Promise.resolve());
const broadcastStub = sandbox.stub(sourceDonuts.viewer_,
'broadcast');
const viewer = Services.viewerForDoc(ampdoc);
const broadcastStub = sandbox.stub(viewer, 'broadcast');
const sourceDonutsMock = sandbox.mock(sourceDonuts);
sourceDonutsMock.expects('openLoginDialog_')
.withExactArgs('https://acme.com/l?rid=reader1')
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
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,12 @@ describes.realWin('amp-ad-network-adsense-impl', {
{
getUpgradeDelayMs: () => 1,
});

// Make sure the ad iframe (FIE) has a local URL replacements service.
const urlReplacements = Services.urlReplacementsForDoc(element);
sandbox.stub(Services, 'urlReplacementsForDoc')
.withArgs(a).returns(urlReplacements);

impl.buildCallback();
impl.size_ = {width: 123, height: 456};
impl.onCreativeRender({customElementExtensions: []});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,12 @@ describes.realWin('amp-ad-network-doubleclick-impl', realWinConfig, env => {
{
getUpgradeDelayMs: () => 1,
});

// Make sure the ad iframe (FIE) has a local URL replacements service.
const urlReplacements = Services.urlReplacementsForDoc(element);
sandbox.stub(Services, 'urlReplacementsForDoc')
.withArgs(a).returns(urlReplacements);

impl.buildCallback();
impl.size_ = {width: 123, height: 456};
impl.onCreativeRender({customElementExtensions: []});
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