687093 - Clicking on a stack frame in the debugger should put the caret in the proper source line

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

When a user clicks on a stack frame, the source editor pane should scroll the corresponding line into view (preferably at the top of the pane) and put the caret there. Highlighting the line would be nice, too.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP

Clicking on a stack frame now works as expected, but when entering the paused state the right script and line are not automatically selected. This cannot be fixed until bug 706506 is taken care of.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP 2

This works now (on top of bug 706506), but the test still has a couple of failures that I need to get to the bottom of.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch

Fixed the test as well as a few others that this patch broke, and added an extra fix for a flicker during onScripts.

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 593134 [details] [diff] [review]
Working patch

Review of attachment 593134 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good!

::: browser/devtools/debugger/test/browser_dbg_select-line.js
@@ +69,5 @@
> +        });
> +      });
> +
> +      // Click the oldest stack frame.
> +      EventUtils.sendMouseEvent({ type: "click" },

Can you use synthesizeMouse() here?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch v2

(In reply to Mihai Sucan [:msucan] from comment #4)
> Comment on attachment 593134 [details] [diff] [review]
> Working patch
> 
> Review of attachment 593134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good!
> 
> ::: browser/devtools/debugger/test/browser_dbg_select-line.js
> @@ +69,5 @@
> > +        });
> > +      });
> > +
> > +      // Click the oldest stack frame.
> > +      EventUtils.sendMouseEvent({ type: "click" },
> 
> Can you use synthesizeMouse() here?

Absolutely, fixed.

(carrying over the r+)

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: [in-fx-team] Working patch v3

Test fix required after the latest merge and a small hunk from the patch in bug 711164, since I reordered them. This hunk has extensive tests in that patch, but I wanted to get this in and not wait for stepping support to be finished.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Diff between v2 and v3

Changes since your last review.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 596060 [details] [diff] [review]
[in-fx-team] Working patch v3

Sorry, I got confused. Mihai has been reviewing this.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 596060 [details] [diff] [review]
[in-fx-team] Working patch v3

Looks good!

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/c17bad2abafe

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c17bad2abafe