From 3e46e800a02aa1f7d70f179e70efda95aa3239c1 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 25 Oct 2019 13:37:28 +0200 Subject: [PATCH 1/5] [MessageHandler] Replace the internal `isReply` property, as sent when Promise callbacks are used, with enumeration values Given that the `isReply` property is an internal implementation detail, changing its type shouldn't be a problem. Note that by directly indicating if either data or an Error is sent, it's no longer necessary to use `in` when handling the callback. --- src/shared/message_handler.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 97c1fe5cc..97b5352d4 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -18,6 +18,12 @@ import { ReadableStream, UnexpectedResponseException, UnknownErrorException } from './util'; +const CallbackKind = { + UNKNOWN: 0, + DATA: 1, + ERROR: 2, +}; + const StreamKind = { UNKNOWN: 0, CANCEL: 1, @@ -73,15 +79,18 @@ function MessageHandler(sourceName, targetName, comObj) { } if (data.stream) { this._processStreamMessage(data); - } else if (data.isReply) { + } else if (data.callback) { let callbackId = data.callbackId; if (data.callbackId in callbacksCapabilities) { let callback = callbacksCapabilities[callbackId]; delete callbacksCapabilities[callbackId]; - if ('reason' in data) { + + if (data.callback === CallbackKind.DATA) { + callback.resolve(data.data); + } else if (data.callback === CallbackKind.ERROR) { callback.reject(wrapReason(data.reason)); } else { - callback.resolve(data.data); + throw new Error('Unexpected callback case'); } } else { throw new Error(`Cannot resolve callback ${callbackId}`); @@ -97,7 +106,7 @@ function MessageHandler(sourceName, targetName, comObj) { comObj.postMessage({ sourceName, targetName, - isReply: true, + callback: CallbackKind.DATA, callbackId: data.callbackId, data: result, }); @@ -105,7 +114,7 @@ function MessageHandler(sourceName, targetName, comObj) { comObj.postMessage({ sourceName, targetName, - isReply: true, + callback: CallbackKind.ERROR, callbackId: data.callbackId, reason: wrapReason(reason), }); From 62f28e11a3ce8e7f19c0b7d7e4308f4780500e2a Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 30 Oct 2019 11:44:59 +0100 Subject: [PATCH 2/5] [MessageHandler] Remove unnecessary usage of `in` from the code Note that using `in` leads to unnecessary stringification of the properties, which seems completely unnecessary here. To avoid future problems from these changes the `MessageHandler.on` method will now assert, in non-`PRODUCTION`/`TESTING` builds, that it's always called with a function as expected. This patch also renames `callbacksCapabilities` to `callbackCapabilities`, note the removed "s", since using a double plural format looks a bit strange. --- src/shared/message_handler.js | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 97b5352d4..80519ef88 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -69,8 +69,8 @@ function MessageHandler(sourceName, targetName, comObj) { this.postMessageTransfers = true; this.streamSinks = Object.create(null); this.streamControllers = Object.create(null); - let callbacksCapabilities = this.callbacksCapabilities = Object.create(null); - let ah = this.actionHandler = Object.create(null); + this.callbackCapabilities = Object.create(null); + const ah = this.actionHandler = Object.create(null); this._onComObjOnMessage = (event) => { let data = event.data; @@ -80,23 +80,24 @@ function MessageHandler(sourceName, targetName, comObj) { if (data.stream) { this._processStreamMessage(data); } else if (data.callback) { - let callbackId = data.callbackId; - if (data.callbackId in callbacksCapabilities) { - let callback = callbacksCapabilities[callbackId]; - delete callbacksCapabilities[callbackId]; + const callbackId = data.callbackId; + const capability = this.callbackCapabilities[callbackId]; + + if (capability) { + delete this.callbackCapabilities[callbackId]; if (data.callback === CallbackKind.DATA) { - callback.resolve(data.data); + capability.resolve(data.data); } else if (data.callback === CallbackKind.ERROR) { - callback.reject(wrapReason(data.reason)); + capability.reject(wrapReason(data.reason)); } else { throw new Error('Unexpected callback case'); } } else { throw new Error(`Cannot resolve callback ${callbackId}`); } - } else if (data.action in ah) { - let action = ah[data.action]; + } else if (ah[data.action]) { + const action = ah[data.action]; if (data.callbackId) { let sourceName = this.sourceName; let targetName = data.sourceName; @@ -133,7 +134,12 @@ function MessageHandler(sourceName, targetName, comObj) { MessageHandler.prototype = { on(actionName, handler) { - var ah = this.actionHandler; + if (typeof PDFJSDev === 'undefined' || + PDFJSDev.test('!PRODUCTION || TESTING')) { + assert(typeof handler === 'function', + 'MessageHandler.on: Expected "handler" to be a function.'); + } + const ah = this.actionHandler; if (ah[actionName]) { throw new Error(`There is already an actionName called "${actionName}"`); } @@ -164,7 +170,7 @@ MessageHandler.prototype = { sendWithPromise(actionName, data, transfers) { var callbackId = this.callbackId++; var capability = createPromiseCapability(); - this.callbacksCapabilities[callbackId] = capability; + this.callbackCapabilities[callbackId] = capability; try { this.postMessage({ sourceName: this.sourceName, From f61fb3e0f926555dd5fd6ecd67d7371a1e6857ff Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 30 Oct 2019 12:12:50 +0100 Subject: [PATCH 3/5] [MessageHandler] Re-factor the `_onComObjOnMessage` function to use early returns When `ReadableStream` support was added to the `MessageHandler`, the `_onComObjOnMessage` function became more complex than previously. All of the nested `if`/`else if`/`else` branches are now, at least in my opinion, making some of this code a bit difficult to follow. Hence this patch, which attempts to help readability by making use of early `return`s and `Error`s. The patch also changes a couple of `var`/`let` occurences to `const`. --- src/shared/message_handler.js | 89 ++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 43 deletions(-) diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 80519ef88..cf5601f26 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -70,64 +70,67 @@ function MessageHandler(sourceName, targetName, comObj) { this.streamSinks = Object.create(null); this.streamControllers = Object.create(null); this.callbackCapabilities = Object.create(null); - const ah = this.actionHandler = Object.create(null); + this.actionHandler = Object.create(null); this._onComObjOnMessage = (event) => { - let data = event.data; + const data = event.data; if (data.targetName !== this.sourceName) { return; } if (data.stream) { this._processStreamMessage(data); - } else if (data.callback) { + return; + } + if (data.callback) { const callbackId = data.callbackId; const capability = this.callbackCapabilities[callbackId]; - - if (capability) { - delete this.callbackCapabilities[callbackId]; - - if (data.callback === CallbackKind.DATA) { - capability.resolve(data.data); - } else if (data.callback === CallbackKind.ERROR) { - capability.reject(wrapReason(data.reason)); - } else { - throw new Error('Unexpected callback case'); - } - } else { + if (!capability) { throw new Error(`Cannot resolve callback ${callbackId}`); } - } else if (ah[data.action]) { - const action = ah[data.action]; - if (data.callbackId) { - let sourceName = this.sourceName; - let targetName = data.sourceName; - new Promise(function(resolve) { - resolve(action(data.data)); - }).then(function(result) { - comObj.postMessage({ - sourceName, - targetName, - callback: CallbackKind.DATA, - callbackId: data.callbackId, - data: result, - }); - }, function(reason) { - comObj.postMessage({ - sourceName, - targetName, - callback: CallbackKind.ERROR, - callbackId: data.callbackId, - reason: wrapReason(reason), - }); - }); - } else if (data.streamId) { - this._createStreamSink(data); + delete this.callbackCapabilities[callbackId]; + + if (data.callback === CallbackKind.DATA) { + capability.resolve(data.data); + } else if (data.callback === CallbackKind.ERROR) { + capability.reject(wrapReason(data.reason)); } else { - action(data.data); + throw new Error('Unexpected callback case'); } - } else { + return; + } + const action = this.actionHandler[data.action]; + if (!action) { throw new Error(`Unknown action from worker: ${data.action}`); } + if (data.callbackId) { + const sourceName = this.sourceName; + const targetName = data.sourceName; + new Promise(function(resolve) { + resolve(action(data.data)); + }).then(function(result) { + comObj.postMessage({ + sourceName, + targetName, + callback: CallbackKind.DATA, + callbackId: data.callbackId, + data: result, + }); + }, function(reason) { + comObj.postMessage({ + sourceName, + targetName, + callback: CallbackKind.ERROR, + callbackId: data.callbackId, + reason: wrapReason(reason), + }); + }); + return; + } + if (data.streamId) { + this._createStreamSink(data); + return; + } + action(data.data); }; comObj.addEventListener('message', this._onComObjOnMessage); } From 5d5733c0a733f52ffbd4ad213870891727542e82 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 30 Oct 2019 17:37:52 +0100 Subject: [PATCH 4/5] [MessageHandler] Convert all instances of `var` to `const` in the code --- .eslintrc | 1 + src/shared/message_handler.js | 41 +++++++++++++++++------------------ 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/.eslintrc b/.eslintrc index 61feb3f70..feb1e8207 100644 --- a/.eslintrc +++ b/.eslintrc @@ -198,6 +198,7 @@ "object-shorthand": ["error", "always", { "avoidQuotes": true, }], + "prefer-const": "off", "rest-spread-spacing": ["error", "never"], "sort-imports": ["error", { "ignoreCase": true, diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index cf5601f26..51cb283cd 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -12,6 +12,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +/* eslint no-var: error, prefer-const: error */ import { AbortException, assert, createPromiseCapability, MissingPDFException, @@ -171,8 +172,8 @@ MessageHandler.prototype = { * @returns {Promise} Promise to be resolved with response data. */ sendWithPromise(actionName, data, transfers) { - var callbackId = this.callbackId++; - var capability = createPromiseCapability(); + const callbackId = this.callbackId++; + const capability = createPromiseCapability(); this.callbackCapabilities[callbackId] = capability; try { this.postMessage({ @@ -198,14 +199,14 @@ MessageHandler.prototype = { * @returns {ReadableStream} ReadableStream to read data in chunks. */ sendWithStream(actionName, data, queueingStrategy, transfers) { - let streamId = this.streamId++; - let sourceName = this.sourceName; - let targetName = this.targetName; + const streamId = this.streamId++; + const sourceName = this.sourceName; + const targetName = this.targetName; const comObj = this.comObj; return new ReadableStream({ start: (controller) => { - let startCapability = createPromiseCapability(); + const startCapability = createPromiseCapability(); this.streamControllers[streamId] = { controller, startCall: startCapability, @@ -226,7 +227,7 @@ MessageHandler.prototype = { }, pull: (controller) => { - let pullCapability = createPromiseCapability(); + const pullCapability = createPromiseCapability(); this.streamControllers[streamId].pullCall = pullCapability; comObj.postMessage({ sourceName, @@ -242,7 +243,7 @@ MessageHandler.prototype = { cancel: (reason) => { assert(reason instanceof Error, 'cancel must have a valid reason'); - let cancelCapability = createPromiseCapability(); + const cancelCapability = createPromiseCapability(); this.streamControllers[streamId].cancelCall = cancelCapability; this.streamControllers[streamId].isClosed = true; comObj.postMessage({ @@ -259,21 +260,19 @@ MessageHandler.prototype = { }, _createStreamSink(data) { - let self = this; - let action = this.actionHandler[data.action]; - let streamId = data.streamId; - let desiredSize = data.desiredSize; - let sourceName = this.sourceName; - let targetName = data.sourceName; - let capability = createPromiseCapability(); + const self = this; + const action = this.actionHandler[data.action]; + const streamId = data.streamId; + const sourceName = this.sourceName; + const targetName = data.sourceName; const comObj = this.comObj; - let streamSink = { + const streamSink = { enqueue(chunk, size = 1, transfers) { if (this.isCancelled) { return; } - let lastDesiredSize = this.desiredSize; + const lastDesiredSize = this.desiredSize; this.desiredSize -= size; // Enqueue decreases the desiredSize property of sink, // so when it changes from positive to negative, @@ -320,11 +319,11 @@ MessageHandler.prototype = { }); }, - sinkCapability: capability, + sinkCapability: createPromiseCapability(), onPull: null, onCancel: null, isCancelled: false, - desiredSize, + desiredSize: data.desiredSize, ready: null, }; @@ -353,9 +352,9 @@ MessageHandler.prototype = { }, _processStreamMessage(data) { - let sourceName = this.sourceName; - let targetName = data.sourceName; const streamId = data.streamId; + const sourceName = this.sourceName; + const targetName = data.sourceName; const comObj = this.comObj; switch (data.stream) { From 0293222b9690ff41a0daa50043a6111e889928c2 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 30 Oct 2019 22:05:25 +0100 Subject: [PATCH 5/5] [MessageHandler] Convert the code to a proper `class` --- src/shared/message_handler.js | 188 ++++++++++++++++++---------------- 1 file changed, 100 insertions(+), 88 deletions(-) diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 51cb283cd..a8d44073d 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -61,82 +61,82 @@ function wrapReason(reason) { } } -function MessageHandler(sourceName, targetName, comObj) { - this.sourceName = sourceName; - this.targetName = targetName; - this.comObj = comObj; - this.callbackId = 1; - this.streamId = 1; - this.postMessageTransfers = true; - this.streamSinks = Object.create(null); - this.streamControllers = Object.create(null); - this.callbackCapabilities = Object.create(null); - this.actionHandler = Object.create(null); +class MessageHandler { + constructor(sourceName, targetName, comObj) { + this.sourceName = sourceName; + this.targetName = targetName; + this.comObj = comObj; + this.callbackId = 1; + this.streamId = 1; + this.postMessageTransfers = true; + this.streamSinks = Object.create(null); + this.streamControllers = Object.create(null); + this.callbackCapabilities = Object.create(null); + this.actionHandler = Object.create(null); - this._onComObjOnMessage = (event) => { - const data = event.data; - if (data.targetName !== this.sourceName) { - return; - } - if (data.stream) { - this._processStreamMessage(data); - return; - } - if (data.callback) { - const callbackId = data.callbackId; - const capability = this.callbackCapabilities[callbackId]; - if (!capability) { - throw new Error(`Cannot resolve callback ${callbackId}`); + this._onComObjOnMessage = (event) => { + const data = event.data; + if (data.targetName !== this.sourceName) { + return; } - delete this.callbackCapabilities[callbackId]; - - if (data.callback === CallbackKind.DATA) { - capability.resolve(data.data); - } else if (data.callback === CallbackKind.ERROR) { - capability.reject(wrapReason(data.reason)); - } else { - throw new Error('Unexpected callback case'); + if (data.stream) { + this._processStreamMessage(data); + return; } - return; - } - const action = this.actionHandler[data.action]; - if (!action) { - throw new Error(`Unknown action from worker: ${data.action}`); - } - if (data.callbackId) { - const sourceName = this.sourceName; - const targetName = data.sourceName; - new Promise(function(resolve) { - resolve(action(data.data)); - }).then(function(result) { - comObj.postMessage({ - sourceName, - targetName, - callback: CallbackKind.DATA, - callbackId: data.callbackId, - data: result, - }); - }, function(reason) { - comObj.postMessage({ - sourceName, - targetName, - callback: CallbackKind.ERROR, - callbackId: data.callbackId, - reason: wrapReason(reason), - }); - }); - return; - } - if (data.streamId) { - this._createStreamSink(data); - return; - } - action(data.data); - }; - comObj.addEventListener('message', this._onComObjOnMessage); -} + if (data.callback) { + const callbackId = data.callbackId; + const capability = this.callbackCapabilities[callbackId]; + if (!capability) { + throw new Error(`Cannot resolve callback ${callbackId}`); + } + delete this.callbackCapabilities[callbackId]; + + if (data.callback === CallbackKind.DATA) { + capability.resolve(data.data); + } else if (data.callback === CallbackKind.ERROR) { + capability.reject(wrapReason(data.reason)); + } else { + throw new Error('Unexpected callback case'); + } + return; + } + const action = this.actionHandler[data.action]; + if (!action) { + throw new Error(`Unknown action from worker: ${data.action}`); + } + if (data.callbackId) { + const sourceName = this.sourceName; + const targetName = data.sourceName; + new Promise(function(resolve) { + resolve(action(data.data)); + }).then(function(result) { + comObj.postMessage({ + sourceName, + targetName, + callback: CallbackKind.DATA, + callbackId: data.callbackId, + data: result, + }); + }, function(reason) { + comObj.postMessage({ + sourceName, + targetName, + callback: CallbackKind.ERROR, + callbackId: data.callbackId, + reason: wrapReason(reason), + }); + }); + return; + } + if (data.streamId) { + this._createStreamSink(data); + return; + } + action(data.data); + }; + comObj.addEventListener('message', this._onComObjOnMessage); + } -MessageHandler.prototype = { on(actionName, handler) { if (typeof PDFJSDev === 'undefined' || PDFJSDev.test('!PRODUCTION || TESTING')) { @@ -148,7 +148,8 @@ MessageHandler.prototype = { throw new Error(`There is already an actionName called "${actionName}"`); } ah[actionName] = handler; - }, + } + /** * Sends a message to the comObj to invoke the action with the supplied data. * @param {string} actionName - Action to call. @@ -156,13 +157,14 @@ MessageHandler.prototype = { * @param {Array} [transfers] - List of transfers/ArrayBuffers. */ send(actionName, data, transfers) { - this.postMessage({ + this._postMessage({ sourceName: this.sourceName, targetName: this.targetName, action: actionName, data, }, transfers); - }, + } + /** * Sends a message to the comObj to invoke the action with the supplied data. * Expects that the other side will callback with the response. @@ -176,7 +178,7 @@ MessageHandler.prototype = { const capability = createPromiseCapability(); this.callbackCapabilities[callbackId] = capability; try { - this.postMessage({ + this._postMessage({ sourceName: this.sourceName, targetName: this.targetName, action: actionName, @@ -187,7 +189,8 @@ MessageHandler.prototype = { capability.reject(ex); } return capability.promise; - }, + } + /** * Sends a message to the comObj to invoke the action with the supplied data. * Expect that the other side will callback to signal 'start_complete'. @@ -214,7 +217,7 @@ MessageHandler.prototype = { cancelCall: null, isClosed: false, }; - this.postMessage({ + this._postMessage({ sourceName, targetName, action: actionName, @@ -257,8 +260,11 @@ MessageHandler.prototype = { return cancelCapability.promise; }, }, queueingStrategy); - }, + } + /** + * @private + */ _createStreamSink(data) { const self = this; const action = this.actionHandler[data.action]; @@ -281,7 +287,7 @@ MessageHandler.prototype = { this.sinkCapability = createPromiseCapability(); this.ready = this.sinkCapability.promise; } - self.postMessage({ + self._postMessage({ sourceName, targetName, stream: StreamKind.ENQUEUE, @@ -349,8 +355,11 @@ MessageHandler.prototype = { reason: wrapReason(reason), }); }); - }, + } + /** + * @private + */ _processStreamMessage(data) { const streamId = data.streamId; const sourceName = this.sourceName; @@ -482,8 +491,11 @@ MessageHandler.prototype = { default: throw new Error('Unexpected stream case'); } - }, + } + /** + * @private + */ async _deleteStreamController(streamId) { // Delete the `streamController` only when the start, pull, and cancel // capabilities have settled, to prevent `TypeError`s. @@ -495,26 +507,26 @@ MessageHandler.prototype = { return capability && capability.promise.catch(function() { }); })); delete this.streamControllers[streamId]; - }, + } /** * Sends raw message to the comObj. - * @private * @param {Object} message - Raw message. * @param transfers List of transfers/ArrayBuffers, or undefined. + * @private */ - postMessage(message, transfers) { + _postMessage(message, transfers) { if (transfers && this.postMessageTransfers) { this.comObj.postMessage(message, transfers); } else { this.comObj.postMessage(message); } - }, + } destroy() { this.comObj.removeEventListener('message', this._onComObjOnMessage); - }, -}; + } +} export { MessageHandler,