Issue 29338275: Issue 3499 - Create a clean messaging API for internal use (Closed)
|
Created:
March 15, 2016, 10:57 a.m. by Wladimir Palant Modified:
April 18, 2016, 10:32 a.m. Visibility:
Public. |
DescriptionIssue 3499 - Create a clean messaging API for internal use
Repository: hg.adblockplus.org/adblockplus
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixed copyright year #
Total comments: 38
Patch Set 3 : Allow duplicate callbacks, introduce isPromise() function, minor changes #Patch Set 4 : Don't store all responses unnecessarily #
MessagesTotal messages: 16
On 2016/03/15 10:57:25, Wladimir Palant wrote: So much of the event related code is implemented in low level sdk/event/core module https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/event_core and I think we can reuse it here in the child process, since we appear to use an sdk loader there. For the parent process side there is a global loader that we can use to access sdk/event/core like so: const { require } = Cu.import("resource://gre/modules/commonjs/toolkit/require.js", {}); const { ... } = require("sdk/event/core") or we can use the devtools loader: const { require } = Cu.import('resource://devtools/shared/Loader.jsm', {}).devtools; I think the former is only used by jpm add-ons, and the devtools loader may be lazy loaded.
On 2016/03/15 10:57:25, Wladimir Palant wrote: The overall approach looks good to me!
https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:69: function getSender(origin) I'm not quire sure when this would be used, since sent messages using `MESSAGE_NAME` appear to only occur above and they appear to not use the `message.target` attribute.
On 2016/03/15 20:27:54, Erik wrote: > So much of the event related code is implemented in low level sdk/event/core > module https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/event_core > and I think we can reuse it here in the child process, since we appear to use an > sdk loader there. While the API is obviously inspired by the Add-on SDK and Node, I think that actually using the SDK implementation is a very bad idea: 1) It's a low-level module meaning that it's an implementation detail (to a higher degree than a loader which is meant for external consumption). 2) The SDK is abandoned so I'd not bet on issues being fixed if they arise. 3) The SDK is notoriously having issues on applications other than Firefox, issues that nobody feels responsible for. The loader is something we can easily replace if we need to. But I'd object to adding more dependencies on the SDK at this point, except for unit tests maybe because our users don't care about those. Besides, the SDK doesn't have all the necessary functionality - e.g. no synchronous messaging. https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:69: function getSender(origin) On 2016/03/15 22:56:35, Erik wrote: > I'm not quire sure when this would be used, since sent messages using > `MESSAGE_NAME` appear to only occur above and they appear to not use the > `message.target` attribute. This is something that the first-run page will need - it subscribes to notifications, so messageResponder module needs a way to remember the sender of the "subscriptions.listen" message.
https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:3: * Copyright (C) 2006-2015 Eyeo GmbH It's 2016. https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:31: else if (messageManager instanceof Ci.nsIMessageBroadcaster) Nit: Redundant else https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:36: else Nit: Redundant else
https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:3: * Copyright (C) 2006-2015 Eyeo GmbH On 2016/03/16 10:44:13, Sebastian Noack wrote: > It's 2016. Right, this patch is older... Fixed. https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:31: else if (messageManager instanceof Ci.nsIMessageBroadcaster) On 2016/03/16 10:44:13, Sebastian Noack wrote: > Nit: Redundant else As you know, I like redundant else clauses :). While the compiler doesn't need them, humans like us can immediately see that only one branch is being taken without having to look at the contents of the blocks. https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:36: else On 2016/03/16 10:44:13, Sebastian Noack wrote: > Nit: Redundant else Same as above.
https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:251: callbacks.splice(index, 1); If a listener is temporarily added we leak the key and empty array. But I'm not sure either whether this is worth to be addressed.
https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338276/lib/messaging.js#ne... lib/messaging.js:251: callbacks.splice(index, 1); On 2016/03/16 11:53:32, Sebastian Noack wrote: > If a listener is temporarily added we leak the key and empty array. But I'm not > sure either whether this is worth to be addressed. IMHO not really, so far we don't have a single call to .off() in the codebase - our listeners are usually permanent. Should this change at some point, it might be that the temporary listeners are always being added for the same message in which case the current behavior is actually better.
On 2016/03/15 10:57:25, Wladimir Palant wrote: LGTM, I only mentioned some minor things.
https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:58: if (typeof response == "undefined") nit `==` -> `===` https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:61: if (typeof result == "undefined") nit `==` -> `===` https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:152: if (result && typeof result.then == "function") It'd be nice to have a `isPromise` function to reuse, I think it might make this bit of code a little easier to read. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:183: if (responses.length == expectedResponses) nit `==` -> `===` https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:216: * @param {LightWeightPort} sender object that can be used to communicate with this param appears to just accept a function, and I don't think it would accept a `LightWeightPort`. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:220: * response). `on` doesn't appear to return a promise, or anything. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:226: * @param {Port~messageHandler} callback same comment as above, this appears to just accept a function, also I'm not sure why this function appears to have two sets of docs. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:241: * @param {Port~messageHandler} callback if I am not mistaken about the above comments for `on`, then this should just be `function` as well. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:275: return new Promise((resolve, reject) => { nit add a newline here?
https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:49: return response; Detail: What's the reason for not using `flattenResponses()` here? https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:142: var response = {callbackID, payload}; Detail: Unless there's a particular reason not to, please use `let` here like in the rest of this file. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:152: if (result && typeof result.then == "function") On 2016/03/17 05:02:04, Erik wrote: > It'd be nice to have a `isPromise` function to reuse, I think it might make this > bit of code a little easier to read. You mean `result instanceof Promise`? https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:182: responses.push(payload); Why do you wait for and store all the responses if you're throwing all but one of them away anyway? https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:196: callbacks = callbacks.slice(); Why do you copy the array? I don't see that a callback would modify the array while iterating through it. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:231: this._callbacks.set(messageName, []); Detail: Since you're using `Map` I wonder why you're not using `Set` to avoid having to deal with array indices. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:257: * @param [payload] data to attach to the message Detail: Is this valid JSDoc syntax even with the type missing?
https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:49: return response; On 2016/03/17 12:09:26, Thomas Greiner wrote: > Detail: What's the reason for not using `flattenResponses()` here? No real reason, flattenResponses() was simply introduced after the function here. Done. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:58: if (typeof response == "undefined") On 2016/03/17 05:02:04, Erik wrote: > nit `==` -> `===` We generally don't use === unless it makes a difference. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:61: if (typeof result == "undefined") On 2016/03/17 05:02:04, Erik wrote: > nit `==` -> `===` Same as above. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:142: var response = {callbackID, payload}; On 2016/03/17 12:09:26, Thomas Greiner wrote: > Detail: Unless there's a particular reason not to, please use `let` here like in > the rest of this file. Done. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:152: if (result && typeof result.then == "function") On 2016/03/17 12:09:26, Thomas Greiner wrote: > You mean `result instanceof Promise`? No, `result instanceof Promise` would be wrong - there isn't necessary a single Promise class (shim/native + different frames), and there can also be promise-like classes, e.g. Task. I added a function isPromise(). https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:182: responses.push(payload); On 2016/03/17 12:09:26, Thomas Greiner wrote: > Why do you wait for and store all the responses if you're throwing all but one > of them away anyway? Because otherwise this code gets rather complicated, at least if I still want to warn about multiple responses. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:183: if (responses.length == expectedResponses) On 2016/03/17 05:02:04, Erik wrote: > nit `==` -> `===` Same as above. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:196: callbacks = callbacks.slice(); On 2016/03/17 12:09:26, Thomas Greiner wrote: > Why do you copy the array? I don't see that a callback would modify the array > while iterating through it. It sure could. Classic example is a callback that removes itself when called. This works fine as long it's the only callback. However, if there is a second callback in the list it will be skipped. We actually hit that exact issue for other code not too long ago. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:216: * @param {LightWeightPort} sender object that can be used to communicate with On 2016/03/17 05:02:05, Erik wrote: > this param appears to just accept a function, and I don't think it would accept > a `LightWeightPort`. Explanation below. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:220: * response). On 2016/03/17 05:02:05, Erik wrote: > `on` doesn't appear to return a promise, or anything. Explanation below. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:226: * @param {Port~messageHandler} callback On 2016/03/17 05:02:04, Erik wrote: > same comment as above, this appears to just accept a function, also I'm not sure > why this function appears to have two sets of docs. Please see http://usejsdoc.org/tags-callback.html - the first block documents the callback type. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:231: this._callbacks.set(messageName, []); On 2016/03/17 12:09:26, Thomas Greiner wrote: > Detail: Since you're using `Map` I wonder why you're not using `Set` to avoid > having to deal with array indices. As Sebastian pointed out, Node.js (unlike Add-on SDK) allows duplicate callbacks. I changed the logic here to be consistent. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:241: * @param {Port~messageHandler} callback On 2016/03/17 05:02:04, Erik wrote: > if I am not mistaken about the above comments for `on`, then this should just be > `function` as well. Same as above. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:257: * @param [payload] data to attach to the message On 2016/03/17 12:09:26, Thomas Greiner wrote: > Detail: Is this valid JSDoc syntax even with the type missing? Yes, it is. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:275: return new Promise((resolve, reject) => { On 2016/03/17 05:02:04, Erik wrote: > nit add a newline here? Done.
https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:182: responses.push(payload); On 2016/03/21 15:31:31, Wladimir Palant wrote: > On 2016/03/17 12:09:26, Thomas Greiner wrote: > > Why do you wait for and store all the responses if you're throwing all but one > > of them away anyway? > > Because otherwise this code gets rather complicated, at least if I still want to > warn about multiple responses. So decrementing `expectedResponses` is not an option or do you mean that you intend to access individual responses in a later change? https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:196: callbacks = callbacks.slice(); On 2016/03/21 15:31:32, Wladimir Palant wrote: > On 2016/03/17 12:09:26, Thomas Greiner wrote: > > Why do you copy the array? I don't see that a callback would modify the array > > while iterating through it. > > It sure could. Classic example is a callback that removes itself when called. > This works fine as long it's the only callback. However, if there is a second > callback in the list it will be skipped. We actually hit that exact issue for > other code not too long ago. Of course, in theory it could happen because it's not a "private" variable. However, that'd require other code to access `_callbacks` from outside which it's not supposed to do. Do you think that convention is insufficient here? https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:231: this._callbacks.set(messageName, []); On 2016/03/21 15:31:31, Wladimir Palant wrote: > On 2016/03/17 12:09:26, Thomas Greiner wrote: > > Detail: Since you're using `Map` I wonder why you're not using `Set` to avoid > > having to deal with array indices. > > As Sebastian pointed out, Node.js (unlike Add-on SDK) allows duplicate > callbacks. I changed the logic here to be consistent. Interesting, I wonder why they thought that'd be a good idea…
https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:182: responses.push(payload); On 2016/03/21 18:43:22, Thomas Greiner wrote: > So decrementing `expectedResponses` is not an option or do you mean that you > intend to access individual responses in a later change? Without storing all the values in an array we cannot call flattenResponses and reuse the code. Anyway, I solved this by using a generator-like object. I used a proper generator first but the generator protocol is too complicated for this scenario. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:196: callbacks = callbacks.slice(); On 2016/03/21 18:43:22, Thomas Greiner wrote: > Of course, in theory it could happen because it's not a "private" variable. > However, that'd require other code to access `_callbacks` from outside which > it's not supposed to do. No, it would merely have to call port.off(arguments.callee). This would modify the list of callbacks right under our feet. https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:231: this._callbacks.set(messageName, []); On 2016/03/21 18:43:23, Thomas Greiner wrote: > Interesting, I wonder why they thought that'd be a good idea… Probably because it's simpler/more efficient to implement. Also, arguably eliminating duplicate callbacks tends to mask programming errors.
LGTM https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js File lib/messaging.js (right): https://codereview.adblockplus.org/29338275/diff/29338401/lib/messaging.js#ne... lib/messaging.js:196: callbacks = callbacks.slice(); On 2016/03/21 20:42:34, Wladimir Palant wrote: > On 2016/03/21 18:43:22, Thomas Greiner wrote: > > Of course, in theory it could happen because it's not a "private" variable. > > However, that'd require other code to access `_callbacks` from outside which > > it's not supposed to do. > > No, it would merely have to call port.off(arguments.callee). This would modify > the list of callbacks right under our feet. Ah, right. Nevermind then. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
