Do not create a `Guest` instance in the VS container

In order to not create an unnecessary `Guest` instance in the
VitalSource container we needed to do the following:

* The `VitalSourceContainerIntegration` is stripped down and converted
  to a class, `VitalSourceInjector`, that does not implement the
  Integration interface but is only responsible for injecting the client
  into content frames.

* The host frame checks the VS frame role and either constructs a Guest
  or sets up the `VitalSourceInjector` as appropriate.

* The `HypothesisInjector`-related logic in `Guest` is extracted out of
  that class and set up in the annotator entry point instead.

src/annotator/guest.js

@@ -13,7 +13,6 @@ import {
   setHighlightsFocused,
   setHighlightsVisible,
 } from './highlighter';
-import { HypothesisInjector } from './hypothesis-injector';
 import { createIntegration } from './integrations';
 import * as rangeUtil from './range-util';
 import { SelectionObserver } from './selection-observer';
@@ -176,10 +175,6 @@ export class Guest {
       source: 'guest',
     });
 
-    // Set up automatic and integration-triggered injection of client into
-    // iframes in this frame.
-    this._hypothesisInjector = new HypothesisInjector(this.element, config);
-
     /**
      * Integration that handles document-type specific functionality in the
      * guest.
@@ -282,15 +277,6 @@ export class Guest {
     this._listeners.add(window, 'resize', () => this._repositionAdder());
   }
 
-  /**
-   * Inject the Hypothesis client into a guest frame.
-   *
-   * @param {HTMLIFrameElement} frame
-   */
-  async injectClient(frame) {
-    return this._hypothesisInjector.injectClient(frame);
-  }
-
   /**
    * Retrieve metadata for the current document.
    */
