[webkit-reviews] review denied: [Bug 43332] [JSC] Web Inspector: implement breaking from native callback : [Attachment 145039] 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 43332: [JSC] Web Inspector: implement breaking from native callback
https://bugs.webkit.org/show_bug.cgi?id=43332

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

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


> Source/WebCore/ChangeLog:10
> +	   No test. No impact on others interface of JSC.

There is a bunch of layout tests in LayouTests/inspector/debugger disabled for
JSC because of this bug. You should enable them if the functionality is now
fixed.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:158
> +    m_paused = true;

This is not what we expect this method to do, you should run message loop here
until the execution is resumed like in case if a regular JS breakpoint.
Probably the following code will work:

m_pauseOnNextStatement = true;
pauseIfNeeded(m_currentCallFrame->dynamicGlobalObject());

> Source/WebCore/bindings/js/ScriptDebugServer.h:88
> +    bool supportsNativeBreakpoints() { return true; }

This method now can be removed from both
Source/WebCore/bindings/{js,v8}/ScriptDebugServer.h as it returns the same
value.