From e75f58e3ed0eada6617af29c667e9af90d62c7ee Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Thu, 10 Dec 2020 15:42:34 -0800 Subject: [PATCH] Use an embed override for timer service (#31551) * Use an embed override for timer service * fixed tests --- extensions/amp-a4a/0.1/amp-a4a.js | 4 +- extensions/amp-a4a/0.1/friendly-frame-util.js | 4 +- src/friendly-iframe-embed.js | 2 +- src/service.js | 40 ++++++++++++++----- src/service/timer-impl.js | 7 +++- src/service/url-replacements-impl.js | 9 ++--- src/services.js | 3 +- test/unit/test-amp-pixel.js | 2 +- test/unit/test-element-service.js | 12 ++++-- test/unit/test-friendly-iframe-embed.js | 13 +++--- test/unit/test-service.js | 19 +++++++++ 11 files changed, 80 insertions(+), 35 deletions(-) diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index 46361344fa027..dd4fca7710fb9 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -1848,9 +1848,7 @@ export class AmpA4A extends AMP.BaseElement { preinstallCallback_(embedWin, ampdoc) { const parentAmpdoc = this.getAmpDoc(); installUrlReplacementsForEmbed( - // TODO(#22733): Cleanup `parentAmpdoc` once ampdoc-fie is launched. - ampdoc || parentAmpdoc, - embedWin, + ampdoc, new A4AVariableSource(parentAmpdoc, embedWin) ); } diff --git a/extensions/amp-a4a/0.1/friendly-frame-util.js b/extensions/amp-a4a/0.1/friendly-frame-util.js index d8d87f99da2fc..a01d66a552b40 100644 --- a/extensions/amp-a4a/0.1/friendly-frame-util.js +++ b/extensions/amp-a4a/0.1/friendly-frame-util.js @@ -79,9 +79,7 @@ export function renderCreativeIntoFriendlyFrame( (embedWin, ampdoc) => { const parentAmpdoc = element.getAmpDoc(); installUrlReplacementsForEmbed( - // TODO(#22733): Cleanup `parentAmpdoc` once ampdoc-fie is launched. - ampdoc || parentAmpdoc, - embedWin, + ampdoc, new A4AVariableSource(parentAmpdoc, embedWin) ); } diff --git a/src/friendly-iframe-embed.js b/src/friendly-iframe-embed.js index 6da716faacad3..44c160368efee 100644 --- a/src/friendly-iframe-embed.js +++ b/src/friendly-iframe-embed.js @@ -795,7 +795,7 @@ export class Installers { * @param {!./service/ampdoc-impl.AmpDoc} ampdoc */ static installStandardServicesInEmbed(ampdoc) { - installAmpdocServicesForEmbed(ampdoc); installTimerInEmbedWindow(ampdoc.win); + installAmpdocServicesForEmbed(ampdoc); } } diff --git a/src/service.js b/src/service.js index ebf588f0ea71c..9cf3f04416e81 100644 --- a/src/service.js +++ b/src/service.js @@ -58,19 +58,11 @@ export class Disposable { /** * Installs a service override on amp-doc level. - * @param {!Window} embedWin + * @param {!./service/ampdoc-impl.AmpDoc} ampdoc * @param {string} id * @param {!Object} service The service. */ -export function installServiceInEmbedScope(embedWin, id, service) { - // TODO(#22733): completely remove this method once ampdoc-fie launches. - const topWin = getTopWindow(embedWin); - devAssert( - embedWin != topWin, - 'Service override can only be installed in embed window: %s', - id - ); - const ampdoc = getAmpdoc(embedWin.document); +export function installServiceInEmbedDoc(ampdoc, id, service) { registerServiceInternal( getAmpdocServiceHolder(ampdoc), ampdoc, @@ -82,6 +74,22 @@ export function installServiceInEmbedScope(embedWin, id, service) { ); } +/** + * Installs a service override in the scope of an embedded window. + * @param {!Window} embedWin + * @param {string} id + * @param {function(new:Object, !Window)} constructor + */ +export function registerServiceBuilderInEmbedWin(embedWin, id, constructor) { + registerServiceInternal( + embedWin, + embedWin, + id, + constructor, + /* override */ true + ); +} + /** * Registers a service given a class to be used as implementation. * @param {!Window} win @@ -146,6 +154,18 @@ export function getService(win, id) { return getServiceInternal(win, id); } +/** + * Returns a service for the given id and window (a per-window singleton). But + * it looks in the immediate window scope, not the top-level window. + * @param {!Window} win + * @param {string} id of the service. + * @template T + * @return {T} + */ +export function getServiceInEmbedWin(win, id) { + return getServiceInternal(win, id); +} + /** * Returns a promise for a service for the given id and window. Also expects an * element that has the actual implementation. The promise resolves when the diff --git a/src/service/timer-impl.js b/src/service/timer-impl.js index 7034d876bc83a..197ee48357851 100644 --- a/src/service/timer-impl.js +++ b/src/service/timer-impl.js @@ -15,7 +15,10 @@ */ import {getMode} from '../mode'; -import {installServiceInEmbedScope, registerServiceBuilder} from '../service'; +import { + registerServiceBuilder, + registerServiceBuilderInEmbedWin, +} from '../service'; import {reportError} from '../error'; import {user} from '../log'; @@ -186,7 +189,7 @@ export function installTimerService(window) { * @param {!Window} embedWin */ export function installTimerInEmbedWindow(embedWin) { - installServiceInEmbedScope(embedWin, TAG, new Timer(embedWin)); + registerServiceBuilderInEmbedWin(embedWin, TAG, Timer); } /** diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 0c97960a9aa49..8068669d31259 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -35,7 +35,7 @@ import { } from '../url'; import {dev, devAssert, user, userAssert} from '../log'; import { - installServiceInEmbedScope, + installServiceInEmbedDoc, registerServiceBuilderForDoc, } from '../service'; @@ -1149,12 +1149,11 @@ export function installUrlReplacementsServiceForDoc(ampdoc) { /** * @param {!./ampdoc-impl.AmpDoc} ampdoc - * @param {!Window} embedWin * @param {!VariableSource} varSource */ -export function installUrlReplacementsForEmbed(ampdoc, embedWin, varSource) { - installServiceInEmbedScope( - embedWin, +export function installUrlReplacementsForEmbed(ampdoc, varSource) { + installServiceInEmbedDoc( + ampdoc, 'url-replace', new UrlReplacements(ampdoc, varSource) ); diff --git a/src/services.js b/src/services.js index 0694c7ace7dd3..505b69968c8eb 100644 --- a/src/services.js +++ b/src/services.js @@ -20,6 +20,7 @@ import { getService, getServiceForDoc, getServiceForDocOrNull, + getServiceInEmbedWin, getServicePromiseForDoc, } from './service'; import { @@ -659,7 +660,7 @@ export class Services { */ static timerFor(window) { // TODO(alabiaga): This will always return the top window's Timer service. - return /** @type {!./service/timer-impl.Timer} */ (getService( + return /** @type {!./service/timer-impl.Timer} */ (getServiceInEmbedWin( window, 'timer' )); diff --git a/test/unit/test-amp-pixel.js b/test/unit/test-amp-pixel.js index a221afa84c01f..7f5a88a506a69 100644 --- a/test/unit/test-amp-pixel.js +++ b/test/unit/test-amp-pixel.js @@ -177,7 +177,7 @@ describes.realWin( .stub(env.ampdoc, 'whenFirstVisible') .callsFake(() => whenFirstVisiblePromise); - installUrlReplacementsForEmbed(env.ampdoc, win, new TestVariableSource()); + installUrlReplacementsForEmbed(env.ampdoc, new TestVariableSource()); pixel = win.document.createElement('amp-pixel'); pixel.setAttribute( diff --git a/test/unit/test-element-service.js b/test/unit/test-element-service.js index 260f72f0fb5db..e35831209563d 100644 --- a/test/unit/test-element-service.js +++ b/test/unit/test-element-service.js @@ -24,7 +24,7 @@ import { isExtensionScriptInNode, } from '../../src/element-service'; import { - installServiceInEmbedScope, + installServiceInEmbedDoc, registerServiceBuilder, registerServiceBuilderForDoc, resetServiceForTesting, @@ -411,6 +411,7 @@ describes.fakeWin('in embed scope', {amp: true}, (env) => { let nodeInTopWin; let frameElement; let service; + let embedAmpDoc; beforeEach(() => { win = env.win; @@ -422,7 +423,10 @@ describes.fakeWin('in embed scope', {amp: true}, (env) => { embedWin.frameElement = frameElement; setParentWindow(embedWin, win); - env.ampdocService.installFieDoc('https://example.org', embedWin); + embedAmpDoc = env.ampdocService.installFieDoc( + 'https://example.org', + embedWin + ); nodeInEmbedWin = { nodeType: Node.ELEMENT_NODE, @@ -439,7 +443,7 @@ describes.fakeWin('in embed scope', {amp: true}, (env) => { }); it('should return existing service', () => { - installServiceInEmbedScope(embedWin, 'foo', service); + installServiceInEmbedDoc(embedAmpDoc, 'foo', service); return getElementServiceIfAvailableForDocInEmbedScope( nodeInEmbedWin, 'foo', @@ -456,7 +460,7 @@ describes.fakeWin('in embed scope', {amp: true}, (env) => { 'foo', 'amp-foo' ); - installServiceInEmbedScope(embedWin, 'foo', service); + installServiceInEmbedDoc(embedAmpDoc, 'foo', service); return promise.then((returned) => { expect(returned).to.equal(service); }); diff --git a/test/unit/test-friendly-iframe-embed.js b/test/unit/test-friendly-iframe-embed.js index d12fa527c0e8d..cabedac8ad29a 100644 --- a/test/unit/test-friendly-iframe-embed.js +++ b/test/unit/test-friendly-iframe-embed.js @@ -29,12 +29,13 @@ import { import {Services} from '../../src/services'; import {Signals} from '../../src/utils/signals'; import {getFriendlyIframeEmbedOptional} from '../../src/iframe-helper'; -import {installExtensionsService} from '../../src/service/extensions-impl'; import { - installServiceInEmbedScope, + getServiceInEmbedWin, registerServiceBuilder, + registerServiceBuilderInEmbedWin, setParentWindow, } from '../../src/service'; +import {installExtensionsService} from '../../src/service/extensions-impl'; import {isAnimationNone} from '../../testing/test-helper'; import {layoutRectLtwh} from '../../src/layout-rect'; import {loadPromise} from '../../src/event-helper'; @@ -368,7 +369,7 @@ describes.realWin('friendly-iframe-embed', {amp: true}, (env) => { expect(customElementsDefineStub).to.be.calledWith('amp-test'); }); - it.skip('should install and dispose services', () => { + it('should install and dispose services', () => { const disposeSpy = env.sandbox.spy(); const embedService = { dispose: disposeSpy, @@ -381,11 +382,13 @@ describes.realWin('friendly-iframe-embed', {amp: true}, (env) => { html: '', }, (embedWin) => { - installServiceInEmbedScope(embedWin, 'c', embedService); + registerServiceBuilderInEmbedWin(embedWin, 'c', function () { + return embedService; + }); } ); return embedPromise.then((embed) => { - expect(embed.win.__AMP_SERVICES['c'].obj).to.equal(embedService); + expect(getServiceInEmbedWin(embed.win, 'c')).to.equal(embedService); expect(disposeSpy).to.not.be.called; embed.destroy(); expect(disposeSpy).to.be.calledOnce; diff --git a/test/unit/test-service.js b/test/unit/test-service.js index 8b6a99d2a4dc3..b01dc25c1dee2 100644 --- a/test/unit/test-service.js +++ b/test/unit/test-service.js @@ -23,6 +23,7 @@ import { getService, getServiceForDoc, getServiceForDocOrNull, + getServiceInEmbedWin, getServicePromise, getServicePromiseForDoc, getServicePromiseOrNull, @@ -30,6 +31,7 @@ import { isDisposable, registerServiceBuilder, registerServiceBuilderForDoc, + registerServiceBuilderInEmbedWin, rejectServicePromiseForDoc, resetServiceForTesting, setParentWindow, @@ -578,6 +580,23 @@ describe('service', () => { .exist; }); + it('should override services on embedded window', () => { + const topService = {}; + const embedService = {}; + registerServiceBuilder(windowApi, 'A', function () { + return topService; + }); + registerServiceBuilderInEmbedWin(childWin, 'A', function () { + return embedService; + }); + + expect(getService(windowApi, 'A')).to.equal(topService); + expect(getService(childWin, 'A')).to.equal(topService); + + expect(getServiceInEmbedWin(windowApi, 'A')).to.equal(topService); + expect(getServiceInEmbedWin(childWin, 'A')).to.equal(embedService); + }); + it('should dispose disposable services', () => { const disposableFactory = function () { return {