[webkit-reviews] review granted: [Bug 183429] [JSC] Add inherits<T>(VM&) leveraging JSCast fast path : [Attachment 335287] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 183429: [JSC] Add inherits<T>(VM&) leveraging JSCast fast path
https://bugs.webkit.org/show_bug.cgi?id=183429

Attachment 335287: Patch

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




--- Comment #15 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 335287
  --> https://bugs.webkit.org/attachment.cgi?id=335287
Patch

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

r=me.  Nice work.

> Source/JavaScriptCore/runtime/JSType.h:121
> +static const uint32_t LastObjectType = MaxJSType;

I think you should put your ChangeLog comment near here to explain why you use
MaxJSType and not the last type in the enum list.  BTW, do we still need
LastJSCObjectType?  Are the other uses of LastJSCObjectType correct (in light
of the bug you found)?

> Source/WebCore/bindings/js/JSEventTargetCustom.cpp:60
> +    if (value.inherits<JS##interfaceName>(vm))		       \

nit: the amount of whitespace at the end seems arbitrary.  Might as well trim
it so that the \ is aligned with the line above, or just 1 space after the end
of this line.