[webkit-reviews] review granted: [Bug 131578] Change callToJavaScript and callToNativeFunction so their callFrames match the native calling conventions : [Attachment 236667] Updated patch addressing prior concerns and with other changes from discussion with ggaren

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 131578: Change callToJavaScript and callToNativeFunction so their
callFrames match the native calling conventions
https://bugs.webkit.org/show_bug.cgi?id=131578

Attachment 236667: Updated patch addressing prior concerns and with other
changes from discussion with ggaren
https://bugs.webkit.org/attachment.cgi?id=236667&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236667&action=review


r=me with some naming fixes.

> Source/JavaScriptCore/jsc.cpp:696
>  EncodedJSValue JSC_HOST_CALL functionDumpCallFrame(ExecState* exec)
>  {
> -    if (!exec->callerFrame()->isVMEntrySentinel())
> -	   exec->vm().interpreter->dumpCallFrame(exec->callerFrame());
> +    VMEntryFrame* topVMEntryCallFrame = exec->vm().topVMEntryCallFrame;
> +    ExecState* callerFrame = exec->callerFrame(topVMEntryCallFrame);
> +    if (callerFrame)
> +	   exec->vm().interpreter->dumpCallFrame(callerFrame);
>      return JSValue::encode(jsUndefined());
>  }
>  #endif

It's weird that this function dumps exec's caller and not exec. That seems like
a bug. Oh well. Not new, I guess.

> Source/JavaScriptCore/debugger/Debugger.cpp:610
> +    VMEntryFrame* topVMEntryCallFrame = m_vm->topVMEntryCallFrame;

You should either name the class VMEntryCallFrame or name the variable and the
data member vmEntryFrame. It's not so good to name one "EntryFrame" and the
other "EntryCallFrame". That makes it sounds like they're subtly different
things.

I'd suggest the "EntryFrame" variant, since it is shorter, and we don't really
want this class to seem similar to the CallFrame class, since they are
different data structures.

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:45
> +    m_frame.m_callerIsVMEntry = false;

Should be "m_callerIsVMEntryFrame" to match the type name.

> Source/JavaScriptCore/llint/LLIntThunks.cpp:109
> +extern "C" VMEntryRecord* vmEntryRecordFromVMEntryFrame(VMEntryFrame*
entryFrame)

No need to put "FromVMEntryFrame" in the name here, since it's part of the
signature of the function.