Bug 94706 - [V8] Move runScript() from V8Proxy to ScriptRunner
Summary: [V8] Move runScript() from V8Proxy to ScriptRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 85330
  Show dependency treegraph
 
Reported: 2012-08-22 07:01 PDT by Kentaro Hara
Modified: 2012-08-22 18:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.80 KB, patch)
2012-08-22 07:05 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (12.49 KB, patch)
2012-08-22 18:13 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (12.71 KB, patch)
2012-08-22 18:16 PDT, Kentaro Hara
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-08-22 07:01:09 PDT
To kill V8Proxy, we can move runScript() from V8Proxy to ScriptSourceCode.

- ScriptSourceCode::runScript() should be a static method. It should receive ScriptExecutionContext on which the script is evaluated.
- After this patch is landed, I'll remove WorkerContextExecutionContext::runScript() and ScriptDebugServer::runScript().
Comment 1 Kentaro Hara 2012-08-22 07:05:50 PDT
Created attachment 159927 [details]
Patch
Comment 2 Adam Barth 2012-08-22 12:22:02 PDT
Comment on attachment 159927 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptSourceCode.cpp:45
> +v8::Local<v8::Value> ScriptSourceCode::runScript(v8::Handle<v8::Script> script, ScriptExecutionContext* context)

Hum...  I think of ScriptSourceCode has being a model class for the code.  This feels more like something that should go in a controller class, like ScriptController since we're instructing V8 to do something (i.e., execute script).

Is this function called on multiple threads?  If it's only the main thread, I'd put it on ScriptController and try to use m_frame->document() rather than passing in a ScriptExecutionContext* function.  If this is something we call from worker threads as well, we might need to introduce a new header, something like V8Executive, that we use for actually executing code.
Comment 3 Adam Barth 2012-08-22 12:22:55 PDT
I'm going to give this some more thought after I make it through some other tasks for today.
Comment 4 Eric Seidel (no email) 2012-08-22 13:24:21 PDT
Comment on attachment 159927 [details]
Patch

I like the idea, but I agree with dr. barth that this is the wrong place to put this.
Comment 5 Kentaro Hara 2012-08-22 18:13:12 PDT
Created attachment 160057 [details]
Patch
Comment 6 Kentaro Hara 2012-08-22 18:13:35 PDT
Put runScript() in ScriptRunner.h.
Comment 7 Kentaro Hara 2012-08-22 18:16:51 PDT
Created attachment 160058 [details]
Patch
Comment 8 Adam Barth 2012-08-22 18:20:56 PDT
Comment on attachment 160058 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptRunner.h:38
> +    // Run an already compiled script.
> +    static v8::Local<v8::Value> runScript(v8::Handle<v8::Script>, ScriptExecutionContext*);

runScript -> runCompiledScript and delete the comment?
Comment 9 Kentaro Hara 2012-08-22 18:27:12 PDT
Committed r126377: <http://trac.webkit.org/changeset/126377>
Comment 10 Kentaro Hara 2012-08-22 18:27:41 PDT
(In reply to comment #8)
> > Source/WebCore/bindings/v8/ScriptRunner.h:38
> > +    // Run an already compiled script.
> > +    static v8::Local<v8::Value> runScript(v8::Handle<v8::Script>, ScriptExecutionContext*);
> 
> runScript -> runCompiledScript and delete the comment?

Done. Thanks.