[webkit-reviews] review denied: [Bug 21449] Support conditional breakpoints : [Attachment 38742] patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied apavlov at chromium.org's
request for review:
Bug 21449: Support conditional breakpoints
https://bugs.webkit.org/show_bug.cgi?id=21449

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

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
I'm going to review the back end code and leave out the front end changes for
now. I think we can have the back end land separately.


> +    LineToBreakpointMap* lineToInfos = m_breakpoints.get(sourceID);

lineToInfos is an odd name for this variable. Maybe just sourceBreakpoints.


> +    LineToBreakpointMap* lineToInfos = m_breakpoints.get(sourceID);

Ditto on lineToInfos.


> +	   return NULL;

We use 0 in WebKit for NULL.


> +    LineToBreakpointMap* lineToInfos = m_breakpoints.get(sourceID);

lineToInfos.


> +    if (exception) {
> +	   // An erroneous condition counts as "false".
> +	   return false;
> +    } else {
> +	   return
result.toBoolean(m_currentCallFrame->scopeChain()->globalObject()->globalExec()
);
> +    }

No need for the else since there is a return inside the if.


> +	   typedef HashMap<intptr_t, LineToBreakpointMap*> BreakpointsMap;

Maybe this should be called LineToBreakpointInfoMap?