Bug 120187 - Web Inspector: Breakpoints should have Automatically Continue Option
Summary: Web Inspector: Breakpoints should have Automatically Continue Option
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-08-22 17:57 PDT by Joseph Pecoraro
Modified: 2013-08-30 14:44 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] In Progress - Needs to Update LayoutTests (30.75 KB, patch)
2013-08-22 18:03 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Edit Popover with Option to Continue (43.54 KB, image/png)
2013-08-22 18:05 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (55.65 KB, patch)
2013-08-29 16:58 PDT, Joseph Pecoraro
timothy: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (801.87 KB, application/zip)
2013-08-29 19:53 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2013-08-22 17:57:40 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-08-22 17:58:27 PDT
<rdar://problem/14815088>
Comment 2 Joseph Pecoraro 2013-08-22 18:03:14 PDT
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.
Comment 3 Joseph Pecoraro 2013-08-22 18:05:58 PDT
Created attachment 209417 [details]
[IMAGE] Edit Popover with Option to Continue

This is what the in progress patch looks like.
Comment 4 Timothy Hatcher 2013-08-24 09:24:08 PDT
Comment on attachment 209415 [details]
[PATCH] In Progress - Needs to Update LayoutTests

Looking good!
Comment 5 Joseph Pecoraro 2013-08-29 16:58:23 PDT
Created attachment 210048 [details]
[PATCH] Proposed Fix
Comment 6 Build Bot 2013-08-29 19:53:57 PDT
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
Comment 7 Build Bot 2013-08-29 19:53:59 PDT
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
Comment 8 Timothy Hatcher 2013-08-30 10:39:29 PDT
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!
Comment 9 Joseph Pecoraro 2013-08-30 11:16:43 PDT
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.
Comment 10 Joseph Pecoraro 2013-08-30 14:44:29 PDT
Committed <http://trac.webkit.org/changeset/154910>.