951828 - Always select the variables tab when pausing and stack frames are available

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Victor Porof [:vporof][:vp]]

Scenario:

I went to the Events tab and checked an event. Then I did something to the debuggee that triggered that event. I was briefly confused as to why the debugger didn't break, then realized it did, but the Events tab was still selected and my instinct expected to see the Variables tab instead.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: dbg-always-switch.patch

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8349629 [details] [diff] [review]
dbg-always-switch.patch

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

What about when:

* I am in the event listener tab and am *not* paused

* I set a breakpoint by clicking in the gutter

* Because we aren't paused, the client interrupts and causes a pause so it can set the BP

* The event listener tab is switched away from, and as a user I am surprised. "Why did an "unrelated" action change tabs??"

-------------------

r+ once you have logic and a test to handle the above case.

::: browser/devtools/debugger/test/browser_dbg_break-on-dom-08.js
@@ +14,5 @@
> +    let gEvents = gView.EventListeners;
> +
> +    Task.spawn(function() {
> +      yield waitForSourceShown(aPanel, ".html");
> +      aDebuggee.addBodyClickEventListener();

Do we need to remove this event listener at the end of the test, or is that being too paranoid?

Comment 3

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> Comment on attachment 8349629 [details] [diff] [review]
> dbg-always-switch.patch
> 
> Review of attachment 8349629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What about when:
> 
> * I am in the event listener tab and am *not* paused
> 
> * I set a breakpoint by clicking in the gutter
> 
> * Because we aren't paused, the client interrupts and causes a pause so it
> can set the BP
> 
> * The event listener tab is switched away from, and as a user I am
> surprised. "Why did an "unrelated" action change tabs??"
> 

Smart thinking. I'll take care of this.

> -------------------
> 
> r+ once you have logic and a test to handle the above case.
> 
> ::: browser/devtools/debugger/test/browser_dbg_break-on-dom-08.js
> @@ +14,5 @@
> > +    let gEvents = gView.EventListeners;
> > +
> > +    Task.spawn(function() {
> > +      yield waitForSourceShown(aPanel, ".html");
> > +      aDebuggee.addBodyClickEventListener();
> 
> Do we need to remove this event listener at the end of the test, or is that
> being too paranoid?

Dude, there are so many event listeners added by this test :) Tab gets anyway, event listeners are added to content, so it doesn't matter.

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> 
> * The event listener tab is switched away from, and as a user I am
> surprised. "Why did an "unrelated" action change tabs??"


Wait, this actually doesn't happen. The instruments pane is shown only when we also have stack frames [0]. Do you still want me to write a test for this?

https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#701

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'll leave it up to you to decide whether you think it is worth it or not.

Comment 6

[Victor Porof [:vporof][:vp]]

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

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/07b3207166ba