Use an embed override for timer service (ampproject#31551)

* Use an embed override for timer service

* fixed tests

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)
     );
   }

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)
       );
     }

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);
   }
 }

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

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);
 }
 
 /**

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)
   );

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'
     ));

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(

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);
     });

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: '<amp-test></amp-test>',
       },
       (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;

test/unit/test-service.js

@@ -23,13 +23,15 @@ import {
   getService,
   getServiceForDoc,
   getServiceForDocOrNull,
+  getServiceInEmbedWin,
   getServicePromise,
   getServicePromiseForDoc,
   getServicePromiseOrNull,
   getServicePromiseOrNullForDoc,
   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 {