Bug 89646 – Web Inspector: Support separate script compilation and execution.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

Created attachment 148761 [details]
Patch

[Early Warning System Bot]

Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13036071

[Early Warning System Bot]

Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13031063

[Gyuyoung Kim]

Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13029075

[Build Bot]

Comment on attachment 148761 [details]
Patch

Attachment 148761 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13037073

[Vsevolod Vlasov]

Created attachment 148767 [details]
Patch

[Build Bot]

Comment on attachment 148767 [details]
Patch

Attachment 148767 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13029091

[Yury Semikhatsky]

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)

[Vsevolod Vlasov]

Created attachment 148802 [details]
Patch

[Vsevolod Vlasov]

(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.

[Vsevolod Vlasov]

Created attachment 148805 [details]
Patch

[Yury Semikhatsky]

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)

[Pavel Feldman]

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?

[Vsevolod Vlasov]

Created attachment 148835 [details]
Patch

[Vsevolod Vlasov]

(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.

[Pavel Feldman]

Comment on attachment 148835 [details]
Patch

Could you please cover the new API with the test?

[Vsevolod Vlasov]

Created attachment 148858 [details]
Patch

[Build Bot]

Comment on attachment 148858 [details]
Patch

Attachment 148858 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13032250

[Vsevolod Vlasov]

Created attachment 148979 [details]
Patch (for landing)

[Vsevolod Vlasov]

Committed r121014: <http://trac.webkit.org/changeset/121014>