RESOLVED FIXED 94706
[V8] Move runScript() from V8Proxy to ScriptRunner 
https://bugs.webkit.org/show_bug.cgi?id=94706
Summary [V8] Move runScript() from V8Proxy to ScriptRunner
Kentaro Hara
Reported 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().
Attachments
Patch (8.80 KB, patch)
2012-08-22 07:05 PDT, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (12.49 KB, patch)
2012-08-22 18:13 PDT, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (12.71 KB, patch)
2012-08-22 18:16 PDT, Kentaro Hara 		abarth: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kentaro Hara
Comment 1 2012-08-22 07:05:50 PDT
Created attachment 159927 [details] Patch
Adam Barth
Comment 2 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.
Adam Barth
Comment 3 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.
Eric Seidel (no email)
Comment 4 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.
Kentaro Hara
Comment 5 2012-08-22 18:13:12 PDT
Created attachment 160057 [details] Patch
Kentaro Hara
Comment 6 2012-08-22 18:13:35 PDT
Put runScript() in ScriptRunner.h.
Kentaro Hara
Comment 7 2012-08-22 18:16:51 PDT
Created attachment 160058 [details] Patch
Adam Barth
Comment 8 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?
Kentaro Hara
Comment 9 2012-08-22 18:27:12 PDT
Committed r126377: <http://trac.webkit.org/changeset/126377>
Kentaro Hara
Comment 10 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.
Note You need to log in before you can comment on or make changes to this bug.