Skip to content

C
coopwire-hypothesis
Commit 00c59e0e authored by Eduardo Sanz García's avatar Eduardo Sanz García Committed by Eduardo
Browse files
Options

Apply suggestions from code review 

Co-authored-by: 's avatarRobert Knight <robertknight@gmail.com>
parent e78ada77
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 296 additions and 321 deletions
src/shared/port-finder.js
View file @ 00c59e0e
......@@ -2,7 +2,7 @@ import { ListenerCollection } from './listener-collection';
import { isMessageEqual, SOURCE as source } from './port-util';
const MAX_WAIT_FOR_PORT = 1000 * 30;
const POLLING_INTERVAL_FOR_PORT = 500;
const POLLING_INTERVAL_FOR_PORT = 250;
/**
* @typedef {import('../types/annotator').Destroyable} Destroyable
......@@ -12,8 +12,7 @@ const POLLING_INTERVAL_FOR_PORT = 500;
*/
/**
* PortFinder class should be used in frames that are not the `host` frame. It
* helps to discover `MessagePort` on a specific channel.
* PortFinder helps to discover `MessagePort` on a specific channel.
*
* Channel nomenclature is `[frame1]-[frame2]` so that:
* - `port1` should be owned by/transferred to `frame1`, and
......@@ -28,118 +27,65 @@ export class PortFinder {
this._listeners = new ListenerCollection();
}
// Two important characteristics of `MessagePort`:
// - it can only be used by one frame; the port is neutered if, after started to
// be used to receive messages, the port is transferred to a different frame.
// - messages are queued until the other port is ready to listen (`port.start()`)
destroy() {
this._listeners.removeAll();
}
/**
* `guest-host` communication
* @typedef {{channel: 'guest-host', hostFrame: Window, port: 'guest'}} options0
*
* `guest-sidebar` communication
* @typedef {{channel: 'guest-sidebar', hostFrame: Window, port: 'guest'}} options1
* Polls the hostFrame for a specific port and returns a Promise of the port.
*
* `host-sidebar` communication
* @typedef {{channel: 'host-sidebar', hostFrame: Window, port: 'sidebar'}} options2
*
* `notebook-sidebar` communication
* @typedef {{channel: 'notebook-sidebar', hostFrame: Window, port: 'notebook'}} options3
*
* @param {options0|options1|options2|options3} options
* @param {object} options
* @param {Channel} options.channel - requested channel
* @param {Window} options.hostFrame - frame where the hypothesis client is
* loaded and `PortProvider` is listening for messages
* @param {Port} options.port - requested port
* @return {Promise<MessagePort>}
*/
discover(options) {
const { channel, port } = options;
discover({ channel, hostFrame, port }) {
let isValidRequest = false;
if (
(channel === 'guest-host' && port === 'guest') ||
(channel === 'guest-sidebar' && port === 'guest') ||
(channel === 'host-sidebar' && port === 'sidebar') ||
(channel === 'notebook-sidebar' && port === 'notebook')
) {
isValidRequest = true;
}
return new Promise((resolve, reject) => {
if (
(channel === 'guest-host' && port === 'guest') ||
(channel === 'guest-sidebar' && port === 'guest') ||
(channel === 'host-sidebar' && port === 'sidebar') ||
(channel === 'notebook-sidebar' && port === 'notebook')
) {
this._requestPort({
...options,
reject,
resolve,
});
if (!isValidRequest) {
reject(new Error('Invalid request of channel/port'));
return;
}
reject(new Error('Invalid request of channel/port'));
});
}
/**
* @typedef RequestPortOptions
* @prop {Channel} channel - requested channel
* @prop {Window} hostFrame - the frame where the hypothesis client is loaded.
* It is used to send a `window.postMessage`.
* @prop {Port} port - requested port
* @prop {(reason: Error) => void} reject - execute the `Promise.reject` in case
* the `host` frame takes too long to answer the request.
* @prop {(port: MessagePort) => void} resolve - execute the `Promise.resolve`
* when `host` frame successfully answers the request.
*/
/**
* Register a listener for the port `offer` and sends a request for one port.
*
* @param {RequestPortOptions} options
*/
_requestPort({ channel, hostFrame, port, reject, resolve }) {
function postRequest() {
hostFrame.postMessage({ channel, port, source, type: 'request' }, '*');
}
const intervalId = window.setInterval(
() => postRequest(),
POLLING_INTERVAL_FOR_PORT
);
function postRequest() {
hostFrame.postMessage({ channel, port, source, type: 'request' }, '*');
}
// The `host` frame maybe busy, that's why we should wait.
const timeoutId = window.setTimeout(() => {
clearInterval(intervalId);
reject(
new Error(`Unable to find '${port}' port on '${channel}' channel`)
const intervalId = setInterval(
() => postRequest(),
POLLING_INTERVAL_FOR_PORT
);
}, MAX_WAIT_FOR_PORT);
// TODO: It would be nice to remove the listener after receiving the port.
this._listeners.add(window, 'message', event =>
this._handlePortOffer(/** @type {MessageEvent} */ (event), {
intervalId,
message: { channel, port, source, type: 'offer' },
resolve,
timeoutId,
})
);
postRequest();
}
// The `host` frame maybe busy, that's why we should wait.
const timeoutId = window.setTimeout(() => {
clearInterval(intervalId);
reject(
new Error(`Unable to find '${port}' port on '${channel}' channel`)
);
}, MAX_WAIT_FOR_PORT);
/**
* Resolve with a MessagePort when the `offer` message matches.
*
* @param {MessageEvent} event
* @param {object} options
* @param {Message} options.message
* @param {(port: MessagePort) => void} options.resolve
* @param {number} options.timeoutId
* @param {number} [options.intervalId]
*/
_handlePortOffer(
{ data, ports },
{ message, resolve, timeoutId, intervalId }
) {
if (isMessageEqual(data, message)) {
clearInterval(intervalId);
clearTimeout(timeoutId);
resolve(ports[0]);
}
}
// TODO: It would be nice to remove the listener after receiving the port.
this._listeners.add(window, 'message', event => {
const { data, ports } = /** @type {MessageEvent} */ (event);
if (isMessageEqual(data, { channel, port, source, type: 'offer' })) {
clearInterval(intervalId);
clearTimeout(timeoutId);
resolve(ports[0]);
}
});
destroy() {
this._listeners.removeAll();
postRequest();
});
}
}
src/shared/port-provider.js
View file @ 00c59e0e
This diff is collapsed.
src/shared/port-util.js
View file @ 00c59e0e
// Because there are many `postMessages` on the `host` frame, the SOURCE property
// is added to the hypothesis `postMessages` to identify the provenance of the
// message and avoid listening to messages that could have the same properties
// but different source. This is not a is not a security feature but an
// but different source. This is not a security feature but an
// anti-collision mechanism.
export const SOURCE = 'hypothesis';
/**
* These types are the used in by `PortProvider` and `PortFinder` for the
* inter-frame discovery and communication processes.
* @typedef {'guest-host'|'guest-sidebar'|'host-sidebar'|'notebook-sidebar'} Channel
* @typedef {'guest'|'host'|'notebook'|'sidebar'} Port
*
......
src/shared/test/port-finder-test.js
View file @ 00c59e0e
......@@ -26,92 +26,93 @@ describe('PortFinder', () => {
portFinder.destroy();
});
[
{ channel: 'invalid', port: 'guest' },
{ channel: 'guest-host', port: 'invalid' },
].forEach(({ channel, port }) =>
it('rejects if requesting an invalid port', async () => {
describe('#destroy', () => {
it('ignores `offer` messages of ports', async () => {
let error;
const channel = 'host-sidebar';
const port = 'sidebar';
const { port1 } = new MessageChannel();
const clock = sinon.useFakeTimers();
try {
await portFinder.discover({
channel,
hostFrame: window,
port,
portFinder
.discover({
channel,
hostFrame: window,
port,
})
.catch(e => (error = e));
portFinder.destroy();
sendMessage({
data: { channel, port, source, type: 'offer' },
ports: [port1],
});
} catch (e) {
error = e;
clock.tick(30000);
} finally {
clock.restore();
}
assert.equal(error.message, 'Invalid request of channel/port');
})
);
[
{ channel: 'guest-host', port: 'guest' },
{ channel: 'guest-sidebar', port: 'guest' },
{ channel: 'host-sidebar', port: 'sidebar' },
{ channel: 'notebook-sidebar', port: 'notebook' },
].forEach(({ channel, port }) =>
it('resolves if requesting a valid port', async () => {
const { port1 } = new MessageChannel();
let resolvedPort;
portFinder
.discover({
channel,
hostFrame: window,
port,
})
.then(port => (resolvedPort = port));
sendMessage({
data: { channel, port, source, type: 'offer' },
ports: [port1],
});
await delay(0);
assert.instanceOf(resolvedPort, MessagePort);
})
);
it("timeouts if host doesn't respond", async () => {
let error;
const channel = 'host-sidebar';
const port = 'sidebar';
const clock = sinon.useFakeTimers();
try {
portFinder
.discover({
channel,
hostFrame: window,
port,
})
.catch(e => (error = e));
clock.tick(30000);
} finally {
clock.restore();
}
assert.callCount(window.postMessage, 61);
assert.alwaysCalledWithExactly(
window.postMessage,
{ channel, port, source, type: 'request' },
'*'
assert.equal(
error.message,
"Unable to find 'sidebar' port on 'host-sidebar' channel"
);
});
});
describe('#discover', () => {
[
{ channel: 'invalid', port: 'guest' },
{ channel: 'guest-host', port: 'invalid' },
{ channel: 'guest-host', port: 'host' },
].forEach(({ channel, port }) =>
it('rejects if requesting an invalid port', async () => {
let error;
try {
await portFinder.discover({
channel,
hostFrame: window,
port,
});
} catch (e) {
error = e;
}
assert.equal(error.message, 'Invalid request of channel/port');
})
);
await delay(0);
[
{ channel: 'guest-host', port: 'guest' },
{ channel: 'guest-sidebar', port: 'guest' },
{ channel: 'host-sidebar', port: 'sidebar' },
{ channel: 'notebook-sidebar', port: 'notebook' },
].forEach(({ channel, port }) =>
it('resolves if requesting a valid port', async () => {
const { port1 } = new MessageChannel();
let resolvedPort;
assert.equal(
error.message,
"Unable to find 'sidebar' port on 'host-sidebar' channel"
portFinder
.discover({
channel,
hostFrame: window,
port,
})
.then(port => (resolvedPort = port));
sendMessage({
data: { channel, port, source, type: 'offer' },
ports: [port1],
});
await delay(0);
assert.instanceOf(resolvedPort, MessagePort);
})
);
});
describe('#destroy', () => {
it('ignores `offer` messages of ports', async () => {
it("timeouts if host doesn't respond", async () => {
let error;
const channel = 'host-sidebar';
const port = 'sidebar';
const { port1 } = new MessageChannel();
const clock = sinon.useFakeTimers();
try {
......@@ -122,16 +123,18 @@ describe('PortFinder', () => {
port,
})
.catch(e => (error = e));
portFinder.destroy();
sendMessage({
data: { channel, port, source, type: 'offer' },
ports: [port1],
});
clock.tick(30000);
} finally {
clock.restore();
}
assert.callCount(window.postMessage, 121);
assert.alwaysCalledWithExactly(
window.postMessage,
{ channel, port, source, type: 'request' },
'*'
);
await delay(0);
assert.equal(
......
src/shared/test/port-provider-test.js
View file @ 00c59e0e
......@@ -23,7 +23,8 @@ describe('PortProvider', () => {
beforeEach(() => {
sinon.stub(window, 'postMessage');
portProvider = new PortProvider(window.location.href);
portProvider = new PortProvider(window.location.origin);
portProvider.listen();
});
afterEach(() => {
......@@ -31,6 +32,23 @@ describe('PortProvider', () => {
portProvider.destroy();
});
describe('#destroy', () => {
it('ignores valid port request if `PortFinder` has been destroyed', async () => {
portProvider.destroy();
sendMessage({
data: {
channel: 'host-sidebar',
port: 'sidebar',
source,
type: 'request',
},
});
await delay(0);
assert.notCalled(window.postMessage);
});
});
describe('#getPort', () => {
it('returns `null` if called with wrong arguments', () => {
let hostPort;
......@@ -59,20 +77,30 @@ describe('PortProvider', () => {
});
});
describe('#destroy', () => {
it('ignores valid port request if `PortFinder` has been destroyed', async () => {
describe('#listen', () => {
it('ignores all port request until start listening', async () => {
portProvider.destroy();
portProvider = new PortProvider(window.location.origin);
const data = {
channel: 'host-sidebar',
port: 'sidebar',
source,
type: 'request',
};
sendMessage({
data: {
channel: 'host-sidebar',
port: 'sidebar',
source,
type: 'request',
},
data,
});
await delay(0);
assert.notCalled(window.postMessage);
portProvider.listen();
sendMessage({
data,
});
await delay(0);
assert.calledOnce(window.postMessage);
});
});
......@@ -94,7 +122,6 @@ describe('PortProvider', () => {
data,
source: new MessageChannel().port1,
});
await delay(0);
assert.notCalled(window.postMessage);
......@@ -132,7 +159,6 @@ describe('PortProvider', () => {
data,
origin: 'https://dummy.com',
});
await delay(0);
assert.notCalled(window.postMessage);
......@@ -148,7 +174,6 @@ describe('PortProvider', () => {
sendMessage({
data,
});
await delay(0);
assert.calledWith(
......@@ -217,7 +242,7 @@ describe('PortProvider', () => {
it('sends the reciprocal port of the `guest-host` channel (via listener)', async () => {
const handler = sinon.stub();
portProvider.addEventListener('onHostPortRequest', handler);
portProvider.on('hostPortRequest', handler);
const data = {
channel: 'guest-host',
port: 'guest',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment