Bug 89646 - Web Inspector: Support separate script compilation and execution.
Summary: Web Inspector: Support separate script compilation and execution.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks: 89652
  Show dependency treegraph
 
Reported: 2012-06-21 03:42 PDT by Vsevolod Vlasov
Modified: 2012-06-22 02:40 PDT (History)
18 users (show)

See Also:


Attachments
Patch (24.96 KB, patch)
2012-06-21 03:55 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (24.98 KB, patch)
2012-06-21 04:53 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (28.92 KB, patch)
2012-06-21 07:54 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (28.85 KB, patch)
2012-06-21 08:00 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (27.96 KB, patch)
2012-06-21 10:45 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (36.43 KB, patch)
2012-06-21 12:06 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (for landing) (36.66 KB, patch)
2012-06-22 02:06 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2012-06-21 03:42:45 PDT
Separate script compilation and execution will be used for implementing snippets support: we want to have Script object (scriptId) available at the moment of script execution.
Also script compilation can be used for error highlighting.
Comment 1 Vsevolod Vlasov 2012-06-21 03:55:28 PDT
Created attachment 148761 [details]
Patch
Comment 2 Early Warning System Bot 2012-06-21 04:11:54 PDT
Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13036071
Comment 3 Early Warning System Bot 2012-06-21 04:17:49 PDT
Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13031063
Comment 4 Gyuyoung Kim 2012-06-21 04:22:18 PDT
Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13029075
Comment 5 Build Bot 2012-06-21 04:46:01 PDT
Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13037073
Comment 6 Vsevolod Vlasov 2012-06-21 04:53:04 PDT
Created attachment 148767 [details]
Patch
Comment 7 Build Bot 2012-06-21 05:19:26 PDT
Comment on attachment 148767 [details]
Patch

Attachment 148767 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13029091
Comment 8 Yury Semikhatsky 2012-06-21 05:42:58 PDT
Comment on attachment 148767 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:448
> +    OwnPtr<OwnHandle<v8::Script> > scriptPtr = adoptPtr(new OwnHandle<v8::Script>(script));

Maybe inline this statement as a parameter below?

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:462
> +    v8::Handle<v8::Script> script = scriptOwnHandle->get();

This should be a local handle(and you should create a HandleScope here), otherwise next line may release the only handle to the script and the script may be GC'ed, r- for this.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:470
> +    Frame* frame = scriptExecutionContext && scriptExecutionContext->isDocument() ? static_cast<Document*>(scriptExecutionContext)->frame() : 0;

You can move this part to the PageScriptDebugServer where scriptExecutionContext should always be a Document.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:479
> +    v8::Handle<v8::Context> context = state->context();

Alternatively you could use ScriptScope that would create, HandleScope and context scope and also provide you with a TryCatch block.

> Source/WebCore/inspector/Inspector.json:2556
> +                    { "name": "contextId", "type": "integer", "optional": true, "description": "Specifies isolated context to compile in. Each content script lives in an isolated context and this parameter may be used to specify one of those contexts. If the parameter is omitted or 0 the compilation will be performed in the context of the inspected page." },

contextId type should be ExecutionContextId

> Source/WebCore/inspector/Inspector.json:2571
> +                    { "name": "contextId", "type": "integer", "optional": true, "description": "Specifies in which isolated context to perform script run. Each content script lives in an isolated context and this parameter may be used to specify on of those contexts. If the parameter is omitted or 0 the evaluation will be performed in the context of the inspected page." },