@@ -458,7 +444,6 @@ export class Guest {
     this._hostRPC.destroy();
     this._sidebarRPC.destroy();
 
-    this._hypothesisInjector.destroy();
     this._listeners.removeAll();
 
     this._selectionObserver.disconnect();

src/annotator/hypothesis-injector.js

@@ -26,7 +26,7 @@ export class HypothesisInjector {
     this._config = config;
     this._frameObserver = new FrameObserver(
       element,
-      frame => this.injectClient(frame), // Frame added callback
+      frame => injectClient(frame, config), // Frame added callback
       () => {} // Frame removed callback
     );
   }
@@ -37,51 +37,6 @@ export class HypothesisInjector {
   destroy() {
     this._frameObserver.disconnect();
   }
-
-  /**
-   * Inject Hypothesis client into a frame.
-   *
-   * IMPORTANT: This method requires that the iframe is "accessible"
-   * (frame.contentDocument|contentWindow is not null).
-   *
-   * This waits for the frame to finish loading before injecting the client.
-   * See {@link onDocumentReady}.
-   *
-   * @param {HTMLIFrameElement} frame
-   */
-  async injectClient(frame) {
-    if (hasHypothesis(frame)) {
-      return;
-    }
-
-    await onDocumentReady(frame);
-
-    // Propagate the client resource locations from the current frame.
-    //
-    // These settings are set only in the browser extension and not by the
-    // embedded client (served by h).
-    //
-    // We could potentially do this by allowing these settings to be part of
-    // the "annotator" config (see `annotator/config/index.js`) which gets passed
-    // to the constructor.
-    const { assetRoot, notebookAppUrl, sidebarAppUrl } =
-      parseJsonConfig(document);
-
-    // Generate a random string to use as a frame ID. The format is not important.
-    const subFrameIdentifier = generateHexString(10);
-    const injectedConfig = {
-      ...this._config,
-
-      assetRoot,
-      notebookAppUrl,
-      sidebarAppUrl,
-
-      subFrameIdentifier,
-    };
-
-    const { clientUrl } = this._config;
-    injectHypothesis(frame, clientUrl, injectedConfig);
-  }
 }
 
 /**
@@ -95,24 +50,57 @@ function hasHypothesis(iframe) {
 }
 
 /**
- * Inject the client's boot script into the iframe. The iframe must be from the
- * same origin as the current window.
+ * Inject Hypothesis client into a frame.
  *
- * @param {HTMLIFrameElement} iframe
- * @param {string} scriptSrc
- * @param {Record<string, any>} config
+ * IMPORTANT: This method requires that the iframe is "accessible"
+ * (frame.contentDocument|contentWindow is not null).
+ *
+ * This waits for the frame to finish loading before injecting the client.
+ * See {@link onDocumentReady}.
+ *
+ * @param {HTMLIFrameElement} frame
+ * @param {Record<string, any>} config - Annotator configuration that is
+ *   injected, along with the Hypothesis client, into the child iframes
  */
-function injectHypothesis(iframe, scriptSrc, config) {
+export async function injectClient(frame, config) {
+  if (hasHypothesis(frame)) {
+    return;
+  }
+
+  await onDocumentReady(frame);
+
+  // Propagate the client resource locations from the current frame.
+  //
+  // These settings are set only in the browser extension and not by the
+  // embedded client (served by h).
+  //
+  // We could potentially do this by allowing these settings to be part of
+  // the "annotator" config (see `annotator/config/index.js`) which gets passed
+  // to the constructor.
+  const { assetRoot, notebookAppUrl, sidebarAppUrl } =
+    parseJsonConfig(document);
+
+  const injectedConfig = {
+    ...config,
+
+    assetRoot,
+    notebookAppUrl,
+    sidebarAppUrl,
+
+    // Generate a random string to use as a frame ID. The format is not important.
+    subFrameIdentifier: generateHexString(10),
+  };
+
   const configElement = document.createElement('script');
   configElement.className = 'js-hypothesis-config';
   configElement.type = 'application/json';
-  configElement.innerText = JSON.stringify(config);
+  configElement.innerText = JSON.stringify(injectedConfig);
 
   const bootScript = document.createElement('script');
   bootScript.async = true;
-  bootScript.src = scriptSrc;
+  bootScript.src = config.clientUrl;
 
-  const iframeDocument = /** @type {Document} */ (iframe.contentDocument);
+  const iframeDocument = /** @type {Document} */ (frame.contentDocument);
   iframeDocument.body.appendChild(configElement);
   iframeDocument.body.appendChild(bootScript);
 }

src/annotator/index.js

@@ -12,6 +12,11 @@ registerIcons(annotatorIcons);
 import { PortProvider } from '../shared/messaging';
 import { getConfig } from './config/index';
 import { Guest } from './guest';
+import { HypothesisInjector } from './hypothesis-injector';
+import {
+  VitalSourceInjector,
+  vitalSourceFrameRole,
+} from './integrations/vitalsource';
 import { Notebook } from './notebook';
 import { Sidebar } from './sidebar';
 import { EventBus } from './util/emitter';
@@ -41,8 +46,11 @@ function init() {
 
   const hostFrame = annotatorConfig.subFrameIdentifier ? window.parent : window;
 
+  /** @type {Sidebar|undefined} */
   let sidebar;
+  /** @type {Notebook|undefined} */
   let notebook;
+  /** @type {PortProvider|undefined} */
   let portProvider;
   if (hostFrame === window) {
     const sidebarConfig = getConfig('sidebar');
@@ -55,18 +63,34 @@ function init() {
     notebook = new Notebook(document.body, eventBus, getConfig('notebook'));
 
     portProvider.on('frameConnected', (source, port) =>
-      sidebar.onFrameConnected(source, port)
+      /** @type {Sidebar} */ (sidebar).onFrameConnected(source, port)
     );
   }
 
-  // Create the guest that handles creating annotations and displaying highlights.
-  const guest = new Guest(document.body, annotatorConfig, hostFrame);
+  /** @type {Guest|undefined} */
+  let guest;
+  /** @type {VitalSourceInjector|undefined} */
+  let vitalSourceInjector;
+  /** @type {HypothesisInjector|undefined} */
+  let hypothesisInjector;
+  const vsFrameRole = vitalSourceFrameRole();
+  if (vsFrameRole === 'container') {
+    vitalSourceInjector = new VitalSourceInjector(annotatorConfig);
+  } else {
+    // Set up automatic and integration-triggered injection of client into
+    // iframes in this frame.
+    hypothesisInjector = new HypothesisInjector(document.body, annotatorConfig);
+    // Create the guest that handles creating annotations and displaying highlights.
+    guest = new Guest(document.body, annotatorConfig, hostFrame);
+  }
 
   sidebarLinkElement.addEventListener('destroy', () => {
     portProvider?.destroy();
     sidebar?.destroy();
     notebook?.destroy();
-    guest.destroy();
+    vitalSourceInjector?.destroy();
+    hypothesisInjector?.destroy();
+    guest?.destroy();
 
     // Remove all the `<link>`, `<script>` and `<style>` elements added to the
     // page by the boot script.

src/annotator/integrations/index.js

@@ -2,7 +2,6 @@ import { HTMLIntegration } from './html';
 import { PDFIntegration, isPDF } from './pdf';
 import {
   VitalSourceContentIntegration,
-  VitalSourceContainerIntegration,
   vitalSourceFrameRole,
 } from './vitalsource';
 
@@ -15,7 +14,7 @@ import {
  * Create the integration that handles document-type specific aspects of
  * guest functionality.
  *
- * @param {Annotator} annotator
+ * @param {Pick<Annotator, 'anchor'|'anchors'>} annotator
  * @return {Integration}
  */
 export function createIntegration(annotator) {
@@ -24,9 +23,7 @@ export function createIntegration(annotator) {
   }
 
   const vsFrameRole = vitalSourceFrameRole();
-  if (vsFrameRole === 'container') {
-    return new VitalSourceContainerIntegration(annotator);
-  } else if (vsFrameRole === 'content') {
+  if (vsFrameRole === 'content') {
     return new VitalSourceContentIntegration();
   }
 

src/annotator/integrations/pdf.js

@@ -61,7 +61,7 @@ export function isPDF() {
  */
 export class PDFIntegration {
   /**
-   * @param {Annotator} annotator
+   * @param {Pick<Annotator, 'anchor'|'anchors'>} annotator
    * @param {object} options
    *   @param {number} [options.reanchoringMaxWait] - Max time to wait for
    *     re-anchoring to complete when scrolling to an un-rendered page.

src/annotator/integrations/test/index-test.js

@@ -3,7 +3,6 @@ import { createIntegration, $imports } from '../index';
 describe('createIntegration', () => {
   let FakeHTMLIntegration;
   let FakePDFIntegration;
-  let FakeVitalSourceContainerIntegration;
   let FakeVitalSourceContentIntegration;
   let fakeIsPDF;
   let fakeVitalSourceFrameRole;
@@ -14,14 +13,12 @@ describe('createIntegration', () => {
     fakeIsPDF = sinon.stub().returns(false);
 
     fakeVitalSourceFrameRole = sinon.stub().returns(null);
-    FakeVitalSourceContainerIntegration = sinon.stub();
     FakeVitalSourceContentIntegration = sinon.stub();
 
     $imports.$mock({
       './html': { HTMLIntegration: FakeHTMLIntegration },
       './pdf': { PDFIntegration: FakePDFIntegration, isPDF: fakeIsPDF },
       './vitalsource': {
-        VitalSourceContainerIntegration: FakeVitalSourceContainerIntegration,
         VitalSourceContentIntegration: FakeVitalSourceContentIntegration,
         vitalSourceFrameRole: fakeVitalSourceFrameRole,
       },
@@ -42,16 +39,6 @@ describe('createIntegration', () => {
     assert.instanceOf(integration, FakePDFIntegration);
   });
 
-  it('creates VitalSource container integration in the VS Bookshelf reader', () => {
-    const annotator = {};
-    fakeVitalSourceFrameRole.returns('container');
-
-    const integration = createIntegration(annotator);
-
-    assert.calledWith(FakeVitalSourceContainerIntegration, annotator);
-    assert.instanceOf(integration, FakeVitalSourceContainerIntegration);
-  });
-
   it('creates VitalSource content integration in the VS Bookshelf reader', () => {
     const annotator = {};
     fakeVitalSourceFrameRole.returns('content');

src/annotator/integrations/test/vitalsource-test.js

 import { delay } from '../../../test-util/wait';
 import {
-  VitalSourceContainerIntegration,
+  VitalSourceInjector,
   VitalSourceContentIntegration,
   vitalSourceFrameRole,
   $imports,
@@ -57,6 +57,7 @@ describe('annotator/integrations/vitalsource', () => {
   let fakeViewer;
   let FakeHTMLIntegration;
   let fakeHTMLIntegration;
+  let fakeInjectClient;
 
   beforeEach(() => {
     fakeViewer = new FakeVitalSourceViewer();
@@ -71,8 +72,11 @@ describe('annotator/integrations/vitalsource', () => {
 
     FakeHTMLIntegration = sinon.stub().returns(fakeHTMLIntegration);
 
+    fakeInjectClient = sinon.stub();
+
     $imports.$mock({
       './html': { HTMLIntegration: FakeHTMLIntegration },
+      '../hypothesis-injector': { injectClient: fakeInjectClient },
     });
   });
 
@@ -99,57 +103,57 @@ describe('annotator/integrations/vitalsource', () => {
     });
   });
 
-  describe('VitalSourceContainerIntegration', () => {
-    let fakeGuest;
-    let integration;
+  describe('VitalSourceInjector', () => {
+    let fakeConfig;
+    let injector;
 
     beforeEach(() => {
-      fakeGuest = {
-        injectClient: sinon.stub(),
+      fakeConfig = {
+        clientUrl: 'https://hypothes.is',
       };
-      integration = new VitalSourceContainerIntegration(fakeGuest);
+      injector = new VitalSourceInjector(fakeConfig);
     });
 
     afterEach(() => {
-      integration.destroy();
+      injector.destroy();
     });
 
     it('throws if constructed outside the VitalSource book reader', () => {
       fakeViewer.destroy();
       assert.throws(() => {
-        new VitalSourceContainerIntegration(fakeGuest);
+        new VitalSourceInjector(fakeConfig);
       }, 'Book container element not found');
     });
 
     it('injects client into content frame', () => {
-      assert.calledWith(fakeGuest.injectClient, fakeViewer.contentFrame);
+      assert.calledWith(fakeInjectClient, fakeViewer.contentFrame, fakeConfig);
     });
 
     it('re-injects client when content frame is changed', async () => {
-      fakeGuest.injectClient.resetHistory();
+      fakeInjectClient.resetHistory();
 
       fakeViewer.beginChapterLoad();
       await delay(0);
-      assert.notCalled(fakeGuest.injectClient);
+      assert.notCalled(fakeInjectClient);
 
       fakeViewer.finishChapterLoad();
       await delay(0);
-      assert.calledWith(fakeGuest.injectClient, fakeViewer.contentFrame);
+      assert.calledWith(fakeInjectClient, fakeViewer.contentFrame, fakeConfig);
     });
 
     it("doesn't re-inject if content frame is removed", async () => {
-      fakeGuest.injectClient.resetHistory();
+      fakeInjectClient.resetHistory();
 
       // Remove the content frame. This will trigger a re-injection check, but
       // do nothing as there is no content frame.
       fakeViewer.contentFrame.remove();
       await delay(0);
 
-      assert.notCalled(fakeGuest.injectClient);
+      assert.notCalled(fakeInjectClient);
     });
 
     it("doesn't re-inject if content frame siblings change", async () => {
-      fakeGuest.injectClient.resetHistory();
+      fakeInjectClient.resetHistory();
 
       // Modify the DOM tree. This will trigger a re-injection check, but do
       // nothing as we've already handled the current frame.
@@ -159,23 +163,7 @@ describe('annotator/integrations/vitalsource', () => {
       );
       await delay(0);
 
-      assert.notCalled(fakeGuest.injectClient);
-    });
-
-    it('does not allow annotation in the container frame', async () => {
-      assert.equal(integration.canAnnotate(), false);
-
-      // Briefly check the results of the stub methods.
-      assert.instanceOf(await integration.anchor(), Range);
-      assert.throws(() => integration.describe());
-      assert.equal(integration.fitSideBySide(), false);
-      assert.deepEqual(await integration.getMetadata(), {
-        title: '',
-        link: [],
-      });
-      assert.equal(await integration.uri(), document.location.href);
-      assert.equal(integration.contentContainer(), document.body);
-      await integration.scrollToAnchor({}); // nb. No assert, this does nothing.
+      assert.notCalled(fakeInjectClient);
     });
   });
 

src/annotator/integrations/vitalsource.js

 import { ListenerCollection } from '../../shared/listener-collection';
 import { HTMLIntegration } from './html';
 import { ImageTextLayer } from './image-text-layer';
+import { injectClient } from '../hypothesis-injector';
 
 /**
  * @typedef {import('../../types/annotator').Anchor} Anchor
@@ -38,18 +39,14 @@ export function vitalSourceFrameRole(window_ = window) {
 }
 
 /**
- * Integration for the container frame in VitalSource's Bookshelf ebook reader.
- *
- * This frame cannot be annotated directly. This integration serves only to
- * load the client into the frame that contains the book content.
- *
- * @implements {Integration}
+ * Observe the book content iframe and load the client into this frame.
  */
-export class VitalSourceContainerIntegration {
+export class VitalSourceInjector {
   /**
-   * @param {Annotator} annotator
+   * @param {Record<string, any>} config - Annotator configuration that is
+   *   injected, along with the Hypothesis client, into the book content iframes
    */
-  constructor(annotator) {
+  constructor(config) {
     const bookElement = findBookElement();
     if (!bookElement) {
       throw new Error('Book container element not found');
@@ -72,7 +69,7 @@ export class VitalSourceContainerIntegration {
       // being called again once loading completes.
       const isBookContent = frame.contentDocument?.querySelector('p');
       if (isBookContent) {
-        annotator.injectClient(frame);
+        injectClient(frame, config);
       }
     };
 
@@ -101,35 +98,6 @@ export class VitalSourceContainerIntegration {
   destroy() {
     this._frameObserver.disconnect();
   }
-
-  canAnnotate() {
-    // No part of the container frame can be annotated.
-    return false;
-  }
-
-  // The methods below are all stubs. Creating annotations is not supported
-  // in the container frame.
-  async anchor() {
-    return new Range();
-  }
-
-  /** @return {Selector[]} */
-  describe() {
-    throw new Error('This frame cannot be annotated');
-  }
-  contentContainer() {
-    return document.body;
-  }
-  fitSideBySide() {
-    return false;
-  }
-  async getMetadata() {
-    return { title: '', link: [] };
-  }
-  async uri() {
-    return document.location.href;
-  }
-  async scrollToAnchor() {}
 }
 
 /**

src/annotator/test/guest-test.js

@@ -39,8 +39,6 @@ describe('Guest', () => {
   let fakeBucketBarClient;
   let fakeCreateIntegration;
   let fakeFindClosestOffscreenAnchor;
-  let FakeHypothesisInjector;
-  let fakeHypothesisInjector;
   let fakeIntegration;
   let fakePortFinder;
   let FakePortRPC;
@@ -124,12 +122,6 @@ describe('Guest', () => {
 
     fakeFindClosestOffscreenAnchor = sinon.stub();
 
-    fakeHypothesisInjector = {
-      destroy: sinon.stub(),
-      injectClient: sinon.stub().resolves(),
-    };
-    FakeHypothesisInjector = sinon.stub().returns(fakeHypothesisInjector);
-
     fakeIntegration = {
       anchor: sinon.stub(),
       canAnnotate: sinon.stub().returns(true),
@@ -147,12 +139,6 @@ describe('Guest', () => {
 
     fakeCreateIntegration = sinon.stub().returns(fakeIntegration);
 
-    fakeHypothesisInjector = {
-      destroy: sinon.stub(),
-      injectClient: sinon.stub().resolves(),
-    };
-    FakeHypothesisInjector = sinon.stub().returns(fakeHypothesisInjector);
-
     fakePortFinder = {
       discover: sinon.stub().resolves({}),
       destroy: sinon.stub(),
@@ -178,7 +164,6 @@ describe('Guest', () => {
         BucketBarClient: FakeBucketBarClient,
       },
       './highlighter': highlighter,
-      './hypothesis-injector': { HypothesisInjector: FakeHypothesisInjector },
       './integrations': {
         createIntegration: fakeCreateIntegration,
       },
@@ -1290,12 +1275,6 @@ describe('Guest', () => {
     );
   });
 
-  it('stops injecting client into annotation-enabled iframes', () => {
-    const guest = createGuest();
-    guest.destroy();
-    assert.calledWith(fakeHypothesisInjector.destroy);
-  });
-
   it('discovers and creates a channel for communication with the sidebar', async () => {
     const { port1 } = new MessageChannel();
     fakePortFinder.discover.resolves(port1);
@@ -1361,33 +1340,4 @@ describe('Guest', () => {
       assert.isTrue(guest.sideBySideActive);
     });
   });
-
-  describe('#injectClient', () => {
-    it('injects client into target frame', async () => {
-      const config = {};
-      const guest = createGuest({});
-      const frame = {};
-
-      await guest.injectClient(frame);
-
-      assert.calledWith(FakeHypothesisInjector, guest.element, config);
-      assert.calledWith(fakeHypothesisInjector.injectClient, frame);
-    });
-
-    it('can be called by the integration when created', async () => {
-      const config = {};
-      const frame = {};
-
-      // Simulate the integration injecting the client into pre-existing content
-      // frames when created. The VitalSource integration does this for example.
-      fakeCreateIntegration.callsFake(annotator => {
-        annotator.injectClient(frame);
-        return fakeIntegration;
-      });
-      const guest = createGuest({});
-
-      assert.calledWith(FakeHypothesisInjector, guest.element, config);
-      assert.calledWith(fakeHypothesisInjector.injectClient, frame);
-    });
-  });
 });

src/annotator/test/integration/hypothesis-injector-test.js

 import { delay } from '../../../test-util/wait';
 import { DEBOUNCE_WAIT, onDocumentReady } from '../../frame-observer';
-import { HypothesisInjector, $imports } from '../../hypothesis-injector';
+import {
+  HypothesisInjector,
+  injectClient,
+  $imports,
+} from '../../hypothesis-injector';
 
 describe('HypothesisInjector integration test', () => {
   let container;
@@ -64,20 +68,19 @@ describe('HypothesisInjector integration test', () => {
     container.remove();
   });
 
-  describe('#injectClient', () => {
+  describe('injectClient', () => {
     it('configures client', async () => {
       const frame = document.createElement('iframe');
       container.append(frame);
-      const injector = createHypothesisInjector();
 
-      await injector.injectClient(frame);
+      await injectClient(frame, { clientUrl: 'https://hyp.is' });
 
       const configElement = frame.contentDocument.querySelector(
         '.js-hypothesis-config'
       );
       const config = JSON.parse(configElement.textContent);
 
-      assert.match(config.subFrameIdentifier, /[0-9]+/);
+      assert.match(config.subFrameIdentifier, /[a-f0-9]+/);
       assert.notOk(config.assetRoot);
       assert.notOk(config.notebookAppUrl);
       assert.notOk(config.sidebarAppUrl);
@@ -85,6 +88,7 @@ describe('HypothesisInjector integration test', () => {
 
     it('copies client asset locations from host frame', async () => {
       hostJSONConfig = {
+        clientUrl: 'chrome-extension:///o',
         assetRoot: 'chrome-extension://abc/client',
         notebookAppUrl: 'chrome-extension://abc/client/notebook.html',
         sidebarAppUrl: 'chrome-extension://abc/client/sidebar.html',
@@ -92,9 +96,8 @@ describe('HypothesisInjector integration test', () => {
 
       const frame = document.createElement('iframe');
       container.append(frame);
-      const injector = createHypothesisInjector();
 
-      await injector.injectClient(frame);
+      await injectClient(frame, hostJSONConfig);
 
       const configElement = frame.contentDocument.querySelector(
         '.js-hypothesis-config'

src/types/annotator.js

@@ -82,8 +82,6 @@
  * @typedef Annotator
  * @prop {Anchor[]} anchors
  * @prop {(ann: AnnotationData) => Promise<Anchor[]>} anchor
- * @prop {(frame: HTMLIFrameElement) => void} injectClient - Inject the Hypothesis client
- *   into a same-origin iframe in guest mode.
  */
 
 /**