[webkit-reviews] review granted: [Bug 215572] Introduce OpIsCallable bytecode and intrinsic : [Attachment 406719] Patch

[bugzilla-daemon at webkit.org]

Ross Kirsling <ross.kirsling at sony.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 215572: Introduce OpIsCallable bytecode and intrinsic
https://bugs.webkit.org/show_bug.cgi?id=215572

Attachment 406719: Patch

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




--- Comment #2 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 406719
  --> https://bugs.webkit.org/attachment.cgi?id=406719
Patch

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

Looks good from my perspective.

> Source/JavaScriptCore/ChangeLog:12
> +	   1. Aligns slow_path_is_function with DFG/FTL implementations by
introducing
> +	      jsTypeofIsFunction() helper. This fixes `typeof document.all ===
"function"`
> +	      to return `true` instead of `false`.

I think you have the operator swapped here -- should be !==, right?

> Source/JavaScriptCore/runtime/Operations.h:49
> +	   if (object->type() == JSFunctionType)
> +	       return true;
> +	   if (object->structure(vm)->masqueradesAsUndefined(globalObject))
> +	       return false;
> +	   if (object->isCallable(vm))
> +	       return true;

isCallable starts by checking JSFunctionType, right? So I think we could drop
the first conditional unless there's a perf implication.

> LayoutTests/js/dom/script-tests/document-all-typeof-is-function-fold.js:4
> +function testTypeofIsFuction() {

nit: Fuction -> Function (x2)