Issue 7334 - Remove support for callbacks in API wrappers

polyfill.js

Index: polyfill.js
===================================================================
--- a/polyfill.js
+++ b/polyfill.js
@@ -107,26 +107,25 @@
     // If the property is not writable assigning it will fail, so we use
     // Object.defineProperty here instead. Assuming the property isn't
     // inherited its other attributes (e.g. enumerable) are preserved,
     // except for accessor attributes (e.g. get and set) which are discarded
     // since we're specifying a value.
     Object.defineProperty(object, name, {
       value(...args)
       {
-        if (typeof args[args.length - 1] == "function")
-          return func.apply(object, args);
+        let lastArgumentType = typeof args[args.length - 1];
 
-        // 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")
-          args.pop();
+        // If the last argument is undefined, we assume it stands for the
+        // optional callback.
+        if (lastArgumentType == "function" ||
+            lastArgumentType == "undefined" && args.length > 0)

[Manish Jethani, 2019/04/24 14:42:23]

We need to support calls like `browser.runtime.openOptionsPage()`.

[Sebastian Noack, 2019/04/24 19:50:38]

I just tested on Firefox (where the promise-based API is supported natively)
without the polyfill, and browser.runtime.openOptionsPage(undefined) just works
fine. So we shouldn't emulate an error in that case.

[Manish Jethani, 2019/04/24 20:09:47]

But we're not? Quite the opposite, the additional change I made is about letting
`browser.runtime.openOptionsPage()` work. Did I miss something in the rebase?

[Sebastian Noack, 2019/04/24 20:42:29]

Like I read the code here, you throw if the last argument is undefined given
explicitly, Firefox doesn't. Do I miss something?

[Manish Jethani, 2019/04/25 08:04:54]

Are you suggesting we don't throw the error at all?

My original idea was to do this so we catch mistakes where we're still using
callbacks unknowingly. Of course this can cause trouble when you have something
like this:

  function openOptions(callback)
  {
    return browser.runtime.openOptionsPage(callback);
  }

  openOptions();

It will throw. But that is exactly the intention here. If it throws, we need to
migrate that code to use promises. After a couple of releases we can remove the
check for `"undefined"` and finally we remove it for `"function"` as well.

[Sebastian Noack, 2019/04/25 08:18:21]

I think a polyfill should behave like the implementation it is imitating. I see
your point, but once we drop the polyfill and assuming Chrome by then will
behave like Firefox, we won't have that feature anymore anyway.

[Manish Jethani, 2019/04/25 08:51:20]

Fair enough, in that case we have two options:

 1.  Check only for `"function"`
 2.  Remove any checks

I don't think we should have the checks at all if we're sure we've migrated to
promises.

+        {
+          throw new Error("Callbacks are no longer supported.");
+        }
 
         let resolvePromise = null;
         let rejectPromise = null;
 
         func.call(object, ...args, result =>
         {
           let error = browser.runtime.lastError;
           if (error && !portClosedBeforeResponseError.test(error.message))