[webkit-reviews] review granted: [Bug 120576] Web Inspector: Breakpoint Actions : [Attachment 210234] [PATCH] 1 - Backend and Protocol Tests

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 120576: Web Inspector: Breakpoint Actions
https://bugs.webkit.org/show_bug.cgi?id=120576

Attachment 210234: [PATCH] 1 - Backend and Protocol Tests
https://bugs.webkit.org/attachment.cgi?id=210234&action=review

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


> Source/WebCore/bindings/js/ScriptDebugServer.cpp:205
> +    case ScriptBreakpointActionTypeSound: {
> +	   systemBeep();
> +	   break;
> +    }

No need for the extra braces here.

>> Source/WebCore/bindings/js/ScriptDebugServer.cpp:445
>> +	// ASSERT(m_currentCallFrame);
> 
> So, I have to debug this. It happens almost 100% running
LayoutTest/inspector-protocol/debugger/setBreakpoint-options-exception.html.
But things seem to work as expected.

Perhaps the nested evaluate or reportException calls are confusing the
debugger?

> Source/WebCore/inspector/Inspector.json:2667
> +		       { "name": "type", "type": "string", "enum": ["log",
"eval", "sound"], "description": "Different kinds of breakpoint actions." },

"evaluate"? It would match the others better and other protocol methods that
spell it out.