Issue 4579 - Promisify APIs

ext/common.js

Index: ext/common.js
===================================================================
--- a/ext/common.js
+++ b/ext/common.js
@@ -20,16 +20,105 @@
 (function()
 {
   // Both Edge and Mozilla Web Extensions use the namespace
   // 'browser' instead of 'chrome'. Edge has chrome namespace defined,
   // in some cases, but only with one property: 'app'.
   if (typeof chrome == "undefined" || typeof chrome.extension == "undefined")
     window.chrome = window.browser;
 
+  const asyncAPIs = [
+    "contextMenus.removeAll",
+    "devtools.panels.create",
+    "notifications.clear",
+    "notifications.create",
+    "runtime.openOptionsPage",
+    "runtime.sendMessage",
+    "runtime.setUninstallURL",
+    "storage.local.get",
+    "storage.local.remove",
+    "storage.local.set",
+    "storage.managed.get",
+    "tabs.create",
+    "tabs.get",
+    "tabs.insertCSS",
+    "tabs.query",
+    "tabs.reload",
+    "tabs.sendMessage",
+    "tabs.update",
+    "webNavigation.getAllFrames",
+    "webRequest.handlerBehaviorChanged",
+    "windows.create",
+    "windows.update"
+  ];
+
+  function wrapAPI(api)
+  {
+    let object = chrome;
+    let path = api.split(".");
+    let name = path.pop();
+
+    for (let node of path)
+    {
+      object = object[node];
+
+      if (!object)
+        return;
+    }
+
+    let func = object[name];
+    object[name] = function(...args)
+    {
+      if (typeof args[args.length - 1] == "function")
+        return func.apply(object, args);
+
+      // If the last argument is undefined, we drop it from the list assuming
+      // it stands for the optional callback. We must do this, because we have
+      // to replace it with our own callback. If we simply append our own
+      // callback to the list, it won't match the signature of the function and
+      // will cause an exception.
+      if (typeof args[args.length - 1] == "undefined")

[Wladimir Palant, 2017/10/13 09:53:14]

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?

[Manish Jethani, 2017/10/13 15:15:16]

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.

[Sebastian Noack, 2017/10/14 00:44:07]

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);

[Manish Jethani, 2017/10/14 01:07:30]

You're right, that's how it should have been.

Done.

[Sebastian Noack, 2017/10/14 01:31:21]

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.

[Manish Jethani, 2017/10/14 02:01:30]

Done.

+        args.pop();
+
+      return new Promise((resolve, reject) =>
+      {
+        func.call(object, ...args, result =>
+        {
+          let error = chrome.runtime.lastError;
+          if (error)
+            reject(error);
+          else
+            resolve(result);
+        });
+      });
+    };
+  }
+
+  function shouldWrapAPIs()
+  {
+    if (typeof browser == "undefined")

[Sebastian Noack, 2017/10/14 00:44:07]

Is this shortcut worth it?

[Manish Jethani, 2017/10/14 01:13:02]

I'm not sure about this one honestly. I'd say it doesn't hurt to leave it there.

[Sebastian Noack, 2017/10/14 01:31:21]

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.

[Manish Jethani, 2017/10/14 02:01:30]

OK, that makes sense.

Done.

+      return true;
+
+    try
+    {
+      return !(browser.storage.local.get([]) instanceof Promise);
+    }
+    catch (error)
+    {
+    }
+
+    return true;
+  }
+
+  if (shouldWrapAPIs())
+  {
+    for (let api of asyncAPIs)
+      wrapAPI(api);
+  }
+
   window.ext = {};
 
   let EventTarget = ext._EventTarget = function()
   {
     this._listeners = new Set();
   };
   EventTarget.prototype = {
     addListener(listener)