Apply suggestions from code review

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

src/shared/port-finder.js

@@ -35,7 +35,7 @@ export class PortFinder {
   }
 
   /**
-   * Request a specific port from `hostFrame`
+   * Request a specific port from the host frame
    *
    * @param {Frame} target - the frame aiming to be discovered
    * @return {Promise<MessagePort>}

src/shared/port-provider.js

 import { TinyEmitter } from 'tiny-emitter';
 
 import { ListenerCollection } from './listener-collection';
-import { isMessageEqual } from './port-util';
+import { isMessageEqual, isSourceWindow } from './port-util';
 
 /**
  * @typedef {import('../types/annotator').Destroyable} Destroyable
@@ -119,27 +119,6 @@ export class PortProvider {
     ];
   }
 
-  /**
-   * Check that source is of type Window.
-   *
-   * @param {MessageEventSource|null} source
-   * @return {source is Window}
-   */
-  _isSourceWindow(source) {
-    if (
-      // `source` can be of type Window | MessagePort | ServiceWorker.
-      // The simple check `source instanceof Window`` doesn't work here.
-      // Alternatively, `source` could be casted `/** @type{Window} */ (source)`
-      source === null ||
-      source instanceof MessagePort ||
-      source instanceof ServiceWorker
-    ) {
-      return false;
-    }
-
-    return true;
-  }
-
   /**
    * Check that data and origin matches the expected values.
    *
@@ -175,7 +154,7 @@ export class PortProvider {
    *     send this port either, (1) to the `sidebar` frame using the `sidebar-host`
    *     channel or (2) through the `onHostPortRequest` event listener.
    */
-  _sendPort({ channel, message, origin, source, port1, port2 }) {
+  _sendPorts({ channel, message, origin, source, port1, port2 }) {
     source.postMessage(message, origin, [port1]);
 
     if (!port2) {
@@ -216,7 +195,7 @@ export class PortProvider {
         messageEvent
       );
 
-      if (!this._isSourceWindow(source)) {
+      if (!isSourceWindow(source)) {
         return;
       }
 
@@ -259,7 +238,7 @@ export class PortProvider {
       // constructor.
       if (channel === 'sidebar-host') {
         windowChannelMap.set(source, this._sidebarHostChannel);
-        this._sendPort({
+        this._sendPorts({
           port1: this._sidebarHostChannel.port1,
           ...options,
         });
@@ -270,7 +249,7 @@ export class PortProvider {
       windowChannelMap.set(source, messageChannel);
 
       const { port1, port2 } = messageChannel;
-      this._sendPort({ port1, port2, ...options });
+      this._sendPorts({ port1, port2, ...options });
     });
   }
 

src/shared/port-util.js

@@ -59,3 +59,24 @@ export function isMessageEqual(data, message) {
     return false;
   }
 }
+
+/**
+ * Check that source is of type Window.
+ *
+ * @param {MessageEventSource|null} source
+ * @return {source is Window}
+ */
+export function isSourceWindow(source) {
+  if (
+    // `source` can be of type Window | MessagePort | ServiceWorker.
+    // The simple check `source instanceof Window`` doesn't work here.
+    // Alternatively, `source` could be casted `/** @type{Window} */ (source)`
+    source === null ||
+    source instanceof MessagePort ||
+    source instanceof ServiceWorker
+  ) {
+    return false;
+  }
+
+  return true;
+}

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

@@ -82,31 +82,6 @@ describe('PortProvider', () => {
   });
 
   describe('listens for port requests', () => {
-    [
-      { source: null, reason: 'source is null' },
-      {
-        source: new MessageChannel().port1,
-        reason: 'source is a MessageChannel',
-      },
-    ].forEach(({ source, reason }) =>
-      it(`ignores port requests if ${reason}`, async () => {
-        portProvider.listen();
-        const data = {
-          authority,
-          frame1: 'sidebar',
-          frame2: 'host',
-          type: 'request',
-        };
-
-        await sendPortFinderRequest({
-          data,
-          source,
-        });
-
-        assert.notCalled(window.postMessage);
-      })
-    );
-
     [
       // Disabled this check because it make axes-core to crash
       // Reported: https://github.com/dequelabs/axe-core/pull/3249
@@ -147,6 +122,16 @@ describe('PortProvider', () => {
         },
         reason: 'contains an invalid type',
       },
+      {
+        data: {
+          authority,
+          frame1: 'sidebar',
+          frame2: 'host',
+          type: 'request',
+        },
+        source: null,
+        reason: 'comes from invalid source',
+      },
       {
         data: {
           authority,
@@ -157,12 +142,13 @@ describe('PortProvider', () => {
         origin: 'https://dummy.com',
         reason: 'comes from invalid origin',
       },
-    ].forEach(({ data, reason, origin }) => {
+    ].forEach(({ data, reason, origin, source }) => {
       it(`ignores port request if message ${reason}`, async () => {
         portProvider.listen();
         await sendPortFinderRequest({
           data,
-          origin: origin ?? window.location.origin,
+          origin,
+          source,
         });
 
         assert.notCalled(window.postMessage);

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

-import { isMessageEqual } from '../port-util';
+import { isMessageEqual, isSourceWindow } from '../port-util';
 
 describe('port-util', () => {
   describe('isMessageEqual', () => {
@@ -97,4 +97,33 @@ describe('port-util', () => {
       });
     });
   });
+
+  describe('isSourceWindow', () => {
+    [
+      {
+        expectedResult: true,
+        reason: 'Window',
+        source: window,
+      },
+      {
+        expectedResult: false,
+        reason: 'null',
+        source: null,
+      },
+      {
+        expectedResult: false,
+        reason: 'MessagePort',
+        source: new MessageChannel().port1,
+      },
+      {
+        expectedResult: false,
+        reason: 'ServiceWorker',
+        source: Object.create(ServiceWorker.prototype),
+      },
+    ].forEach(({ expectedResult, reason, source }) =>
+      it(`returns '${expectedResult}' because the source is ${reason}`, () => {
+        assert.equal(expectedResult, isSourceWindow(source));
+      })
+    );
+  });
 });