883220 - When a breakpoint is hit, the devtools window should focus

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Right now if devtools window is detached and breakpoint is hit there's no indication of that at all. I've run into this a few times when I was debugging a page and have not actually figured breakpoint was hit, thought page just got hang.

This is somewhat relates to Bug 823728, although I think active pane can remain active it's just some visual feedback would be useful. And if devtools window is detached it should get a focus so visual feedback can be seen if it's behind other windows.

Comment 1

[Girish Sharma [:Optimizer]]

Bug 868163 takes care of visually highlighting the debugger's tab so that you know a breakpoint is hit. We can use this bug ot raise the toolbox window when a breakpoint is hit just like we raise the toolbox window when we inspect anything

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: 1 liner

Just raise the toolbox when thread is paused.

Comment 3

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

Comment on attachment 763593 [details] [diff] [review]
1 liner

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

r+ with a test.

::: browser/devtools/debugger/DebuggerPanel.jsm
@@ +96,5 @@
>  
>    highlightWhenPaused: function() {
>      this._toolbox.highlightTool("jsdebugger");
> +    // Also raise the toolbox window if it is docked
> +    this._toolbox.raise();

Do you need to check if this is a window host first? Probably not, but just making sure you thought about this.

Comment 4

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #3)
> Comment on attachment 763593 [details] [diff] [review]
> 1 liner
> 
> Review of attachment 763593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a test.
> 
> ::: browser/devtools/debugger/DebuggerPanel.jsm
> @@ +96,5 @@
> >  
> >    highlightWhenPaused: function() {
> >      this._toolbox.highlightTool("jsdebugger");
> > +    // Also raise the toolbox window if it is docked
> > +    this._toolbox.raise();
> 
> Do you need to check if this is a window host first? Probably not, but just
> making sure you thought about this.

Not really, see http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#81 and the raise method is implemented for all host types. Doing the relevant thing in all three cases. I should probably update the comment reflecting that.

As for tests, I am going to add a framework level test, rather than debugger specific. I hope its okay .

Comment 5

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

(In reply to Girish Sharma [:Optimizer] from comment #4)
> 
> As for tests, I am going to add a framework level test, rather than debugger
> specific. I hope its okay .

Perfect!

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: 1 liner + 1 test

Added a test to see if raise works.

Will pass through try soon. Tests pass locally.

Comment 7

[Girish Sharma [:Optimizer]]

Attachment: patch v1

whoops. Forgot to remove debugging code.

Comment 8

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

Comment on attachment 763747 [details] [diff] [review]
1 liner + 1 test

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

I'm not sure where you're testing whether if highlightWhenPaused raises the host when the debugger pauses. If I'm reading the test correctly, it only makes sure that raise() does the right thing.

::: browser/devtools/framework/test/browser_toolbox_raise.js
@@ +35,5 @@
> +function testBottomHost(aToolbox)
> +{
> +  toolbox = aToolbox;
> +
> +  // switch to another tab and tes raise

s/tes/test/

@@ +46,5 @@
> +}
> +
> +function testWindowHost()
> +{
> +

Nit: why newline?

@@ +59,5 @@
> +
> +  cleanup();
> +}
> +
> +function getTopWindow() {try {

weird indenting here

@@ +73,5 @@
> +    if (type == "navigator:browser" || type == "devtools:toolbox") {
> +      return win;
> +    }
> +  }
> +  return null;}catch(ex) {window.alert(ex)}

really weird indenting...

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: patch v2

(In reply to Victor Porof [:vp] from comment #8)
> Comment on attachment 763747 [details] [diff] [review]
> 1 liner + 1 test
> 
> Review of attachment 763747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure where you're testing whether if highlightWhenPaused raises the
> host when the debugger pauses. If I'm reading the test correctly, it only
> makes sure that raise() does the right thing.

Yup, I am ensuring that raise does the right thing. Adding another test to make sure that the raise() does the right thing from debugger will be kinda redundant

> ::: browser/devtools/framework/test/browser_toolbox_raise.js
> @@ +35,5 @@
> > +function testBottomHost(aToolbox)
> > +{
> > +  toolbox = aToolbox;
> > +
> > +  // switch to another tab and tes raise
> 
> s/tes/test/
> 
> @@ +46,5 @@
> > +}
> > +
> > +function testWindowHost()
> > +{
> > +
> 
> Nit: why newline?
> 
> @@ +59,5 @@
> > +
> > +  cleanup();
> > +}
> > +
> > +function getTopWindow() {try {
> 
> weird indenting here
> 
> @@ +73,5 @@
> > +    if (type == "navigator:browser" || type == "devtools:toolbox") {
> > +      return win;
> > +    }
> > +  }
> > +  return null;}catch(ex) {window.alert(ex)}
> 
> really weird indenting...

old patch, this one has these fixed.

Comment 10

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

Comment on attachment 763761 [details] [diff] [review]
patch v2

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

(In reply to Girish Sharma [:Optimizer] from comment #9)
> 
> Yup, I am ensuring that raise does the right thing. Adding another test to
> make sure that the raise() does the right thing from debugger will be kinda
> redundant
> 

It is not redundant. Suppose something happens raise() doesn't work anymore when the debugger is paused. Or highlightWhenPaused gets modified in a way so that toolbox.raise() doesn't get called. Please add another test.

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: patch v3

Adds a test for the debugger too.

Comment 12

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

Comment on attachment 764894 [details] [diff] [review]
patch v3

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

Magnificent. r+ with a green try run.

Comment 13

[Girish Sharma [:Optimizer]]

Attachment: patch v4

Despite the claims, ZOrder window enumerator did not work well on Mac and some Linux, causing oranges. This patch now uses focus event handlers to check the focus state. Running through try. Will r+ if try is green.

Comment 14

[Girish Sharma [:Optimizer]]

aannd... window.focus() + window.addEventListener("focus" .. is also failing on Mac and linux.

Help is appreciated, you can test both the current version of the patch or the previous one and tell me what you see.

Comment 15

[Girish Sharma [:Optimizer]]

Attachment: green try patch

After a number of trial and errors, and help from Paul, this patch works.

green trys : 

https://tbpl.mozilla.org/?tree=Try&rev=f84c40883b88
https://tbpl.mozilla.org/?tree=Try&rev=54c6f37ca85d

Comment 16

[Rob Campbell [:rc] (:robcee)]

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

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5f285200124b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This test is on the fast tracking for disabling if bug 888811 and bug 891176 don't get some attention soon.

Comment 19

[Girish Sharma [:Optimizer]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> This test is on the fast tracking for disabling if bug 888811 and bug 891176
> don't get some attention soon.

Is there a way to disable the test only on Ubuntu ?

Because Ubuntu is conflicting here, I tried using the approach used in the second (test that was added in the bug) and it was failing for debug builds almost as a perma-orange.

Comment 20

[Girish Sharma [:Optimizer]]

(In reply to Girish Sharma [:Optimizer] from comment #19)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> second (test that was added in the bug) and it was failing for debug builds

This should be "second test (that"