1537740 - Misleading background when debugging using column breakpoints

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

Attachment: image.png

When the debugger hits a column breakpoints the code at the paused line is using background color to indicate (at least it seams to) the next expression being evaluated.

See the attached screenshot:

The second row: Pause at column 13 (the square(a) function is about to be executed)

But the entire line starting with square(a) is highlighted.

Shouldn't just square(a) expression be highlighted?

The entire example is:

function square(a) {
  return a * a;
}

function sum(a, b) {
  return a + b;
}

function test(a, b) {
  return sum(square(a), square(b));
}

Honza

Comment 1

[Logan Smyth [:loganfsmyth]]

Shouldn't just square(a) expression be highlighted?

We don't have any way to know the end of the expression that is being evaluated. All we know is the line/column of the breakpoint itself. It seems like our options are either:

• Highlight the whole line starting from the breakpoint position (which is what we do now)
• Highlight the single token found at the location of the breakpoint

Comment 2

[David Walsh :davidwalsh]

Do you have an opinion here Harald?

Comment 3

[Jason Laster [:jlast]]

It would be nice to support highlighting the expression (start & end). This is both a chrome parity issue and usability win.

In the short/medium term, I'd vote for the current behavior which makes it clear where the debug expression begins.

Comment 4

[Logan Smyth [:loganfsmyth]]

Attachment: Example pause highlights in Chrome

It would be nice to support highlighting the expression (start & end). This is both a chrome parity issue and usability win.

As far as I know, Chrome does not highlight the whole expression. I also don't think I'd support that as the behavior. The intention of the highlight is as an indication of roughly where execution is currently paused, not tell you what expression you are currently inside of. From looking at Chrome, it seems like they highlight the token starting at the current position and if that token is punctuation, also the next token, or something?

Comment 5

[Jason Laster [:jlast]]

Hmm, interesting... This definitely warrants some more thought...

I moved your attachment to a google drawing so it would be easier to add more examples and comment

Comment 6

[:Harald Kirschner :digitarald]

It can be confusing, as we currently highlight too much. Maybe just reducing this to the expression helps as quickfix.

With the column breakpoint information we have, could we use the next column breakpoint as end location for the highlight?

Comment 7

[Logan Smyth [:loganfsmyth]]

The only information we have is the exact location of the breakpoint itself. We can't know when the expression starts or ends unless we parse the content ourselves, which would be slow and not a good idea.

The next column breakpoint location also isn't super useful because that could be anywhere. For instance in this case

var foo = 4; for(var i = 0; ...
          ^              ^

the arrows would be the two column locations, and highlighting between them would cover other unrelated code.

I think realistically all we can do is highlight using the simple text processing. Right now we go from the location to the end of the line, but it should be pretty straightforward to go from the location to the end of the current token.

Comment 8

[:Harald Kirschner :digitarald]

I think realistically all we can do is highlight using the simple text processing. Right now we go from the location to the end of the line, but it should be pretty straightforward to go from the location to the end of the current token.

WFM, argument being that selecting less is better than too much.

Comment 9

[Miriam]

Attachment: highlighting_examples.png

From what I can tell, the logic responsible for highlighting on paused column breakpoints is also responsible for highlighting in contexts other than column breakpoints. While experimenting with changing the highlighting column breakpoints, I realized my change extended this behavior to all highlighting.

If we highlight from location to end of current token for column breakpoints, should we also use that same rule for all highlighting in general? I personally think extending the rule to all situations will be more consistent and clearer. I’m curious to hear what other people think.

Comment 10

[Jason Laster [:jlast]]

Hey Miriam, it seems worth exploring :)

I think this might line up well with how we do preview.

Comment 11

[Miriam]

I would like to claim this issue. Thanks!

Comment 12

[Miriam]

Attachment: Bug 1537740 highlight from location to end of current token when paused on breakpoint r=jlast

With the changes I made, any time the debugger is paused, only the token directly after the column breakpoint is highlighted. My commit also changes highlighting rules for debug lines without column breakpoints. I collected examples in the document below demonstrating cases with and without column breakpoints.

https://docs.google.com/document/d/1w9kaAzVgns5oQnONTPuPiREyhaq6NGd4mp6yvVUEU1Y/edit?usp=sharing

I changed one CSS file because without that change, the next column breakpoint after the token would also be highlighted which was misleading.

I would appreciate any feedback on these changes. Thank you!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/840bfd021063
highlight from location to end of current token when paused on breakpoint

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/840bfd021063