Bug 21449 - Support conditional breakpoints
Summary: Support conditional breakpoints
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
Depends on: 28846 28908
  Show dependency treegraph
Reported: 2008-10-07 15:01 PDT by Timothy Hatcher
Modified: 2009-11-02 05:06 PST (History)
7 users (show)

See Also:

Condition editor + conditional breakpoint screenshot (37.01 KB, image/png)
2009-08-28 08:33 PDT, Alexander Pavlov (apavlov)
no flags Details
patch (87.57 KB, patch)
2009-08-28 09:53 PDT, Alexander Pavlov (apavlov)
timothy: review-
Details | Formatted Diff | Diff
Drosera's Breakpoint Editor (18.81 KB, image/png)
2009-08-28 13:32 PDT, Timothy Hatcher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2008-10-07 15:01:34 PDT
Drosera had conditional breakpoints, we need to add them back to the Inspector.
Comment 1 Oliver Hunt 2008-10-07 15:46:36 PDT
I doubt this should be too hard for any one interested in implementing it.  A couple of other things worth investigating are hit count based break points.  eg. only trigger after n hits, every n-th hit, etc

I was thinking have breakpoints with actions would be worth while, however you could emulate that with a conditional breakpoint akin to:
"alert('Foo!'); false' 
or similar would do the trick, but then i realised we should guard against:
function f() {
     <conditional breakpoint "f()"> return true;

Which would result in infinite recursion (although the re-entry guard would catch it) which we may want to be more graceful in handling -- possibly re-entry to the debugger through the debugger should just throw an uncatchable exception (which would not be too difficult) which the debugger itself can catch, although it would need to occur in C++ :-/

Comment 2 Patrick Mueller 2009-06-29 08:53:33 PDT
Seems non-trivial to me.  The main problem seems like managing the UI for the wad of code you want to execute to indicate whether to stop or not.  Perhaps a little icon (?) beside the breakpoint marker that you could click, which would open a text editor to enter the conditional expression?

Note that you can already do this sort of thing, manually, by using the "debugger" statement in your own code, and presumably "wrapping" it's execution in an if statement that tests the condition you want.
Comment 3 Alexander Pavlov (apavlov) 2009-08-28 08:33:48 PDT
Created attachment 38740 [details]
Condition editor + conditional breakpoint screenshot

This is the proposed rendering of the condition editor (popup) and conditional breakpoints (a different marker in the source frame and a question mark after the breakpoint label in the sidebar pane.)
Comment 4 Oliver Hunt 2009-08-28 09:45:34 PDT
(In reply to comment #3)
> Created an attachment (id=38740) [details]
> Condition editor + conditional breakpoint screenshot
> This is the proposed rendering of the condition editor (popup) and conditional
> breakpoints (a different marker in the source frame and a question mark after
> the breakpoint label in the sidebar pane.)

The style of the condition editor doesn't match that of the inspector, and i'm also not to keen on the wording in the dialog
Comment 5 Alexander Pavlov (apavlov) 2009-08-28 09:53:16 PDT
Created attachment 38742 [details]
Comment 6 Alexander Pavlov (apavlov) 2009-08-28 10:00:34 PDT
(In reply to comment #4)

Sorry Oliver, I did not reload the bug before uploading the patch.

The popup style was borrowed from an XCode breakpoint-related window but that must have been not the best style source for WebKit. I'm fine with any style if you or someone else has an opinion. Also, what are the suggestions on the editbox
Comment 7 Oliver Hunt 2009-08-28 10:03:03 PDT
Can you talk to tim? (i'm not a UI designer - i just looked at the xcode one and thought basically exactly what i just told you, so maybe i should defer to someone who does :D )
Comment 8 Timothy Hatcher 2009-08-28 13:21:17 PDT
Comment on attachment 38742 [details]

I don't see why we need the Layout.js file. Maybe we do need some of these helper functions, but I feel this is too much abstraction. Something we have gotten away with not having up until now.

Sure a specific widget like a Popup might be a good abstraction, but Point, Rectangle and CssHelper are too generic…

We also have some helper function in utilities.js (like for totalOffsetLeft, etc). Adding helpers to that file as prototype functions on Element and Node, not in the WebInspector namespace, would be better and match our existing helpers.

Maybe you can break off the back-end parts as a seperate patch so it can land (since it looks pretty good.)
Comment 9 Timothy Hatcher 2009-08-28 13:31:20 PDT
Comment on attachment 38742 [details]

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


> +    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?
Comment 10 Timothy Hatcher 2009-08-28 13:32:47 PDT
Created attachment 38750 [details]
Drosera's Breakpoint Editor

Here is the old Drosera breakpoint editor I mentioned.
Comment 11 Timothy Hatcher 2009-08-28 13:33:16 PDT
I see the similarities with the Xcode UI, but the context in Xcode is a list view. This is a hovering box so I think the UI needs tweaked a little. We use to have a inline UI that sat below the line in hte source and didn't cover up anything.

This matched Xcode at one time, but is no longer in Xcode and we lost it when we dropped Drosera.

Somthing like the inline error bubbles would be good I think. I can think about the design some more. But getting somthing into the tree now and tweaking the design later would also be fine.
Comment 12 Alexander Pavlov (apavlov) 2009-11-02 05:06:26 PST
This feature has been landed. The associated bugs are
https://bugs.webkit.org/show_bug.cgi?id=28846 (backend) and
https://bugs.webkit.org/show_bug.cgi?id=28908 (frontend).