Issue 6388 - Wrap inherited function properties as well

polyfill.js

Index: polyfill.js
diff --git a/polyfill.js b/polyfill.js
index f80c98f6ee6efed6b1da4d2fca0107723985f736..cf104289d1a3a627341470ac9fc7780dc50a6786 100644
--- a/polyfill.js
+++ b/polyfill.js
@@ -78,60 +78,59 @@
     if (!func)
       return;
 
-    let descriptor = Object.getOwnPropertyDescriptor(object, name);
-
-    delete descriptor["get"];
-    delete descriptor["set"];
-
-    descriptor.value = function(...args)
-    {
-      let callStack = new Error().stack;
+    // If the property is not writable assigning will fail, so we use
+    // Object.defineProperty here instead. Assuming the property isn't
+    // inherited its other attributes (e.g. enumerable) are presereved,

[Manish Jethani, 2018/02/16 14:03:53]

Nit: "preserved" (spelling)

[kzar, 2018/02/16 16:41:08]

Done.

+    // except accessor properties since we're specifying a value.

[Manish Jethani, 2018/02/16 14:03:53]

Nit: "except accessor attributes" (?)

[kzar, 2018/02/16 16:41:08]

How's this?

[Manish Jethani, 2018/02/16 16:47:38]

Since "enumerable" is referred to as an attribute in the previous part of the
sentence, I thought the accessors ("get" and "set") should also be referred to
as attributes, but I may be wrong. Anyway this is very minor.

+    Object.defineProperty(object, name, {
+      value: function(...args)

[Manish Jethani, 2018/02/16 14:03:53]

Since we're using an object literal here now we can use the shorthand:

  Object.defineProperty(object, name, {
    value(...args)
    {
      ...
    }
  });

i.e. no function keyword

[kzar, 2018/02/16 16:41:08]

Good point, eslint picked that up I just forgot to run it. Done.

+      {

[kzar, 2018/02/16 13:36:46]

Sorry for changing the indentation here, it makes the diff a bit messy. I
haven't changed anything else inside the function though... honest.

+        let callStack = new Error().stack;
 
-      if (typeof args[args.length - 1] == "function")
-        return func.apply(object, 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")
-        args.pop();
+        // 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();
 
-      let resolvePromise = null;
-      let rejectPromise = null;
+        let resolvePromise = null;
+        let rejectPromise = null;
 
-      func.call(object, ...args, result =>
-      {
-        let error = browser.runtime.lastError;
-        if (error && !portClosedBeforeResponseError.test(error.message))
+        func.call(object, ...args, result =>
         {
-          // runtime.lastError is already an Error instance on Edge, while on
-          // Chrome it is a plain object with only a message property.
-          if (!(error instanceof Error))
+          let error = browser.runtime.lastError;
+          if (error && !portClosedBeforeResponseError.test(error.message))
           {
-            error = new Error(error.message);
+            // runtime.lastError is already an Error instance on Edge, while on
+            // Chrome it is a plain object with only a message property.
+            if (!(error instanceof Error))
+            {
+              error = new Error(error.message);
+
+              // Add a more helpful stack trace.
+              error.stack = callStack;
+            }
 
-            // Add a more helpful stack trace.
-            error.stack = callStack;
+            rejectPromise(error);
           }
+          else
+          {
+            resolvePromise(result);
+          }
+        });
 
-          rejectPromise(error);
-        }
-        else
+        return new Promise((resolve, reject) =>
         {
-          resolvePromise(result);
-        }
-      });
-
-      return new Promise((resolve, reject) =>
-      {
-        resolvePromise = resolve;
-        rejectPromise = reject;
-      });
-    };
-
-    Object.defineProperty(object, name, descriptor);
+          resolvePromise = resolve;
+          rejectPromise = reject;
+        });
+      }
+    });
   }
 
   function wrapRuntimeOnMessage()