Issue 4579 - Promisify APIs

ext/common.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 (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)
     {
       this._listeners.add(listener);
@@ skipping 39 matching lines @@
     {
       return chrome.extension.getBackgroundPage();
     }
   };
 
 
   /* Utils */
 
   ext.getURL = chrome.extension.getURL;
 }());