Bug 198932 – Web Inspector: Debugger: adding a DOM/Event/URL breakpoint should enable breakpoints

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

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. disable breakpoints (navigation sidebar of Debugger/Sources tab)
3. create an Event/URL breakpoint
 => breakpoint starts out disabled

[Devin Rousso]

Created attachment 372272 [details]
Patch

[Matt Baker]

I'm not sure about this. What about the following scenario:

1. I disable breakpoints. Maybe a long running script kept pausing on some existing breakpoints, and interrupting my work.
2. I add a URL or event breakpoint.
3. This causes breakpoints to be enabled
=> Debugger pauses on one of my other breakpoints, interrupting me

If a user disables breakpoints globally, we should respect it until they turn it back on. We already provide a warning to surface this state.

[Devin Rousso]

(In reply to Matt Baker from comment #2)
> I'm not sure about this. What about the following scenario:
> 
> 1. I disable breakpoints. Maybe a long running script kept pausing on some existing breakpoints, and interrupting my work.
> 2. I add a URL or event breakpoint.
> 3. This causes breakpoints to be enabled
> => Debugger pauses on one of my other breakpoints, interrupting me
> 
> If a user disables breakpoints globally, we should respect it until they turn it back on. We already provide a warning to surface this state.
This patch matches the behavior of regular breakpoints, which enable breakpoints globally when a new breakpoint is set or an existing breakpoint is enabled.

If I'm setting a new breakpoint, it's likely the case that I'm going to soon thereafter trigger that breakpoint, and having to click twice (once to set the new breakpoint and once again to enable breakpoints globally) would be annoying.

Another issue is that if I'm setting a DOM breakpoint or specific event listener breakpoint via the Node sidebar, there's no banner in the Elements tab that says "breakpoints disabled".  I don't think we should add one anywhere in that tab either, as that's just "out of place" (DOM/elements vs JavaScript/debugging).  Currently, setting a DOM breakpoint or a specific event listener breakpoint would basically "silently fail" (which is something that I got quite aggravated about earlier, hence this bug).

I agree that the situation you describe isn't ideal, but I think the rest of the situations I've described here would have a much better experience with this change.

[Devin Rousso]

Created attachment 372281 [details]
Patch

[Matt Baker]

(In reply to Devin Rousso from comment #3)
> (In reply to Matt Baker from comment #2)
> > I'm not sure about this. What about the following scenario:
> > 
> > 1. I disable breakpoints. Maybe a long running script kept pausing on some existing breakpoints, and interrupting my work.
> > 2. I add a URL or event breakpoint.
> > 3. This causes breakpoints to be enabled
> > => Debugger pauses on one of my other breakpoints, interrupting me
> > 
> > If a user disables breakpoints globally, we should respect it until they turn it back on. We already provide a warning to surface this state.
> This patch matches the behavior of regular breakpoints, which enable
> breakpoints globally when a new breakpoint is set or an existing breakpoint
> is enabled.

I agree we should definitely be consistent across all our breakpoint types. I didn't know this also happened when enabling a breakpoint.

> If I'm setting a new breakpoint, it's likely the case that I'm going to soon
> thereafter trigger that breakpoint, and having to click twice (once to set
> the new breakpoint and once again to enable breakpoints globally) would be
> annoying.
> 
> Another issue is that if I'm setting a DOM breakpoint or specific event
> listener breakpoint via the Node sidebar, there's no banner in the Elements
> tab that says "breakpoints disabled".  I don't think we should add one
> anywhere in that tab either, as that's just "out of place" (DOM/elements vs
> JavaScript/debugging).  Currently, setting a DOM breakpoint or a specific
> event listener breakpoint would basically "silently fail" (which is
> something that I got quite aggravated about earlier, hence this bug).

Good point.

> I agree that the situation you describe isn't ideal, but I think the rest of
> the situations I've described here would have a much better experience with
> this change.

The situation I described was a hypothetical, and since we have established an existing behavior with script breakpoints let's go ahead!

[Matt Baker]

Comment on attachment 372281 [details]
Patch

r=me. Nice detailed explanation in the change log.

[WebKit Commit Bot]

Comment on attachment 372281 [details]
Patch

Clearing flags on attachment: 372281

Committed r246523: <https://trac.webkit.org/changeset/246523>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51827160>