[webkit-reviews] review granted: [Bug 232723] Array.prototype.toLocaleString does not respect deletion of Object.prototype.toLocaleString : [Attachment 448243] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <ashvayka at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 232723: Array.prototype.toLocaleString does not respect deletion of
Object.prototype.toLocaleString
https://bugs.webkit.org/show_bug.cgi?id=232723

Attachment 448243: Patch

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




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

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

Sweet, r=me with nits.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:696
> +    if (UNLIKELY(arguments.hasOverflowed())) {

Given the default inline capacity of 8, can this be simplified to
`ASSERT(!arguments.hasOverflowed())`?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:760
> +	   JSValue element = thisObject->get(globalObject, k);

I appreciate following the spec 1:1, but since we are already a bit off with
handling of 0th element, maybe we could do `thisObject->getIndex(globalObject,
k)` here?
Also, getIndex() is currently used in trunk.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:809
> +	   JSValue element = thisObject->get(globalObject, k);

Since we are changing this line, maybe we could do
`thisObject->getIndex(globalObject, k)`?