[webkit-reviews] review denied: [Bug 93519] Web Inspector: implement WorkerScriptDebugServer : [Attachment 161450] 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 93519: Web Inspector: implement WorkerScriptDebugServer
https://bugs.webkit.org/show_bug.cgi?id=93519

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

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


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

How does inspector get notified about parsed scripts in case of worker if is is
skipped here?

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:447
> +void ScriptDebugServer::blockToWaitInstruction(bool& stop)

I'd call it runEventLoopWhilePaused

> Source/WebCore/bindings/js/ScriptDebugServer.h:117
> +    virtual void blockToWaitInstruction(bool& stop);

m_doneProcessingDebuggerEvents is a protected field, no need to pass a
reference to it as an argument.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:71
> +    m_workerContext->script()->attachDebugger(this);

This should happen only when first listener is added. We should probably
simplify the API as I am not aware of any cases when we may have more than one
listener.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:81
> +    m_workerContext->script()->detachDebugger(this);

This should only occur when there are no listeners.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.h:48
> +    bool isWorkerDebugger() { return true; }

This method should be marked virtual.