[webkit-reviews] review denied: [Bug 89646] Web Inspector: Support separate script compilation and execution. : [Attachment 148767] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 89646: Web Inspector: Support separate script compilation and execution.
https://bugs.webkit.org/show_bug.cgi?id=89646

Attachment 148767: Patch
https://bugs.webkit.org/attachment.cgi?id=148767&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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)