Passing contextId second time is redundant since it can be derived from scriptId.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:551
> +    if (doNotPauseOnExceptionsAndMuteConsole ? *doNotPauseOnExceptionsAndMuteConsole : false) {

if (doNotPauseOnExceptionsAndMuteConsole && *doNotPauseOnExceptionsAndMuteConsole)

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:570
> +    if (doNotPauseOnExceptionsAndMuteConsole ? *doNotPauseOnExceptionsAndMuteConsole : false) {

if (doNotPauseOnExceptionsAndMuteConsole && *doNotPauseOnExceptionsAndMuteConsole)
Comment 9 Vsevolod Vlasov 2012-06-21 07:54:33 PDT
Created attachment 148802 [details]
Patch
Comment 10 Vsevolod Vlasov 2012-06-21 07:59:57 PDT
(In reply to comment #8)
> (From update of attachment 148767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148767&action=review
> 
> > Source/WebCore/bindings/v8/ScriptDebugServer.cpp:448
> > +    OwnPtr<OwnHandle<v8::Script> > scriptPtr = adoptPtr(new OwnHandle<v8::Script>(script));
> 
> Maybe inline this statement as a parameter below?
Done.

> > Source/WebCore/bindings/v8/ScriptDebugServer.cpp:462
> > +    v8::Handle<v8::Script> script = scriptOwnHandle->get();
> 
> This should be a local handle(and you should create a HandleScope here), otherwise next line may release the only handle to the script and the script may be GC'ed, r- for this.
Done.

> > Source/WebCore/bindings/v8/ScriptDebugServer.cpp:470
> > +    Frame* frame = scriptExecutionContext && scriptExecutionContext->isDocument() ? static_cast<Document*>(scriptExecutionContext)->frame() : 0;
> You can move this part to the PageScriptDebugServer where scriptExecutionContext should always be a Document.
Moved all frame related code to the PageScriptDebugServer overriding methods.

> > Source/WebCore/bindings/v8/ScriptDebugServer.cpp:479
> > +    v8::Handle<v8::Context> context = state->context();
> 
> Alternatively you could use ScriptScope that would create, HandleScope and context scope and also provide you with a TryCatch block.
ScriptScope does not expose TryCatch publicly.

> > Source/WebCore/inspector/Inspector.json:2556
> > +                    { "name": "contextId", "type": "integer", "optional": true, "description": "Specifies isolated context to compile in. Each content script lives in an isolated context and this parameter may be used to specify one of those contexts. If the parameter is omitted or 0 the compilation will be performed in the context of the inspected page." },
> contextId type should be ExecutionContextId
Done here and in Runtime.evaluate.

> > Source/WebCore/inspector/Inspector.json:2571
> > +                    { "name": "contextId", "type": "integer", "optional": true, "description": "Specifies in which isolated context to perform script run. Each content script lives in an isolated context and this parameter may be used to specify on of those contexts. If the parameter is omitted or 0 the evaluation will be performed in the context of the inspected page." },
> 
> Passing contextId second time is redundant since it can be derived from scriptId.
I am now using Script::New to compile script context-independently, so contextId is only passed to runScript method.

> > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:551
> > +    if (doNotPauseOnExceptionsAndMuteConsole ? *doNotPauseOnExceptionsAndMuteConsole : false) {
> 
> if (doNotPauseOnExceptionsAndMuteConsole && *doNotPauseOnExceptionsAndMuteConsole)
Done.
Comment 11 Vsevolod Vlasov 2012-06-21 08:00:52 PDT
Created attachment 148805 [details]
Patch
Comment 12 Yury Semikhatsky 2012-06-21 08:28:16 PDT
Comment on attachment 148805 [details]
Patch

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

> Source/WebCore/bindings/v8/PageScriptDebugServer.cpp:155
> +    m_compiledScriptURLs.remove(scriptId);

String sourceURL = m_compiledScriptURLs.take(scriptId)
Comment 13 Pavel Feldman 2012-06-21 08:39:17 PDT
Comment on attachment 148805 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:2557
> +                    { "name": "objectGroup", "type": "string", "optional": true, "description": "Symbolic group name that can be used to release multiple objects." },

You don't need objectGroup when compiling.

> Source/WebCore/inspector/Inspector.json:2562
> +                    { "name": "exception", "$ref": "Runtime.RemoteObject", "optional": true, "description": "Object wrapper for compilation exception if there was one." }

This should be returned as a protocol error string instead.

> Source/WebCore/inspector/Inspector.json:2564
> +                "description": "Compiles expression in a given context."

There is no context.

> Source/WebCore/inspector/Inspector.json:2571
> +                    { "name": "scriptId", "$ref": "ScriptId", "description": "Id of the script to run." },

open topic, nit: I wonder if script ids should be actually shared between debugger and this. What if user runs existing script again?
Comment 14 Vsevolod Vlasov 2012-06-21 10:45:46 PDT
Created attachment 148835 [details]
Patch
Comment 15 Vsevolod Vlasov 2012-06-21 10:47:22 PDT
(In reply to comment #13)
> (From update of attachment 148805 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148805&action=review
> 
> > Source/WebCore/inspector/Inspector.json:2557
> > +                    { "name": "objectGroup", "type": "string", "optional": true, "description": "Symbolic group name that can be used to release multiple objects." },
> 
> You don't need objectGroup when compiling.
That was needed for an exception object, removed.

> > Source/WebCore/inspector/Inspector.json:2562
> > +                    { "name": "exception", "$ref": "Runtime.RemoteObject", "optional": true, "description": "Object wrapper for compilation exception if there was one." }
> 
> This should be returned as a protocol error string instead.
Done.

> > Source/WebCore/inspector/Inspector.json:2564
> > +                "description": "Compiles expression in a given context."
> 
> There is no context.
Fixed.

> > Source/WebCore/inspector/Inspector.json:2571
> > +                    { "name": "scriptId", "$ref": "ScriptId", "description": "Id of the script to run." },
> 
> open topic, nit: I wonder if script ids should be actually shared between debugger and this. What if user runs existing script again?
User can run only script that was compiled using compileScript. With that I don't think we need to introduce another kind of identifier.
Comment 16 Pavel Feldman 2012-06-21 11:06:54 PDT
Comment on attachment 148835 [details]
Patch

Could you please cover the new API with the test?
Comment 17 Vsevolod Vlasov 2012-06-21 12:06:47 PDT
Created attachment 148858 [details]
Patch
Comment 18 Build Bot 2012-06-21 15:37:00 PDT
Comment on attachment 148858 [details]
Patch

Attachment 148858 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13032250
Comment 19 Vsevolod Vlasov 2012-06-22 02:06:28 PDT
Created attachment 148979 [details]
Patch (for landing)
Comment 20 Vsevolod Vlasov 2012-06-22 02:40:11 PDT
Committed r121014: <http://trac.webkit.org/changeset/121014>