Bad code preview for source mapped column breakpoints
Categories
(DevTools :: Debugger, defect, P3)
Tracking
(Not tracked)
People
(Reporter: Harald, Assigned: bomsy)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [debugger-reserve])
Attachments
(5 files)
What were you doing?
- Open Debugger on send.firefox.com
- Open
webpack:///app/api.js
- Set breakpoint on line 4,
let fileProtocolWssUrl = null;
- (See What happened?)
- Open the generated code
https://send.firefox.com/app.*.js
- (See What happened?)
What happened?
4: With api.js
open, the Breakpoints panel preview the code null;
6: With the generated source (app.js
) open, the preview is empty
What should have happened?
4: api.js
: See the full line in preview
6: app.js
: See more than an empty line
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
4: api.js: See the full line in preview
This isn't currently how this logic is implemented, though I do take your point. We should discuss what we actually want here. "full line" seems a bit dangerous since the line could be extremely long. At the moment the logic is essentially "take up to 100 characters on the current line starting at the breakable location. The issue is that "null;" actually is the breakable position in this case. Since you could have
var foo = one(),
bar = two();
it is the content after the =
that you are breaking at, not the "var" keyword.
Perhaps we should just take some number of characters before the breakable position on the line a well?
6: app.js: See more than an empty line
This is covered by https://bugzilla.mozilla.org/show_bug.cgi?id=1534328
Reporter | ||
Comment 3•6 years ago
|
||
Perhaps we should just take some number of characters before the breakable position on the line a well?
Going back to the start of the line makes more sense and is that Chrome seems to do as well. Breaking on functions is a typical use case and right now that ends up just showing {
.
Updated•6 years ago
|
Comment 4•6 years ago
|
||
Going back to the start of the line makes more sense and is that Chrome seems to do as well.
We can do start of the line, but we'll need some max number of characters on the line to look backwards since the line could be super long. Like if I have
var a=4;var b=4;var c=4;var d=4;var e=4;var f=4;var g=4;var h=4;var i=a();
in a minified file and we break at the a()
call at the end, we can't just go back forever. Maybe we take up to 10 characters before the breakpoint? Keep in mind that the more characters we have, the harder it will be to see the actual break location, at least in cases where the breakpoint position is father to the left on a line. Even ignoring the minified case, if you have
var result = dbg.selectors.getComplexThing(dbg.getState());
^ // Breakpoint on this call
as an example of code we might have in a test. If you create a breakpoint on getState
. If we just take the whole line, your text snippet preview would be showing var result = dbg.selector...
, which isn't actually related to the getState
call.
Reporter | ||
Comment 5•6 years ago
|
||
We can do start of the line, but we'll need some max number of characters on the line to look backwards since the line could be super long.
Maybe piling on too many heuristics, but here it goes: Take the whole line and, if it is too long, clamping it so the location is centered in the visible parts of source. So for the long example, we would maybe only show
…mplexThing(dbg.🚩getState());
Comment 6•6 years ago
|
||
Harald, would you like us to have also show the breakpoint marker or some other symbol? If so, could you provide some designs so we can make sure that would look good...
Updated•6 years ago
|
Updated•6 years ago
|
Comment 7•6 years ago
•
|
||
(In reply to Jason Laster [:jlast] from comment #6)
Harald, would you like us to have also show the breakpoint marker or some other symbol? If so, could you provide some designs so we can make sure that would look good...
I am voting for using the same marker as in the source code (if at all). So, we can use an existing icon and the same design we already have for columns breakpoints => consistency.
Simpler (and faster?) solution could also be to do what Chrome does.
I am also attaching a screenshot of what I see on my machine (it's a bit different from the existing screenshot)
Honza
Reporter | ||
Comment 8•6 years ago
|
||
I am voting for using the same marker as in the source code (if at all). So, we can use an existing icon and the same design we already have for columns breakpoints => consistency.
Agreed, the short blue arrow in-line marker makes the most sense.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 9•5 years ago
|
||
add flag 'fromStart' to force selecting text from the first column when the breakpoint is the first column breakpoint
Bug 1537615 - Show the text from the start of line if its the first column breakpoint and the text after is less thatn 20 chars r-jlast
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
See http://g.recordit.co/2unBDkmsgY.gif for how it looks after fix
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•4 years ago
|
Updated•2 years ago
|
Description
•