Bug 199033 - Web Inspector: Debugger: setting a breakpoint on a non-JavaScript line should immediately show as unresolved
Summary: Web Inspector: Debugger: setting a breakpoint on a non-JavaScript line should...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-19 14:15 PDT by Devin Rousso
Modified: 2019-06-20 15:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.30 KB, patch)
2019-06-19 15:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.30 KB, patch)
2019-06-20 15:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-06-19 14:15:07 PDT
# STEPS TO REPRODUCE:
1. inspect any page
2. set a breakpoint on a non-<script> line of any HTML file
 => breakpoint icon shows blue
3. move (drag) the breakpoint to a different non-script line in the same file
 => breakpoint icon changes to dark grey
Comment 1 Devin Rousso 2019-06-19 15:53:34 PDT
Created attachment 372497 [details]
Patch
Comment 2 Matt Baker 2019-06-20 11:04:14 PDT
Comment on attachment 372497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372497&action=review

r=me, nice change.

> Source/WebInspectorUI/ChangeLog:12
> +        by the time that `shouldSpeculativelyResolve` would've taken effect, so it's unnecessary.

Any idea why it existed in the first place?

> Source/WebInspectorUI/ChangeLog:14
> +        This only affects resolving breakpoints set via the gutter of a `SourceCodeTextEditor`.

Maybe "This parameter was only used when setting a breakpoint via...". I had to check the code to clarify.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:933
> +    _setBreakpoint(breakpoint, specificTarget)

Interesting. It looks like a breakpoint only begins its life in the resolved state when restoring breakpoints in DebuggerManager.prototype.initializeTarget (specificTarget will be non-null).
Comment 3 Radar WebKit Bug Importer 2019-06-20 13:23:19 PDT
<rdar://problem/51957111>
Comment 4 Devin Rousso 2019-06-20 15:20:33 PDT
Comment on attachment 372497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372497&action=review

>> Source/WebInspectorUI/ChangeLog:12
>> +        by the time that `shouldSpeculativelyResolve` would've taken effect, so it's unnecessary.
> 
> Any idea why it existed in the first place?

It was added in r171784.  My guess is that at the time, `breakpoint.resolved` was used as a "gate" for invoking certain backend commands, so automatically marking it as resolved would help avoid those.  Nowadays, it's not used for basically anything other than UI.
Comment 5 Devin Rousso 2019-06-20 15:20:59 PDT
Created attachment 372592 [details]
Patch
Comment 6 WebKit Commit Bot 2019-06-20 15:52:09 PDT
Comment on attachment 372592 [details]
Patch

Clearing flags on attachment: 372592

Committed r246659: <https://trac.webkit.org/changeset/246659>
Comment 7 WebKit Commit Bot 2019-06-20 15:52:11 PDT
All reviewed patches have been landed.  Closing bug.