Bug 120187 – Web Inspector: Breakpoints should have Automatically Continue Option

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

See Xcode. Breakpoints should be able to automatically continue after they are hit.

This mostly only makes sense after we get Breakpoint Actions, which I will work on next.

[Radar WebKit Bug Importer]

<rdar://problem/14815088>

[Joseph Pecoraro]

Created attachment 209415 [details]
[PATCH] In Progress - Needs to Update LayoutTests

This is a first draft. Seems to work pretty well, and the next step is adding BreakpointActions, which I'll do in a follow-up patch.

Not marking this for review yet, because I didn't update and write new tests yet.

[Joseph Pecoraro]

Created attachment 209417 [details]
[IMAGE] Edit Popover with Option to Continue

This is what the in progress patch looks like.

[Timothy Hatcher]

Comment on attachment 209415 [details]
[PATCH] In Progress - Needs to Update LayoutTests

Looking good!

[Joseph Pecoraro]

Created attachment 210048 [details]
[PATCH] Proposed Fix

[Build Bot]

Comment on attachment 210048 [details]
[PATCH] Proposed Fix

Attachment 210048 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1638369

New failing tests:
inspector-protocol/debugger/setBreakpoint-column.html

[Build Bot]

Created attachment 210061 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Timothy Hatcher]

Comment on attachment 210048 [details]
[PATCH] Proposed Fix

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!

[Joseph Pecoraro]

Comment on attachment 210048 [details]
[PATCH] Proposed Fix

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…)

Yeah, a newline/indent really breaks up the readability of the test.

After thinking about this, we can make InspectorTest.checkForError throw an exception, which would stop further execution. I'll give that a shot!

>> Source/WebCore/bindings/js/ScriptDebugServer.cpp:171
>>          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.

I think this is fine for now because I suspect all of this code to start going through some changes soon. For now hasBreakpoint returns true/false if the breakpoint was actually triggered or not (including checking the condition), but will always send out the breakpoint that was considered, success or not.

[Joseph Pecoraro]

Committed <http://trac.webkit.org/changeset/154910>.