[webkit-reviews] review granted: [Bug 234604] Implement Change Array by copy proposal : [Attachment 456816] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <ashvayka at apple.com> has granted Aditi Singh
<asingh at igalia.com>'s request for review:
Bug 234604: Implement Change Array by copy proposal
https://bugs.webkit.org/show_bug.cgi?id=234604

Attachment 456816: Patch

https://bugs.webkit.org/attachment.cgi?id=456816&action=review




--- Comment #17 from Alexey Shvayka <ashvayka at apple.com> ---
Comment on attachment 456816
  --> https://bugs.webkit.org/attachment.cgi?id=456816
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456816&action=review

Great job, lazy function is correct, no worries on delay.

r=me with just 2 nits.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:155
> +	   Options::useChangeArrayByCopyMethods() ?
&vm.propertyNames->builtinNames().toSplicedPublicName() : nullptr,

"with" appears to be missing from unscopables, failing the tests.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1353
> +	   JSValue arraySort = jsCast<JSFunction*>(arrayPrototype()->get(this,
vm.propertyNames->builtinNames().sortPublicName()));

I understand it's a copy of what we have for "hasOwnProperty", but can we
please do something like

    JSFunction* arrayStore =
jsCast<JSFunction*>(arrayPrototype()->getDirect(this,
vm.propertyNames->builtinNames().sortPublicName()));

removing the RELEASE_ASSERT and extra cast below.