Support conditional breakpoints

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Timothy Hatcher]

Drosera had conditional breakpoints, we need to add them back to the Inspector.

[Oliver Hunt]

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++ :-/

[Patrick Mueller]

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.

[Alexander Pavlov (apavlov)]

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

[Oliver Hunt]

(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

[Alexander Pavlov (apavlov)]

Created attachment 38742 [details]
patch

[Alexander Pavlov (apavlov)]

(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
label?

[Oliver Hunt]

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 )

[Timothy Hatcher]

Comment on attachment 38742 [details]
patch

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

[Timothy Hatcher]

Comment on attachment 38742 [details]
patch

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

lineToInfos.


> +    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?

[Timothy Hatcher]

Created attachment 38750 [details]
Drosera's Breakpoint Editor

Here is the old Drosera breakpoint editor I mentioned.

[Timothy Hatcher]

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.

[Alexander Pavlov (apavlov)]

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