Apply suggestions from code review

src/annotator/guest.js

@@ -102,7 +102,8 @@ function resolveAnchor(anchor) {
  * loads Hypothesis (not all frames will be annotation-enabled). In one frame,
  * usually the top-level one, there will also be an instance of the `Sidebar`
  * class that shows the sidebar app and surrounding UI. The `Guest` instance in
- * each frame connects to the sidebar when {@link connectToSidebar} is called.
+ * each frame connects to the sidebar as part of its initialization, when
+ * {@link _connectSidebarEvents} is called.
  *
  * The anchoring implementation defaults to a generic one for HTML documents and
  * can be overridden to handle different document types.
@@ -118,10 +119,14 @@ export default class Guest {
    * @param {EventBus} eventBus -
    *   Enables communication between components sharing the same eventBus
    * @param {Record<string, any>} [config]
+   * @param {Window} [hostFrame] -
+   *   Host frame which this guest is associated with. This is expected to be
+   *   an ancestor of the guest frame. It may be same or cross origin.
    */
-  constructor(element, eventBus, config = {}) {
+  constructor(element, eventBus, config = {}, hostFrame = window) {
     this.element = element;
     this._emitter = eventBus.createEmitter();
+    this._hostFrame = hostFrame;
     this._highlightsVisible = false;
     this._isAdderVisible = false;
 
@@ -161,9 +166,7 @@ export default class Guest {
     // Set the frame identifier if it's available.
     // The "top" guest instance will have this as null since it's in a top frame not a sub frame
     /** @type {string|null} */
-    this._frameIdentifier = config.subFrameIdentifier;
-    this._hostFrame =
-      config.subFrameIdentifier === null ? window : window.parent;
+    this._frameIdentifier = config.subFrameIdentifier || null;
     this._portFinder = new PortFinder({
       hostFrame: this._hostFrame,
       source: 'guest',
@@ -182,10 +185,6 @@ export default class Guest {
     this._annotationSync = new AnnotationSync(eventBus, this._bridge);
     this._connectAnnotationSync();
 
-    // Discover and connect to the sidebar frame. All RPC events must be
-    // registered before creating the channel.
-    this._connectToSidebar();
-
     // Set up automatic and integration-triggered injection of client into
     // iframes in this frame.
     this._hypothesisInjector = new HypothesisInjector(this.element, config);
@@ -316,7 +315,7 @@ export default class Guest {
     });
   }
 
-  _connectSidebarEvents() {
+  async _connectSidebarEvents() {
     // Handlers for events sent when user hovers or clicks on an annotation card
     // in the sidebar.
     this._bridge.on('focusAnnotations', (tags = []) => {
@@ -367,12 +366,9 @@ export default class Guest {
     this._bridge.on('setHighlightsVisible', showHighlights => {
       this.setHighlightsVisible(showHighlights);
     });
-  }
 
-  /**
-   * Attempt to connect to the sidebar frame.
-   */
-  async _connectToSidebar() {
+    // Discover and connect to the sidebar frame. All RPC events must be
+    // registered before creating the channel.
     const sidebarPort = await this._portFinder.discover('sidebar');
     this._bridge.createChannel(sidebarPort);
   }

src/annotator/index.js

-/**
- * @typedef {import('../types/annotator').HypothesisWindow} HypothesisWindow
- */
-
 // Load polyfill for :focus-visible pseudo-class.
 import 'focus-visible';
 
@@ -13,6 +9,7 @@ import { registerIcons } from '@hypothesis/frontend-shared';
 import iconSet from './icons';
 registerIcons(iconSet);
 
+import { PortProvider } from '../shared/port-provider';
 import { getConfig } from './config/index';
 import Guest from './guest';
 import Notebook from './notebook';
@@ -41,23 +38,36 @@ const sidebarLinkElement = /** @type {HTMLLinkElement} */ (
  */
 function init() {
   const annotatorConfig = getConfig('annotator');
-  const isHostFrame = annotatorConfig.subFrameIdentifier === null;
+
+  const hostFrame = annotatorConfig.subFrameIdentifier ? window.parent : window;
 
   // Create the guest that handles creating annotations and displaying highlights.
   const eventBus = new EventBus();
-  const guest = new Guest(document.body, eventBus, annotatorConfig);
+  const guest = new Guest(document.body, eventBus, annotatorConfig, hostFrame);
 
   let sidebar;
   let notebook;
-  if (isHostFrame) {
-    sidebar = new Sidebar(document.body, eventBus, guest, getConfig('sidebar'));
+  let portProvider;
+  if (hostFrame === window) {
+    const sidebarConfig = getConfig('sidebar');
+
+    const hypothesisAppsOrigin = new URL(sidebarConfig.sidebarAppUrl).origin;
+    portProvider = new PortProvider(hypothesisAppsOrigin);
 
-    // Clear `annotations` value from the notebook's config to prevent direct-linked
-    // annotations from filtering the threads.
+    sidebar = new Sidebar(
+      document.body,
+      eventBus,
+      portProvider.hostPortFor('sidebar'),
+      guest,
+      sidebarConfig
+    );
     notebook = new Notebook(document.body, eventBus, getConfig('notebook'));
+
+    portProvider.listen();
   }
 
   sidebarLinkElement.addEventListener('destroy', () => {
+    portProvider?.destroy();
     sidebar?.destroy();
     notebook?.destroy();
     guest.destroy();

src/annotator/sidebar.js

@@ -2,7 +2,6 @@ import Hammer from 'hammerjs';
 
 import { Bridge } from '../shared/bridge';
 import { ListenerCollection } from '../shared/listener-collection';
-import { PortProvider } from '../shared/port-provider';
 
 import { annotationCounts } from './annotation-counts';
 import BucketBar from './bucket-bar';
@@ -57,16 +56,18 @@ export default class Sidebar {
    * @param {HTMLElement} element
    * @param {import('./util/emitter').EventBus} eventBus -
    *   Enables communication between components sharing the same eventBus
+   * @param {MessagePort} hostPort -
+   *   Host port for the host-sidebar communication channel. The sidebar app will
+   *   request its side of the channel when it starts.
    * @param {Guest} guest -
    *   The `Guest` instance for the current frame. It is currently assumed that
    *   it is always possible to annotate in the frame where the sidebar is
    *   displayed.
    * @param {Record<string, any>} [config]
    */
-  constructor(element, eventBus, guest, config = {}) {
+  constructor(element, eventBus, hostPort, guest, config = {}) {
     this._emitter = eventBus.createEmitter();
-    const hypothesisAppsOrigin = new URL(config.sidebarAppUrl).origin;
-    this._portProvider = new PortProvider(hypothesisAppsOrigin);
+    this._hostPort = hostPort;
 
     /**
      * Channel for host-sidebar communication.
@@ -198,8 +199,7 @@ export default class Sidebar {
     });
 
     // Create channel *after* all bridge events are registered with the `on` method.
-    this._sidebarRPC.createChannel(this._portProvider.sidebarPort);
-    this._portProvider.listen();
+    this._sidebarRPC.createChannel(this._hostPort);
 
     // Notify sidebar when a guest is unloaded. This message is routed via
     // the host frame because in Safari guest frames are unable to send messages
@@ -214,7 +214,6 @@ export default class Sidebar {
   }
 
   destroy() {
-    this._portProvider.destroy();
     this.bucketBar?.destroy();
     this._listeners.removeAll();
     this._hammerManager?.destroy();

src/annotator/test/guest-test.js

@@ -33,8 +33,9 @@ describe('Guest', () => {
   let eventBus;
   let guests;
   let highlighter;
-  let rangeUtil;
+  let hostFrame;
   let notifySelectionChanged;
+  let rangeUtil;
 
   let FakeAnnotationSync;
   let fakeAnnotationSync;
@@ -47,7 +48,7 @@ describe('Guest', () => {
   const createGuest = (config = {}) => {
     const element = document.createElement('div');
     eventBus = new EventBus();
-    const guest = new Guest(element, eventBus, config);
+    const guest = new Guest(element, eventBus, config, hostFrame);
     guests.push(guest);
     return guest;
   };
@@ -62,12 +63,15 @@ describe('Guest', () => {
       setHighlightsFocused: sinon.stub(),
       setHighlightsVisible: sinon.stub(),
     };
+    hostFrame = {
+      postMessage: sinon.stub(),
+    };
+    notifySelectionChanged = null;
     rangeUtil = {
       itemsForRange: sinon.stub().returns([]),
       isSelectionBackwards: sinon.stub(),
       selectionFocusRect: sinon.stub(),
     };
-    notifySelectionChanged = null;
 
     FakeAdder.instance = null;
 
@@ -117,8 +121,6 @@ describe('Guest', () => {
       }
     }
 
-    sandbox.stub(window.parent, 'postMessage');
-
     $imports.$mock({
       '../shared/bridge': { Bridge: sinon.stub().returns(fakeBridge) },
       '../shared/port-finder': {
@@ -1136,7 +1138,7 @@ describe('Guest', () => {
       guest.destroy();
 
       assert.calledWith(
-        window.parent.postMessage,
+        hostFrame.postMessage,
         {
           type: 'hypothesisGuestUnloaded',
           frameIdentifier: 'frame-id',
@@ -1152,7 +1154,7 @@ describe('Guest', () => {
     window.dispatchEvent(new Event('unload'));
 
     assert.calledWith(
-      window.parent.postMessage,
+      hostFrame.postMessage,
       {
         type: 'hypothesisGuestUnloaded',
         frameIdentifier: 'frame-id',
@@ -1174,10 +1176,7 @@ describe('Guest', () => {
 
     await delay(0);
 
-    assert.calledWith(
-      fakeBridge.createChannel,
-      sinon.match.instanceOf(MessagePort)
-    );
+    assert.calledWith(fakeBridge.createChannel, port1);
   });
 
   describe('#contentContainer', () => {

src/annotator/test/integration/anchoring-test.js

 // Tests that the expected parts of the page are highlighted when annotations
 // with various combinations of selector are anchored.
 
-import Guest, { $imports } from '../../guest';
+import Guest, { $imports as guestImports } from '../../guest';
 import { EventBus } from '../../util/emitter';
 import testPageHTML from './test-page.html';
 
@@ -54,7 +54,7 @@ describe('anchoring', () => {
       discover: sinon.stub().resolves(new MessageChannel().port1),
       destroy: sinon.stub(),
     };
-    $imports.$mock({
+    guestImports.$mock({
       '../shared/port-finder': {
         PortFinder: sinon.stub().returns(fakePortFinder),
       },
@@ -66,7 +66,7 @@ describe('anchoring', () => {
     guest.destroy();
     container.remove();
     console.warn.restore();
-    $imports.$restore();
+    guestImports.$restore();
   });
 
   [

src/annotator/test/sidebar-test.js

@@ -7,7 +7,6 @@ const EXTERNAL_CONTAINER_SELECTOR = 'test-external-container';
 
 describe('Sidebar', () => {
   const sandbox = sinon.createSandbox();
-  let fakeGuest;
 
   // Containers and Sidebar instances created by current test.
   let containers;
@@ -16,7 +15,7 @@ describe('Sidebar', () => {
   let fakeBridge;
   let FakeBucketBar;
   let fakeBucketBar;
-  let fakePortProvider;
+  let fakeGuest;
   let FakeToolbarController;
   let fakeToolbar;
 
@@ -41,7 +40,7 @@ describe('Sidebar', () => {
   const createSidebar = (config = {}) => {
     config = {
       // Dummy sidebar app.
-      sidebarAppUrl: 'https://hyp.is/base/annotator/test/empty.html',
+      sidebarAppUrl: '/base/annotator/test/empty.html',
       ...config,
     };
     const container = document.createElement('div');
@@ -49,7 +48,8 @@ describe('Sidebar', () => {
     containers.push(container);
 
     const eventBus = new EventBus();
-    const sidebar = new Sidebar(container, eventBus, fakeGuest, config);
+    const { port1 } = new MessageChannel();
+    const sidebar = new Sidebar(container, eventBus, port1, fakeGuest, config);
     sidebars.push(sidebar);
 
     return sidebar;
@@ -95,12 +95,6 @@ describe('Sidebar', () => {
     }
     fakeGuest = new FakeGuest();
 
-    fakePortProvider = {
-      listen: sinon.stub(),
-      sidebarPort: sinon.stub(),
-      destroy: sinon.stub(),
-    };
-
     fakeToolbar = {
       getWidth: sinon.stub().returns(100),
       useMinimalControls: false,
@@ -113,9 +107,6 @@ describe('Sidebar', () => {
 
     $imports.$mock({
       '../shared/bridge': { Bridge: sinon.stub().returns(fakeBridge) },
-      '../shared/port-provider': {
-        PortProvider: sinon.stub().returns(fakePortProvider),
-      },
       './bucket-bar': { default: FakeBucketBar },
       './toolbar': {
         ToolbarController: FakeToolbarController,
@@ -191,11 +182,14 @@ describe('Sidebar', () => {
   }
 
   it('creates sidebar iframe and passes configuration to it', () => {
-    const appURL = 'https://hyp.is/base/annotator/test/empty.html';
+    const appURL = new URL(
+      '/base/annotator/test/empty.html',
+      window.location.href
+    );
     const sidebar = createSidebar({ annotations: '1234' });
     assert.equal(
       getConfigString(sidebar),
-      `${appURL}${configFragment({ annotations: '1234' })}`
+      appURL + configFragment({ annotations: '1234' })
     );
   });
 

src/shared/port-finder.js

 import { ListenerCollection } from './listener-collection';
 import { isMessageEqual } from './port-util';
 
-const MAX_WAIT_FOR_PORT = 1000 * 5;
+const MAX_WAIT_FOR_PORT = 1000 * 10;
 const POLLING_INTERVAL_FOR_PORT = 250;
 
 /**

src/shared/port-provider.js

@@ -164,14 +164,15 @@ export class PortProvider {
     }
 
     if (channel === 'guest-host' && message.frame1 === 'guest') {
-      this._emitter.emit('hostPortRequest', message.frame1, port2);
+      this._emitter.emit('frameConnected', message.frame1, port2);
     }
   }
 
   /**
-   * @param {'hostPortRequest'} eventName
+   * @param {'frameConnected'} eventName
    * @param {(source: 'guest', port: MessagePort) => void} handler - this handler
-   *   fires when a request for the 'guest-host' channel is listened.
+   *   fires when a request for the host frame has been granted. Currently, only
+   *   the 'guest-host' channel triggers this listener.
    */
   on(eventName, handler) {
     this._emitter.on(eventName, handler);
@@ -179,8 +180,11 @@ export class PortProvider {
 
   /**
    * Returns the `host` port from the `sidebar-host` channel.
+   *
+   * @param {'sidebar'} _targetFrame
    */
-  get sidebarPort() {
+  // eslint-disable-next-line no-unused-vars
+  hostPortFor(_targetFrame) {
     return this._sidebarHostChannel.port2;
   }
 

src/shared/test/port-finder-test.js

 import { delay } from '../../test-util/wait';
 import { PortFinder } from '../port-finder';
 
-const MAX_WAIT_FOR_PORT = 1000 * 5;
+const MAX_WAIT_FOR_PORT = 1000 * 10;
 
 describe('PortFinder', () => {
   const frame1 = 'guest';
@@ -122,7 +122,7 @@ describe('PortFinder', () => {
         clock.restore();
       }
 
-      assert.callCount(window.postMessage, 21);
+      assert.callCount(window.postMessage, 41);
       assert.alwaysCalledWithExactly(
         window.postMessage,
         { frame1, frame2: target, type: 'request' },

src/shared/test/port-provider-test.js

@@ -46,9 +46,9 @@ describe('PortProvider', () => {
     });
   });
 
-  describe('#sidebarPort', () => {
+  describe('#hostPortFor', () => {
     it('returns the `host` port of the `sidebar-host` channel', () => {
-      assert.instanceOf(portProvider.sidebarPort, MessagePort);
+      assert.instanceOf(portProvider.hostPortFor('sidebar'), MessagePort);
     });
   });
 
@@ -209,7 +209,7 @@ describe('PortProvider', () => {
     it('sends the counterpart port via the listener', async () => {
       portProvider.listen();
       const handler = sinon.stub();
-      portProvider.on('hostPortRequest', handler);
+      portProvider.on('frameConnected', handler);
       const data = {
         frame1: 'guest',
         frame2: 'host',

src/sidebar/services/frame-sync.js

@@ -285,12 +285,11 @@ export class FrameSyncService {
       this._guestRPC.call('setHighlightsVisible', visible);
     });
 
-    // Create channel for sidebar-host communication and send port to host.
+    // Create channel for sidebar-host communication.
     const hostPort = await this._portFinder.discover('host');
     this._hostRPC.createChannel(hostPort);
 
-    // Create channel for guest-sidebar communication
-    hostPort.start();
+    // Listen for guests connecting to the sidebar.
     this._listeners.add(hostPort, 'message', event => {
       const { data, ports } = /** @type {MessageEvent} */ (event);
       if (

src/sidebar/services/test/frame-sync-test.js

@@ -61,9 +61,11 @@ describe('FrameSyncService', () => {
   let fakeBridges;
   let fakePortFinder;
   let fakeStore;
-  let frameSync;
   let fakeWindow;
-  const { port1: hostPort, port2: sidebarPort } = new MessageChannel();
+
+  let frameSync;
+  let hostPort;
+  let sidebarPort;
 
   beforeEach(() => {
     fakeAnnotationsService = { create: sinon.stub() };
@@ -84,6 +86,11 @@ describe('FrameSyncService', () => {
       return bridge;
     });
 
+    const { port1, port2 } = new MessageChannel();
+    hostPort = port1;
+    sidebarPort = port2;
+    sidebarPort.start();
+
     fakePortFinder = {
       destroy: sinon.stub(),
       discover: sinon.stub().resolves(sidebarPort),