[webkit-reviews] review granted: [Bug 231202] [JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared : [Attachment 440147] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 231202: [JSC] JSPropertyNameEnumerator should not have cached prototype
chain since empty JSPropertyNameEnumerator is shared
https://bugs.webkit.org/show_bug.cgi?id=231202

Attachment 440147: Patch

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




--- Comment #8 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 440147
  --> https://bugs.webkit.org/attachment.cgi?id=440147
Patch

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

r=me with some nits

> Source/JavaScriptCore/ChangeLog:11
> +	   invariant was broken since we now have shared empty
JSPropertyNameEnumerator, which can

Nit: shared empty => shared empty sentinel. Might be a bit clearer.

> Source/JavaScriptCore/ChangeLog:27
> +	   While reviewing the code, we also found that watchpoint-based
validation didn't care PolyProto.

Nit: care => care about?

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:107
> +    // JSPropertyNameEnumerator is immutable data structure, which allows VM
to cache empty one.

Typo: cache empty one => cache the empty one.

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:131
> +	       if (!(enumeratorAndFlag &
StructureRareData::cachedPropertyNameEnumeratorIsValidatedViaTraversingFlag))

Nit: Maybe use OptionSet I personally prefer that but we use this idiom
everywhere so idc too much.