Issue 4353 - Remove deprecated __proto__ syntax

Issue 4353 - Remove deprecated __proto__ syntax

[kzar, 2016-09-01 23:46:06]

Patch Set 1

[kzar, 2016-09-01 23:52:54]

Patch Set 2 : Missed a file...

[Wladimir Palant, 2016-09-02 08:17:13]

https://codereview.adblockplus.org/29350382/diff/29350393/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29350382/diff/29350393/lib/coreUtils.js#ne...
lib/coreUtils.js:27: value["configurable"] = value["enumerable"] = true;
How about just calling Object.getOwnPropertyDescriptor(properties, property)?
Then you won't need to change the way getters and setters are defined in the
source code. Also, please create a new object for the return value rather than
messing with the parameter.

[kzar, 2016-09-02 11:53:21]

Patch Set 3 : Make use of Object.getOwnPropertyDescriptor

https://codereview.adblockplus.org/29350382/diff/29350393/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29350382/diff/29350393/lib/coreUtils.js#ne...
lib/coreUtils.js:27: value["configurable"] = value["enumerable"] = true;
On 2016/09/02 08:17:13, Wladimir Palant wrote:
> How about just calling Object.getOwnPropertyDescriptor(properties, property)?
> Then you won't need to change the way getters and setters are defined in the
> source code. Also, please create a new object for the return value rather than
> messing with the parameter.

Wow, not sure why I didn't think of that! Done.

[Wladimir Palant, 2016-09-02 12:19:48]

https://codereview.adblockplus.org/29350382/diff/29350436/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29350382/diff/29350436/lib/coreUtils.js#ne...
lib/coreUtils.js:18: function desc(properties)
Since we are only using this for inheritance, how about also exporting:

  function extend(cls, properties)
  {
    return Object.create(cls.prototype, desc(properties));
  }

This should make the callers easier to understand. I would normally suggest
handling the constructors here as well but I realized that this would degrade
performance.

https://codereview.adblockplus.org/29350382/diff/29350436/lib/coreUtils.js#ne...
lib/coreUtils.js:23: descriptor[key] =
Object.getOwnPropertyDescriptor(properties, key);
This will fail should we ever hit a property defined on the prototype of
`properties` (Object.getOwnPropertyDescriptor returns undefined, Object.create
throws). I wonder whether the loop should be `for (let key of
Object.keys(properties))` - or maybe just check the return value of
Object.getOwnPropertyDescriptor.

[kzar, 2016-09-02 15:06:41]

Patch Set 4 : Export extend function too, use Object.keys

https://codereview.adblockplus.org/29350382/diff/29350436/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29350382/diff/29350436/lib/coreUtils.js#ne...
lib/coreUtils.js:18: function desc(properties)
On 2016/09/02 12:19:48, Wladimir Palant wrote:
> Since we are only using this for inheritance, how about also exporting:
> 
>   function extend(cls, properties)
>   {
>     return Object.create(cls.prototype, desc(properties));
>   }
> 
> This should make the callers easier to understand. I would normally suggest
> handling the constructors here as well but I realized that this would degrade
> performance.

Done.

https://codereview.adblockplus.org/29350382/diff/29350436/lib/coreUtils.js#ne...
lib/coreUtils.js:23: descriptor[key] =
Object.getOwnPropertyDescriptor(properties, key);
On 2016/09/02 12:19:48, Wladimir Palant wrote:
> This will fail should we ever hit a property defined on the prototype of
> `properties` (Object.getOwnPropertyDescriptor returns undefined, Object.create
> throws). I wonder whether the loop should be `for (let key of
> Object.keys(properties))` - or maybe just check the return value of
> Object.getOwnPropertyDescriptor.

Done.

[Wladimir Palant, 2016-09-02 20:43:28]

LGTM