Open Bug 1537615 Opened 5 years ago Updated 2 years ago

Bad code preview for source mapped column breakpoints

Categories

(DevTools :: Debugger, defect, P3)

67 Branch
defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: Harald, Assigned: bomsy)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [debugger-reserve])

Attachments

(5 files)

Attached image app.js (deleted) —

What were you doing?

  1. Open Debugger on send.firefox.com
  2. Open webpack:///app/api.js
  3. Set breakpoint on line 4, let fileProtocolWssUrl = null;
  4. (See What happened?)
  5. Open the generated code https://send.firefox.com/app.*.js
  6. (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

Attached image api.js (deleted) —

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

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

Priority: -- → P2

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.

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

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

Flags: needinfo?(hkirschner)
Flags: needinfo?(odvarko)
Attached image image.png (deleted) —

(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

Flags: needinfo?(odvarko)

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.

Flags: needinfo?(hkirschner)
Priority: P2 → P3
Blocks: dbg-breakpoint
No longer blocks: dbg-column-bps-m1
Blocks: dbg-70
Whiteboard: [debugger-reserve]
Blocks: 1565711
Blocks: 1565713
No longer blocks: 1565711
No longer blocks: 1565713

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

Attached file After screencast (deleted) —

See http://g.recordit.co/2unBDkmsgY.gif for how it looks after fix

Assignee: nobody → b4bomsy
Status: NEW → ASSIGNED
Blocks: dbg-71
No longer blocks: dbg-70
Assignee: b4bomsy → nobody
Status: ASSIGNED → NEW
Assignee: nobody → hmanilla
Status: NEW → ASSIGNED
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: