Apply suggestions from code review

Co-authored-by: Robert Knight <robertknight@gmail.com>

src/annotator/hypothesis-injector.js

@@ -52,7 +52,7 @@ function hasHypothesis(iframe) {
 /**
  * Inject Hypothesis client into a frame.
  *
- * IMPORTANT: This method requires that the iframe is "accessible"
+ * IMPORTANT: This method requires that the iframe is same-origin
  * (frame.contentDocument|contentWindow is not null).
  *
  * This waits for the frame to finish loading before injecting the client.

src/annotator/index.js

@@ -21,6 +21,8 @@ import { Notebook } from './notebook';
 import { Sidebar } from './sidebar';
 import { EventBus } from './util/emitter';
 
+/** @typedef {import('../types/annotator').Destroyable} Destroyable */
+
 // Look up the URL of the sidebar. This element is added to the page by the
 // boot script before the "annotator" bundle loads.
 const sidebarLinkElement = /** @type {HTMLLinkElement} */ (
@@ -46,51 +48,46 @@ function init() {
 
   const hostFrame = annotatorConfig.subFrameIdentifier ? window.parent : window;
 
-  /** @type {Sidebar|undefined} */
-  let sidebar;
-  /** @type {Notebook|undefined} */
-  let notebook;
-  /** @type {PortProvider|undefined} */
-  let portProvider;
+  /** @type {Destroyable[]} */
+  const destroyables = [];
+
   if (hostFrame === window) {
     const sidebarConfig = getConfig('sidebar');
 
     const hypothesisAppsOrigin = new URL(sidebarConfig.sidebarAppUrl).origin;
-    portProvider = new PortProvider(hypothesisAppsOrigin);
+    const portProvider = new PortProvider(hypothesisAppsOrigin);
 
     const eventBus = new EventBus();
-    sidebar = new Sidebar(document.body, eventBus, sidebarConfig);
-    notebook = new Notebook(document.body, eventBus, getConfig('notebook'));
+    const sidebar = new Sidebar(document.body, eventBus, sidebarConfig);
+    const notebook = new Notebook(
+      document.body,
+      eventBus,
+      getConfig('notebook')
+    );
 
     portProvider.on('frameConnected', (source, port) =>
       /** @type {Sidebar} */ (sidebar).onFrameConnected(source, port)
     );
+    destroyables.push(portProvider, sidebar, notebook);
   }
 
-  /** @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);
+    const vitalSourceInjector = new VitalSourceInjector(annotatorConfig);
+    destroyables.push(vitalSourceInjector);
   } else {
-    // Set up automatic and integration-triggered injection of client into
-    // iframes in this frame.
-    hypothesisInjector = new HypothesisInjector(document.body, annotatorConfig);
+    // Set up automatic injection of the client into iframes in this frame.
+    const hypothesisInjector = new HypothesisInjector(
+      document.body,
+      annotatorConfig
+    );
     // Create the guest that handles creating annotations and displaying highlights.
-    guest = new Guest(document.body, annotatorConfig, hostFrame);
+    const guest = new Guest(document.body, annotatorConfig, hostFrame);
+    destroyables.push(hypothesisInjector, guest);
   }
 
   sidebarLinkElement.addEventListener('destroy', () => {
-    portProvider?.destroy();
-    sidebar?.destroy();
-    notebook?.destroy();
-    vitalSourceInjector?.destroy();
-    hypothesisInjector?.destroy();
-    guest?.destroy();
+    destroyables.forEach(instance => instance.destroy());
 
     // Remove all the `<link>`, `<script>` and `<style>` elements added to the
     // page by the boot script.

src/annotator/integrations/index.js

@@ -14,7 +14,7 @@ import {
  * Create the integration that handles document-type specific aspects of
  * guest functionality.
  *
- * @param {Pick<Annotator, 'anchor'|'anchors'>} annotator
+ * @param {Annotator} annotator
  * @return {Integration}
  */
 export function createIntegration(annotator) {

src/annotator/integrations/pdf.js

@@ -61,7 +61,7 @@ export function isPDF() {
  */
 export class PDFIntegration {
   /**
-   * @param {Pick<Annotator, 'anchor'|'anchors'>} annotator
+   * @param {Annotator} 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/vitalsource.js

@@ -39,7 +39,8 @@ export function vitalSourceFrameRole(window_ = window) {
 }
 
 /**
- * Observe the book content iframe and load the client into this frame.
+ * VitalSourceInjector runs in the book container frame and loads the client into
+ * book content frames.
  */
 export class VitalSourceInjector {
   /**

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

@@ -88,7 +88,7 @@ describe('HypothesisInjector integration test', () => {
 
     it('copies client asset locations from host frame', async () => {
       hostJSONConfig = {
-        clientUrl: 'chrome-extension:///o',
+        clientUrl: 'chrome-extension://abc/client/build/boot.js',
         assetRoot: 'chrome-extension://abc/client',
         notebookAppUrl: 'chrome-extension://abc/client/notebook.html',
         sidebarAppUrl: 'chrome-extension://abc/client/sidebar.html',