Issue 29585594: Issue 4579 - Wrap runtime.onMessage (Closed)
|
Created:
Oct. 23, 2017, 2:25 a.m. by Manish Jethani Modified:
Jan. 9, 2018, 10 a.m. Reviewers:
kzar CC:
Oleksandr, Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 4579 - Wrap runtime.onMessage
Patch Set 1 #
Total comments: 6
Patch Set 2 : Wrap removeListener and hasListener #
Total comments: 8
Patch Set 3 : Rebase #Patch Set 4 : Rename functions #MessagesTotal messages: 7
Patch Set 1 https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js#newcode128 polyfill.js:128: // Unlike Firefox and Microsoft Edge, Chrome doesn't have a "browser" Unrelated, but there's an ESLint error here because of line length. https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js#newcode146 polyfill.js:146: if (wait instanceof Promise) There's no need to add a catch handler, Chrome (and probably Edge) handles this automagically. The error will be logged to the console, the sender will get undefined as a response. Firefox actually doesn't send an undefined response to the sender, instead it sends an "unexpected error" in the sender's catch handler. We cannot mimic this part, unfortunately. We could probably override the Firefox version of the API to behave like Chrome's (send an undefined response), but I don't know if it matters so much.
https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js#newcode146 polyfill.js:146: if (wait instanceof Promise) On 2017/10/23 02:32:45, Manish Jethani wrote: > There's no need to add a catch handler, Chrome (and probably Edge) handles this > automagically. The error will be logged to the console, the sender will get > undefined as a response. Not really, the message clearly says that not handling promise rejects is deprecated. So you should handle rejections explicitly. In particular, you need to make sure that a response is always sent and be it undefined. > Firefox actually doesn't send an undefined response to the sender, instead it > sends an "unexpected error" in the sender's catch handler. We cannot mimic this > part, unfortunately. We *could*, by adjusting the behavior of the sendMessage wrapper so that it recognizes some special values as rejections. Probably not worth doing however. https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js#newcode151 polyfill.js:151: }; You have a problem here, removeListener will no longer work. The best solution should be a WeakMap mapping wrapers to original listeners. You will have to wrap removeListener then and remove the wrapper if you have it in the map. Ideally, the same should be done with hasListener.
Patch Set 2: Wrap removeListener and hasListener https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js#newcode146 polyfill.js:146: if (wait instanceof Promise) On 2017/10/23 08:53:26, Wladimir Palant wrote: > On 2017/10/23 02:32:45, Manish Jethani wrote: > > There's no need to add a catch handler, Chrome (and probably Edge) handles > this > > automagically. The error will be logged to the console, the sender will get > > undefined as a response. > > Not really, the message clearly says that not handling promise rejects is > deprecated. So you should handle rejections explicitly. In particular, you need > to make sure that a response is always sent and be it undefined. Did you mean that Chrome will no longer do this automatically in future versions? Done. > > Firefox actually doesn't send an undefined response to the sender, instead it > > sends an "unexpected error" in the sender's catch handler. We cannot mimic > this > > part, unfortunately. > > We *could*, by adjusting the behavior of the sendMessage wrapper so that it > recognizes some special values as rejections. Probably not worth doing however. Ack. https://codereview.adblockplus.org/29585594/diff/29585595/polyfill.js#newcode151 polyfill.js:151: }; On 2017/10/23 08:53:26, Wladimir Palant wrote: > You have a problem here, removeListener will no longer work. The best solution > should be a WeakMap mapping wrapers to original listeners. You will have to wrap > removeListener then and remove the wrapper if you have it in the map. Ideally, > the same should be done with hasListener. Done. https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode126 polyfill.js:126: if (typeof listener != "function") We must check the type here to be consistent with Firefox. Note that typeof also works for Proxy instances. https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode147 polyfill.js:147: // sendResponse can throw if the internal port is closed; be sure I haven't verified this, but in any case adding a finally here seems like a good idea.
Sorry for being so slow to look at this one as well. It took me a while to grok but this LGTM, with a couple of suggestions for better function names which are up to you. https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode61 polyfill.js:61: function wrapAPI(api) Since we're now wrapping the message APIs as well perhaps we should rename this to `wrapAsyncAPI` which would be consistent with the `asyncAPIs` variable name as well? https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode119 polyfill.js:119: function wrapMessageAPIs() Nit: `wrapRuntimeOnMessage`? https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode126 polyfill.js:126: if (typeof listener != "function") On 2017/10/23 12:21:04, Manish Jethani wrote: > We must check the type here to be consistent with Firefox. > > Note that typeof also works for Proxy instances. Acknowledged. https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode147 polyfill.js:147: // sendResponse can throw if the internal port is closed; be sure On 2017/10/23 12:21:04, Manish Jethani wrote: > I haven't verified this, but in any case adding a finally here seems like a good > idea. Acknowledged.
Patch Set 4: Rename functions https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode61 polyfill.js:61: function wrapAPI(api) On 2018/01/08 14:35:16, kzar wrote: > Since we're now wrapping the message APIs as well perhaps we should rename this > to `wrapAsyncAPI` which would be consistent with the `asyncAPIs` variable name > as well? Done. https://codereview.adblockplus.org/29585594/diff/29586577/polyfill.js#newcode119 polyfill.js:119: function wrapMessageAPIs() On 2018/01/08 14:35:16, kzar wrote: > Nit: `wrapRuntimeOnMessage`? Done.
Even more LGTM 
|
