[webkit-reviews] review granted: [Bug 120187] Web Inspector: Breakpoints should have Automatically Continue Option : [Attachment 210048] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 120187: Web Inspector: Breakpoints should have Automatically Continue
Option
https://bugs.webkit.org/show_bug.cgi?id=120187

Attachment 210048: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=210048&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210048&action=review


> LayoutTests/inspector-protocol/debugger/removeBreakpoint.html:22
> +		   if (InspectorTest.checkForError(responseObject)) return;

I know it is just a test, but this doesn't match WebKit style and is weird for
me to read now! There are a few places you do this. (Even though it was my
former style 8 years ago…)

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:171
>      // An empty condition counts as no condition which is equivalent to
"true".
>      if (breaksVector.at(i).condition.isEmpty())
>	   return true;

Maybe this should move to the caller now that you pass out the breakpoint? Or
maybe it should move above the new code and not be considered as hitting the
breakpoint if the condition is false.

> Source/WebInspectorUI/UserInterface/Breakpoint.js:287
> +	       var optionsRow =
table.appendChild(document.createElement("tr"));
> +	       var optionsHeader =
optionsRow.appendChild(document.createElement("th"));
> +	       var optionsData =
optionsRow.appendChild(document.createElement("td"));

It is 2013! <td>! <th>! <tr>! Heh. No worries.

> Source/WebInspectorUI/UserInterface/InspectorBackend.js:79
> +	   agent[domainAndMethod[1]]["supports"] = this._supports.bind(this,
method, signature);

Sweet!