[webkit-reviews] review granted: [Bug 94456] [V8] Move instrumentedCallFunction() from V8Proxy to ScriptController : [Attachment 159372] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 94456: [V8] Move instrumentedCallFunction() from V8Proxy to
ScriptController
https://bugs.webkit.org/show_bug.cgi?id=94456

Attachment 159372: Patch
https://bugs.webkit.org/attachment.cgi?id=159372&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=159372&action=review


> Source/WebCore/bindings/v8/ScriptController.cpp:226
> +#if PLATFORM(CHROMIUM)

Are these ifdefs needed?  I would have expected them to be inside TRACE_EVENT1,
but I haven't actually checked.

> Source/WebCore/bindings/v8/ScriptController.h:81
> +    static v8::Local<v8::Value> callFunctionWithInstrumentation(Frame*,
v8::Handle<v8::Function>, v8::Handle<v8::Object> receiver, int argc,
v8::Handle<v8::Value> args[]);

Why make this a static function that takes a Frame* as an argument?  I guess so
it can be null?  This function might make more sense as a member function.

> Source/WebCore/bindings/v8/ScriptFunctionCall.cpp:205
> -    v8::Handle<v8::Value> result = V8Proxy::instrumentedCallFunction(0 /*
frame */, function, object, m_arguments.size(), args.get());
> +    v8::Handle<v8::Value> result =
ScriptController::callFunctionWithInstrumentation(0 /* frame */, function,
object, m_arguments.size(), args.get());

I see.	Frame can be 0...

> Source/WebCore/bindings/v8/V8Callback.cpp:63
>      Frame* frame = scriptExecutionContext &&
scriptExecutionContext->isDocument() ?
static_cast<Document*>(scriptExecutionContext)->frame() : 0;
> -    v8::Handle<v8::Value> result = V8Proxy::instrumentedCallFunction(frame,
callbackFunction, thisObject, argc, argv);
> +    v8::Handle<v8::Value> result =
ScriptController::callFunctionWithInstrumentation(frame, callbackFunction,
thisObject, argc, argv);

Maybe we should try passing a ScriptExecutionContext directly?	This code that
goes from ScriptExecutionContext to Frame just so that
callFunctionWithInstrumentation can go back to ScriptExecutionContext seems
less than ideal.