Closed Bug 883220 Opened 11 years ago Closed 11 years ago

When a breakpoint is hit, the devtools window should focus

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: irakli, Assigned: Optimizer)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
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
Assignee: nobody → scrapmachines
Depends on: 868163
Attached patch 1 liner (obsolete) — Splinter Review
Just raise the toolbox when thread is paused.
Attachment #763593 - Flags: review?(vporof)
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.
Attachment #763593 - Flags: review?(vporof) → feedback+
(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 .
(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!
Attached patch 1 liner + 1 test (obsolete) — Splinter Review
Added a test to see if raise works.

Will pass through try soon. Tests pass locally.
Attachment #763593 - Attachment is obsolete: true
Attachment #763747 - Flags: review?(vporof)
Attached patch patch v1 (obsolete) — Splinter Review
whoops. Forgot to remove debugging code.
Attachment #763747 - Attachment is obsolete: true
Attachment #763747 - Flags: review?(vporof)
Attachment #763751 - Flags: review?(vporof)
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...
Attachment #763747 - Attachment is obsolete: false
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #763747 - Attachment is obsolete: true
Attachment #763751 - Attachment is obsolete: true
Attachment #763751 - Flags: review?(vporof)
Attachment #763761 - Flags: review?(vporof)
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.
Attachment #763761 - Flags: review?(vporof) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Adds a test for the debugger too.
Attachment #763761 - Attachment is obsolete: true
Attachment #764894 - Flags: review?(vporof)
Comment on attachment 764894 [details] [diff] [review]
patch v3

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

Magnificent. r+ with a green try run.
Attachment #764894 - Flags: review?(vporof) → review+
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #764894 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Debugger
OS: Mac OS X → All
Hardware: x86 → All
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.
Attached patch green try patchSplinter Review
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
Attachment #765636 - Attachment is obsolete: true
Attachment #767618 - Flags: review?(vporof)
Attachment #767618 - Flags: review?(vporof) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/5f285200124b
Flags: in-testsuite+
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5f285200124b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Depends on: 891176
Depends on: 888811
This test is on the fast tracking for disabling if bug 888811 and bug 891176 don't get some attention soon.
(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.
(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"
Summary: When breakpoint is hit devtools window should get a focus. → When a breakpoint is hit, the devtools window should focus
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.