[webkit-reviews] review denied: [Bug 99801] Web Inspector: [JSC] implement WorkerScriptDebugServer : [Attachment 169578] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Peter Wang
<peter.wang at torchmobile.com.cn>'s request for review:
Bug 99801: Web Inspector: [JSC] implement WorkerScriptDebugServer
https://bugs.webkit.org/show_bug.cgi?id=99801

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

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=169578&action=review


Inspector part of the change looks good to me except a few minor things. I am
pretty sure we should be able to treat page and worker debuggers similarly in
Source/JavaScriptCore/debugger but I'm not an expert in JSC so you need someone
working on JSC to review that part.

> Source/JavaScriptCore/parser/Parser.h:998
> +    if (debugger && !debugger->isWorkerDebugger() &&
!ParsedNode::scopeIsFunction)

I still don't understand why we should treat parse events differently in case
of workers. We need someone familiar with JSC code to look at this part.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:370
> +	   dispatchDidParseSource(*listeners, sourceProvider,
isWorkerDebugger() ? false : isContentScript(exec));

Please make isContentScript a virtual method on ScriptDebugServer and provide
appropriate implementations for Page/WorkerScriptDebug instead of introducing
isWorkerDebugger.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:58
> +    // If JavaScript stack is not empty postpone recompilation.

It seems that we can safely assume the JS stack is empty if
recompileAllJSFunctions is invoked by Timer but this is a different issue.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:73
> +    recompileAllJSFunctions(0);

recompileAllJSFunctionsSoon