Bug 94456

Summary: [V8] Move instrumentedCallFunction() from V8Proxy to ScriptController
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 85330    
Attachments:
Description Flags
Patch none

Description Kentaro Hara 2012-08-20 02:00:18 PDT
To kill V8Proxy, we can move instrumentedCallFunction() from V8Proxy to ScriptController. Also we can rename instrumentedCallFunction() to callFunctionWithInstrumentation(), for consistency with callFunction().
Comment 1 Kentaro Hara 2012-08-20 02:16:21 PDT
Created attachment 159372 [details]
Patch
Comment 2 Adam Barth 2012-08-20 11:26:24 PDT
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.
Comment 3 Kentaro Hara 2012-08-20 17:50:13 PDT
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.
Comment 4 Kentaro Hara 2012-08-20 17:53:13 PDT
Committed r126109: <http://trac.webkit.org/changeset/126109>