Issue 29573892: Issue 4579 - Promisify APIs (Closed)
|
Created:
Oct. 11, 2017, 7:16 p.m. by Manish Jethani Modified:
Oct. 18, 2017, 11 a.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 4579 - Promisify APIs
Patch Set 1 #
Total comments: 3
Patch Set 2 : Do not wrap APIs on Firefox #
Total comments: 5
Patch Set 3 : Use loop #
Total comments: 10
Patch Set 4 : Do not return promise if last argument is undefined #Patch Set 5 : Revert patch set 4 #Patch Set 6 : Remove browser check #Patch Set 7 : Rebase #
Total comments: 9
Patch Set 8 : Use anonymous block #Patch Set 9 : Use arrow functions #Patch Set 10 : Revert patch set 9 #
Total comments: 2
MessagesTotal messages: 24
Patch Set 1 I thought the proxy-based approach was too unnecessarily expensive, this is much simpler and more efficient. https://codereview.adblockplus.org/29573892/diff/29573893/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29573893/ext/common.js#newco... ext/common.js:28: const asyncAPIs = [ This is the list of async APIs we actually use. Ideally this would be a tree of objects rather than a list of strings, but there are only so many of these, it's slightly inefficient but it doesn't matter. It's definitely easier to maintain this way. https://codereview.adblockplus.org/29573892/diff/29573893/ext/common.js#newco... ext/common.js:77: if (typeof args[args.length - 1] == "undefined") This should work, but if it doesn't we can always use the arity of the function. https://codereview.adblockplus.org/29573892/diff/29573893/ext/common.js#newco... ext/common.js:84: let error = chrome.runtime.lastError; It's OK to refer to chrome directly here. It will be replaced with browser.
Patch Set 2: Do not wrap APIs on Firefox https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js#newco... ext/common.js:94: if (typeof browser == "undefined" || I was going for runtime.getPlatformInfo, but it's not available in content scripts. storage.local.get with an empty list of keys should be a no-op.
(Adding Wladimir to Cc since he has done something similar for EasyPasswords IIRC.)
I think this should be landed after https://codereview.adblockplus.org/29570614/, and then go into polyfill.js, rather than ext/compat.js. https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js#newco... ext/common.js:58: let [root, ...subPath] = api.split("."); Would this logic here potentially somewhat more light-weight if asyncAPIs would be an array of arrays? https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js#newco... ext/common.js:95: !(browser.storage.local.get([]) instanceof Promise)) Wouldn't you get an error here on Chrome, because a function is expected as second argument? I think we need to do something like this: https://github.com/mozilla/webextension-polyfill/issues/3#issuecomment-259425339
Patch Set 3: Use loop https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js#newco... ext/common.js:58: let [root, ...subPath] = api.split("."); On 2017/10/13 04:04:19, Sebastian Noack wrote: > Would this logic here potentially somewhat more light-weight if asyncAPIs would > be an array of arrays? It would be more efficient if it were were an array of arrays, but then it would be a little harder to maintain. I don't think we would gain much from "normalizing" that list. I have modified the code now to use a loop instead of recursion. https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js#newco... ext/common.js:95: !(browser.storage.local.get([]) instanceof Promise)) On 2017/10/13 04:04:19, Sebastian Noack wrote: > Wouldn't you get an error here on Chrome, because a function is expected as > second argument? I think we need to do something like this: > https://github.com/mozilla/webextension-polyfill/issues/3#issuecomment-259425339 It would be Edge on which the error might be thrown. I've wrapped it in a try...catch for now, but we'd have to confirm that it's needed.
On 2017/10/13 04:04:19, Sebastian Noack wrote: > I think this should be landed after > https://codereview.adblockplus.org/29570614/, and then go into polyfill.js, > rather than ext/compat.js. I'll rebase once that one has landed. The logic wouldn't change, except in shouldWrapAPIs we'd have to refer to the original window.browser.
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:78: if (typeof args[args.length - 1] == "undefined") I don't get why this is necessary - the last argument is only undefined if our callers pass undefined to the function. Why should they?
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/13 09:53:14, Wladimir Palant wrote: > I don't get why this is necessary - the last argument is only undefined if our > callers pass undefined to the function. Why should they? Sometimes there's code like this: function setPref(key, value, callback) { chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); } This is one example from popup.js, I know there are at least a couple more. If we don't drop the last argument inside the wrapper, we end up passing an extra argument to the original function and get an error. A good way to avoid this is to use default parameters: function setPref(key, value, callback = () => {}) { chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); } We could also just switch to using promises in such instances.
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/13 15:15:16, Manish Jethani wrote: > On 2017/10/13 09:53:14, Wladimir Palant wrote: > > I don't get why this is necessary - the last argument is only undefined if our > > callers pass undefined to the function. Why should they? > > Sometimes there's code like this: > > function setPref(key, value, callback) > { > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > } > > This is one example from popup.js, I know there are at least a couple more. > > If we don't drop the last argument inside the wrapper, we end up passing an > extra argument to the original function and get an error. > > A good way to avoid this is to use default parameters: > > function setPref(key, value, callback = () => {}) > { > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > } > > We could also just switch to using promises in such instances. If we assume the last argument if undefined being the callback argument, why don't we treat it like the callback argument? let lastArgType = typeof args[args.length - 1]; if (lastArgType == "function" || lastArgType == "undefined") function.apply(object, args); https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:97: if (typeof browser == "undefined") Is this shortcut worth it?
Patch Set 4: Do not return promise if last argument is undefined https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/14 00:44:07, Sebastian Noack wrote: > On 2017/10/13 15:15:16, Manish Jethani wrote: > > On 2017/10/13 09:53:14, Wladimir Palant wrote: > > > I don't get why this is necessary - the last argument is only undefined if > our > > > callers pass undefined to the function. Why should they? > > > > Sometimes there's code like this: > > > > function setPref(key, value, callback) > > { > > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > > } > > > > This is one example from popup.js, I know there are at least a couple more. > > > > If we don't drop the last argument inside the wrapper, we end up passing an > > extra argument to the original function and get an error. > > > > A good way to avoid this is to use default parameters: > > > > function setPref(key, value, callback = () => {}) > > { > > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > > } > > > > We could also just switch to using promises in such instances. > > If we assume the last argument if undefined being the callback argument, why > don't we treat it like the callback argument? > > let lastArgType = typeof args[args.length - 1]; > if (lastArgType == "function" || lastArgType == "undefined") > function.apply(object, args); You're right, that's how it should have been. Done.
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:97: if (typeof browser == "undefined") On 2017/10/14 00:44:07, Sebastian Noack wrote: > Is this shortcut worth it? I'm not sure about this one honestly. I'd say it doesn't hurt to leave it there.
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/14 01:07:30, Manish Jethani wrote: > On 2017/10/14 00:44:07, Sebastian Noack wrote: > > On 2017/10/13 15:15:16, Manish Jethani wrote: > > > On 2017/10/13 09:53:14, Wladimir Palant wrote: > > > > I don't get why this is necessary - the last argument is only undefined if > > our > > > > callers pass undefined to the function. Why should they? > > > > > > Sometimes there's code like this: > > > > > > function setPref(key, value, callback) > > > { > > > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > > > } > > > > > > This is one example from popup.js, I know there are at least a couple more. > > > > > > If we don't drop the last argument inside the wrapper, we end up passing an > > > extra argument to the original function and get an error. > > > > > > A good way to avoid this is to use default parameters: > > > > > > function setPref(key, value, callback = () => {}) > > > { > > > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > > > } > > > > > > We could also just switch to using promises in such instances. > > > > If we assume the last argument if undefined being the callback argument, why > > don't we treat it like the callback argument? > > > > let lastArgType = typeof args[args.length - 1]; > > if (lastArgType == "function" || lastArgType == "undefined") > > function.apply(object, args); > > You're right, that's how it should have been. > > Done. Actually, I just noticed, the way you had it before matched the behavior of the "browser" object on Firefox. So never mind my above comment, and feel free to change it back. Sorry. https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:97: if (typeof browser == "undefined") On 2017/10/14 01:13:02, Manish Jethani wrote: > On 2017/10/14 00:44:07, Sebastian Noack wrote: > > Is this shortcut worth it? > > I'm not sure about this one honestly. I'd say it doesn't hurt to leave it there. Well, we generally do feature detection, not browser detection where possible. Let's say Chrome will support promises at some point in the extension API, we would have to remove that check. However, if they don't, not having that check, relying on our feature detection won't hurt.
Patch Set 6: Remove browser check https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/14 01:31:21, Sebastian Noack wrote: > On 2017/10/14 01:07:30, Manish Jethani wrote: > > On 2017/10/14 00:44:07, Sebastian Noack wrote: > > > On 2017/10/13 15:15:16, Manish Jethani wrote: > > > > On 2017/10/13 09:53:14, Wladimir Palant wrote: > > > > > I don't get why this is necessary - the last argument is only undefined > if > > > our > > > > > callers pass undefined to the function. Why should they? > > > > > > > > Sometimes there's code like this: > > > > > > > > function setPref(key, value, callback) > > > > { > > > > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > > > > } > > > > > > > > This is one example from popup.js, I know there are at least a couple > more. > > > > > > > > If we don't drop the last argument inside the wrapper, we end up passing > an > > > > extra argument to the original function and get an error. > > > > > > > > A good way to avoid this is to use default parameters: > > > > > > > > function setPref(key, value, callback = () => {}) > > > > { > > > > chrome.runtime.sendMessage({type: "prefs.set", key, value}, callback); > > > > } > > > > > > > > We could also just switch to using promises in such instances. > > > > > > If we assume the last argument if undefined being the callback argument, why > > > don't we treat it like the callback argument? > > > > > > let lastArgType = typeof args[args.length - 1]; > > > if (lastArgType == "function" || lastArgType == "undefined") > > > function.apply(object, args); > > > > You're right, that's how it should have been. > > > > Done. > > Actually, I just noticed, the way you had it before matched the behavior of the > "browser" object on Firefox. So never mind my above comment, and feel free to > change it back. Sorry. Done. https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newco... ext/common.js:97: if (typeof browser == "undefined") On 2017/10/14 01:31:21, Sebastian Noack wrote: > On 2017/10/14 01:13:02, Manish Jethani wrote: > > On 2017/10/14 00:44:07, Sebastian Noack wrote: > > > Is this shortcut worth it? > > > > I'm not sure about this one honestly. I'd say it doesn't hurt to leave it > there. > > Well, we generally do feature detection, not browser detection where possible. > Let's say Chrome will support promises at some point in the extension API, we > would have to remove that check. However, if they don't, not having that check, > relying on our feature detection won't hurt. OK, that makes sense. Done.
LGTM
Patch Set 7: Rebase https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (left): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#oldcode20 polyfill.js:20: // Unlike Firefox and Microsoft Edge, Chrome doesn't have a "browser" object, It's best to move this inside, after the call to shouldWrapAPIs, see below. https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() This all needs to be wrapped inside an anonymous function here so we don't pollute the global namespace. https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode49 polyfill.js:49: let object = browser; Note: Using browser now instead of chrome https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode104 polyfill.js:104: // Unlike Firefox and Microsoft Edge, Chrome doesn't have a "browser" object, After calling shouldWrapAPIs we can safely overwrite window.browser
Patch Set 8: Use anonymous block https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 13:33:24, Manish Jethani wrote: > This all needs to be wrapped inside an anonymous function here so we don't > pollute the global namespace. Actually this can just be an anonymous block. Done.
https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 17:35:45, Manish Jethani wrote: > On 2017/10/17 13:33:24, Manish Jethani wrote: > > This all needs to be wrapped inside an anonymous function here so we don't > > pollute the global namespace. > > Actually this can just be an anonymous block. > > Done. IIRC Firefox got upset when declaring functions inside of a block like that.
Patch Set 9: Use arrow functions https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 18:13:42, kzar wrote: > On 2017/10/17 17:35:45, Manish Jethani wrote: > > On 2017/10/17 13:33:24, Manish Jethani wrote: > > > This all needs to be wrapped inside an anonymous function here so we don't > > > pollute the global namespace. > > > > Actually this can just be an anonymous block. > > > > Done. > > IIRC Firefox got upset when declaring functions inside of a block like that. You're right, the function declarations would be "hoisted" to the outer scope here. Fixed, by using arrow functions.
https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 20:48:45, Manish Jethani wrote: > On 2017/10/17 18:13:42, kzar wrote: > > On 2017/10/17 17:35:45, Manish Jethani wrote: > > > On 2017/10/17 13:33:24, Manish Jethani wrote: > > > > This all needs to be wrapped inside an anonymous function here so we don't > > > > pollute the global namespace. > > > > > > Actually this can just be an anonymous block. > > > > > > Done. > > > > IIRC Firefox got upset when declaring functions inside of a block like that. > > You're right, the function declarations would be "hoisted" to the outer scope > here. > > Fixed, by using arrow functions. Both of you are wrong. In strict mode, as of ES2015, and FF47+, function declarations are scoped to the surrounding block. Hence declaring named functions is totally fine, and IMO preferable, here. However, what Dave is talking about is; before ES2015, function declarations in nested blocks were non-standard, and the behavior varied across JavaScript implementations, e.g. Firefox <47 threw an error.
Patch Set 10: Revert patch set 9 https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 23:31:03, Sebastian Noack wrote: > On 2017/10/17 20:48:45, Manish Jethani wrote: > > On 2017/10/17 18:13:42, kzar wrote: > > > On 2017/10/17 17:35:45, Manish Jethani wrote: > > > > On 2017/10/17 13:33:24, Manish Jethani wrote: > > > > > This all needs to be wrapped inside an anonymous function here so we > don't > > > > > pollute the global namespace. > > > > > > > > Actually this can just be an anonymous block. > > > > > > > > Done. > > > > > > IIRC Firefox got upset when declaring functions inside of a block like that. > > > > You're right, the function declarations would be "hoisted" to the outer scope > > here. > > > > Fixed, by using arrow functions. > > Both of you are wrong. In strict mode, as of ES2015, and FF47+, function > declarations are scoped to the surrounding block. Hence declaring named > functions is totally fine, and IMO preferable, here. > > However, what Dave is talking about is; before ES2015, function declarations > in nested blocks were non-standard, and the behavior varied across JavaScript > implementations, e.g. Firefox <47 threw an error. Ah, of course. Reverted.
LGTM
Message was sent while issue was closed.
LGTM Addressing the nit below is optional. https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js#newcode98 polyfill.js:98: return true; Nit: I would put that return inside the catch block which makes the logic clearer IMHO.
Message was sent while issue was closed.
On 2017/10/18 08:59:12, Wladimir Palant wrote: > LGTM Thanks! > Addressing the nit below is optional. I've left it as it is for now. I'm seeing it more as a default return value when all checks fail (we may have multiple checks in the future). https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js#newcode98 polyfill.js:98: return true; On 2017/10/18 08:59:11, Wladimir Palant wrote: > Nit: I would put that return inside the catch block which makes the logic > clearer IMHO. Acknowledged. 
|
