Bug 94456 – [V8] Move instrumentedCallFunction() from V8Proxy to ScriptController

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

To kill V8Proxy, we can move instrumentedCallFunction() from V8Proxy to ScriptController. Also we can rename instrumentedCallFunction() to callFunctionWithInstrumentation(), for consistency with callFunction().

[Kentaro Hara]

Created attachment 159372 [details]
Patch

[Adam Barth]

Comment on attachment 159372 [details]
Patch

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.

[Kentaro Hara]

Comment on attachment 159372 [details]
Patch

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.

Right. Removed.

>> Source/WebCore/bindings/v8/V8Callback.cpp:63
>> +    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.

Done.

[Kentaro Hara]

Committed r126109: <http://trac.webkit.org/changeset/126109>