From 6dad72c1742bb46e602ad444d34e74fc449aec83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Sanz=20Garc=C3=ADa?= Date: Mon, 19 Jul 2021 11:35:34 +0200 Subject: [PATCH] Add support for `MessagePort` in `shared/bridge.js` Initially, I tried to if/else portions of the code to accommodate for `MessagePort`, aiming to avoid duplication of the code (like I did for `shared/frame-rpc.js` #3565). However, I found that, unlike `shared/frame-rpc.js`, this resulted into a spaghetti-type of code, not very understandable. Then, I decided to create two internal methods to support both the current communication using `Window` and the new `MessagePort`. This in my opinion leads to a clearer results, although some code is duplicated in both methods. This PR will result on a reduction in code coverage, which will be fix by #3590. --- src/annotator/cross-frame.js | 6 +- src/annotator/test/cross-frame-test.js | 6 +- src/shared/bridge.js | 79 +++++++++++++++++++++++--- src/shared/test/bridge-test.js | 46 ++++++++------- src/sidebar/services/frame-sync.js | 4 +- 5 files changed, 108 insertions(+), 33 deletions(-) diff --git a/src/annotator/cross-frame.js b/src/annotator/cross-frame.js index bbe15620269..f6e2f463b25 100644 --- a/src/annotator/cross-frame.js +++ b/src/annotator/cross-frame.js @@ -64,9 +64,9 @@ export class CrossFrame { }; // Initiate connection to the sidebar. - const onDiscoveryCallback = (source, origin, token) => - bridge.createChannel(source, origin, token); - discovery.startDiscovery(onDiscoveryCallback); + discovery.startDiscovery((source, origin, token) => + bridge.createChannel({ source, origin, token }) + ); frameObserver.observe(injectIntoFrame, iframeUnloaded); /** diff --git a/src/annotator/test/cross-frame-test.js b/src/annotator/test/cross-frame-test.js index f3e6e49dea7..07221aeada9 100644 --- a/src/annotator/test/cross-frame-test.js +++ b/src/annotator/test/cross-frame-test.js @@ -88,7 +88,11 @@ describe('CrossFrame', () => { createCrossFrame(); fakeDiscovery.startDiscovery.yield('SOURCE', 'ORIGIN', 'TOKEN'); assert.called(fakeBridge.createChannel); - assert.calledWith(fakeBridge.createChannel, 'SOURCE', 'ORIGIN', 'TOKEN'); + assert.calledWith(fakeBridge.createChannel, { + source: 'SOURCE', + origin: 'ORIGIN', + token: 'TOKEN', + }); }); }); diff --git a/src/shared/bridge.js b/src/shared/bridge.js index a9739933c8a..6a29358fe41 100644 --- a/src/shared/bridge.js +++ b/src/shared/bridge.js @@ -14,7 +14,7 @@ export default class Bridge { this.links = []; /** @type {Record void>} */ this.channelListeners = {}; - /** @type {Array<(channel: RPC, window: Window) => void>} */ + /** @type {Array<(channel: RPC, window?: Window) => void>} */ this.onConnectListeners = []; } @@ -27,18 +27,83 @@ export default class Bridge { this.links.forEach(channel => channel.destroy()); } + /** + * Deprecated - Remove after MessagePort conversion + * @typedef windowOptions + * @prop {Window} source - The source window + * @prop {string} origin - The origin of the document in `source` + * @prop {string} token - Shared token between the `source` and this window + * agreed during the discovery process + */ + + /** + * Create a communication channel between frames using either `MessagePort` or + * `Window`. + * + * The created channel is added to the list of channels which `call` + * and `on` send and receive messages over. + * + * @param {windowOptions|MessagePort} options + * @return {RPC} - Channel for communicating with the window. + */ + createChannel(options) { + if (options instanceof MessagePort) { + return this._createChannelForPort(options); + } else { + // Deprecated - Remove after MessagePort conversion + return this._createChannelForWindow(options); + } + } + + /** + * Create a communication channel using a `MessagePort`. + * + * The created channel is added to the list of channels which `call` + * and `on` send and receive messages over. + * + * @param {MessagePort} port + * @return {RPC} - Channel for communicating with the window. + */ + _createChannelForPort(port) { + const listeners = { connect: cb => cb(), ...this.channelListeners }; + + // Set up a channel + const channel = new RPC( + window /* dummy */, + port, + '*' /* dummy */, + listeners + ); + + let connected = false; + const ready = () => { + if (connected) { + return; + } + connected = true; + this.onConnectListeners.forEach(cb => cb(channel)); + }; + + // Fire off a connection attempt + channel.call('connect', ready); + + // Store the newly created channel in our collection + this.links.push(channel); + + return channel; + } + /** * Create a communication channel between this window and `source`. * * The created channel is added to the list of channels which `call` * and `on` send and receive messages over. * - * @param {Window} source - The source window. - * @param {string} origin - The origin of the document in `source`. - * @param {string} token + * @param {windowOptions} options * @return {RPC} - Channel for communicating with the window. + * @deprecated */ - createChannel(source, origin, token) { + _createChannelForWindow({ source, origin, token }) { let channel = null; let connected = false; @@ -53,7 +118,7 @@ export default class Bridge { const connect = (_token, cb) => { if (_token === token) { cb(); - ready(); + ready(); // This is necessary for testing only. } }; @@ -162,7 +227,7 @@ export default class Bridge { /** * Add a listener to be called upon a new connection. * - * @param {(channel: RPC, window: Window) => void} listener + * @param {(channel: RPC, window?: Window) => void} listener */ onConnect(listener) { this.onConnectListeners.push(listener); diff --git a/src/shared/test/bridge-test.js b/src/shared/test/bridge-test.js index f0734e9b50c..8ae24a919f8 100644 --- a/src/shared/test/bridge-test.js +++ b/src/shared/test/bridge-test.js @@ -22,7 +22,11 @@ describe('shared/bridge', () => { bridge = new Bridge(); createChannel = () => - bridge.createChannel(fakeWindow, 'http://example.com', 'TOKEN'); + bridge.createChannel({ + source: fakeWindow, + origin: 'http://example.com', + token: 'TOKEN', + }); fakeWindow = { postMessage: sandbox.stub(), @@ -85,11 +89,11 @@ describe('shared/bridge', () => { it('calls a callback when all channels return successfully', done => { const channel1 = createChannel(); - const channel2 = bridge.createChannel( - fakeWindow, - 'http://example.com', - 'NEKOT' - ); + const channel2 = bridge.createChannel({ + source: fakeWindow, + origin: 'http://example.com', + token: 'NEKOT', + }); channel1.call.yields(null, 'result1'); channel2.call.yields(null, 'result2'); @@ -105,11 +109,11 @@ describe('shared/bridge', () => { it('calls a callback with an error when a channels fails', done => { const error = new Error('Uh oh'); const channel1 = createChannel(); - const channel2 = bridge.createChannel( - fakeWindow, - 'http://example.com', - 'NEKOT' - ); + const channel2 = bridge.createChannel({ + source: fakeWindow, + origin: 'http://example.com', + token: 'NEKOT', + }); channel1.call.throws(error); channel2.call.yields(null, 'result2'); @@ -260,16 +264,16 @@ describe('shared/bridge', () => { describe('#destroy', () => it('destroys all opened channels', () => { - const channel1 = bridge.createChannel( - fakeWindow, - 'http://example.com', - 'foo' - ); - const channel2 = bridge.createChannel( - fakeWindow, - 'http://example.com', - 'bar' - ); + const channel1 = bridge.createChannel({ + source: fakeWindow, + origin: 'http://example.com', + token: 'foo', + }); + const channel2 = bridge.createChannel({ + source: fakeWindow, + origin: 'http://example.com', + token: 'bar', + }); bridge.destroy(); diff --git a/src/sidebar/services/frame-sync.js b/src/sidebar/services/frame-sync.js index 4a22a289808..b8dd2b1a7d6 100644 --- a/src/sidebar/services/frame-sync.js +++ b/src/sidebar/services/frame-sync.js @@ -236,7 +236,9 @@ export class FrameSyncService { }; const discovery = new Discovery(window, { server: true }); - discovery.startDiscovery(this._bridge.createChannel.bind(this._bridge)); + discovery.startDiscovery((source, origin, token) => + this._bridge.createChannel({ source, origin, token }) + ); this._bridge.onConnect(addFrame); this._setupSyncToFrame();