[webkit-reviews] review denied: [Bug 53003] Web Inspector: [JSC] implement setting breakpoints by line:column : [Attachment 143987] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Peter Wang
<peter.wang at torchmobile.com.cn>'s request for review:
Bug 53003: Web Inspector: [JSC] implement setting breakpoints by line:column
https://bugs.webkit.org/show_bug.cgi?id=53003

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=143987&action=review


Looks good, can be improved some more.

> Source/WebCore/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=53003

Better add an empty line here.

> Source/WebCore/ChangeLog:5
> +	   The RIM PR https://bugzilla.qnx.com/bugzilla/show_bug.cgi?id=154675
is depend on this bug.

This needs to go below Reviewed by. Also please add an empty line after
Reviewed by.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:75
> +	   return true;

I think you can combine below so you only have two returns.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:525
> +    m_lastHitScriptBreakpoints.clear();

You are doing same lines of code as in returnEvent. So I think there can be a
helper function to avoid duplicating code.