[webkit-reviews] review denied: [Bug 53003] Web Inspector: [JSC] implement setting breakpoints by line:column : [Attachment 144524] 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 53003: Web Inspector: [JSC] implement setting breakpoints by line:column
https://bugs.webkit.org/show_bug.cgi?id=53003

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

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


> Source/WebCore/bindings/js/ScriptDebugServer.cpp:95
> +    unsigned i;

Please move it to the for() initialization section:
  for (size_t i = 0; i < breaksCount; i++) {

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:135
> +	   if (breaksVector.at(i).columnNumber == (int)columnNumber) {

We should change types of ScriptBreakpoint.{lolumn,line}Number from int to
unsigned if they are always >= 0. Also please use static_cast<int> instead of
c-syle cast.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:163
> +    unsigned j;

Please declare the variables in the corresponding for() sections where they are
initialized.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:438
> +    const TextPosition position = m_currentCallFrame->position();

What's the point in extracting this into a local variable given that it is used
only once in the method?

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:446
> +    m_currentCallFrame->updatePosition(hitPosition);

You shouldn't need to update call frame position from the ScriptDebugServer, VM
should provide correct location.

> Source/WebCore/bindings/js/ScriptDebugServer.h:137
> +    typedef HashMap<Page*, ListenerSet*> PageListenersMap;

Revert this line.

> Source/WebCore/bindings/js/ScriptDebugServer.h:153
> +    mutable BreakpointsInLine m_lastHitScriptBreakpoints;

Why do you need these fields to be mutable?