[webkit-reviews] review granted: [Bug 142933] Redefining a property should not change its insertion index (Object.keys order) : [Attachment 403934] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 142933: Redefining a property should not change its insertion index
(Object.keys order)
https://bugs.webkit.org/show_bug.cgi?id=142933

Attachment 403934: Patch

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




--- Comment #24 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 403934
  --> https://bugs.webkit.org/attachment.cgi?id=403934
Patch

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

r=me with one comment/question

> Source/JavaScriptCore/ChangeLog:61
> -	   beind redefined with PropertyAttribute::None.
> +	   being redefined with PropertyAttribute::None.

no need to fix this now IMO. We usually don't go back and fix ChangeLog typos
afaik.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:317
> +	       if (mode == PutModeDefineOwnProperty && (attributes !=
currentAttributes || (attributes &
PropertyAttribute::AccessorOrCustomAccessorOrValue)))

same comment here. I think we should be ok with just CustomAccessorOrValue. Do
tests failing just using CustomAccessorOrValue? If so, we should find out why.