Issue 6388 - Wrap inherited function properties as well

polyfill.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
@@ skipping 60 matching lines @@
       object = object[node];
 
       if (!object)
         return;
     }
 
     let func = object[name];
     if (!func)
       return;
 
-    // If the property is not writable assigning will fail, so we use
+    // 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 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.

+    // 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: 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.

+      value(...args)
       {

[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 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.
@@ skipping 129 matching lines @@
 
   // Workaround since HTMLCollection, NodeList, StyleSheetList, and CSSRuleList
   // didn't have iterator support before Chrome 51.
   // https://bugs.chromium.org/p/chromium/issues/detail?id=401699
   for (let object of [HTMLCollection, NodeList, StyleSheetList, CSSRuleList])
   {
     if (!(Symbol.iterator in object.prototype))
       object.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];
   }
 }