Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Prevent the Debugger from opening on more than one tab

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rc, Assigned: vporof)

Tracking

unspecified
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
For now, we should prevent the debugger UI from opening on more than one tab.
(Reporter)

Updated

5 years ago
Blocks: 676586
(Reporter)

Updated

5 years ago
Blocks: 710258
(Reporter)

Updated

5 years ago
Priority: -- → P2

Comment 1

5 years ago
Can you expand on why you want to do that?  Is it mostly the nasty interaction of multiple nested event loops?
(Reporter)

Comment 2

5 years ago
there are a few bugs which this will prevent. The nasty multiple nested event loop problem is just one of them, which this will solve (though I don't think it's the right solution for that).

Mainly this is to provide a stop-gap fix for bug 710258 until that has a proper solution in place.

Ultimately, I think we should be able to open the Debugger UI wherever we want it but only pause one page at a time.
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
I'll use a notification box with a "switch to debugged tab" button.
(Assignee)

Comment 4

5 years ago
(In reply to Victor Porof from comment #3)
> I'll use a notification box with a "switch to debugged tab" button.

..and maybe a "close other debugger" option?
(Assignee)

Comment 5

5 years ago
Created attachment 627956 [details] [diff] [review]
v1

Needs test?
Attachment #627956 - Flags: feedback?(past)
Comment on attachment 627956 [details] [diff] [review]
v1

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

Looks good. I think we should have a test, but you could make the case that since this is only temporary, it's not all that critical. Let's get Rob's opinion on this.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +96,4 @@
>    },
>  
>    /**
>     * Get the debugger for a specified tab.

Nit: we no longer specify the tab.

@@ +170,5 @@
> +    notification = nbox.appendNotification(
> +      message, TAB_NAVIGATION_NOTIFICATION,
> +      imageURL, nbox.PRIORITY_WARNING_HIGH, buttons, null);
> +
> +    // Make sure this not a transient notification, to avoid the automatic

Nit: I think you mean "this is not".

::: browser/devtools/debugger/test/head.js
@@ +55,4 @@
>      finish();
>    }, false);
>    if (!aRemoteFlag) {
> +    DebuggerUI.getDebugger().close();

The aTab parameter can be removed from the function signature now.

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +12,5 @@
>  
> +# LOCALIZATION NOTE (confirmTabNavigation): The messages displayed for the
> +# title and buttons on the notification shown when a user attempts to open a
> +# debugger in a new tab while a different tab is already being debugged.
> +confirmTabNavigation.message=Debugger is already open in a different tab.

"in another tab" is better, I think.

@@ +15,5 @@
> +# debugger in a new tab while a different tab is already being debugged.
> +confirmTabNavigation.message=Debugger is already open in a different tab.
> +confirmTabNavigation.buttonSwitch=Switch to tab
> +confirmTabNavigation.buttonSwitch.accessKey=S
> +confirmTabNavigation.buttonOpen=Open anyway

We should probably mention the fact that the other debugger will close.
Attachment #627956 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 7

5 years ago
Comment on attachment 627956 [details] [diff] [review]
v1

How many beers do I owe you, Rob?
Attachment #627956 - Flags: feedback?(rcampbell)
(Reporter)

Comment 8

5 years ago
Comment on attachment 627956 [details] [diff] [review]
v1

I defer to the greater wisdom of The Panosaur.
Attachment #627956 - Flags: feedback?(rcampbell)
(Assignee)

Comment 9

5 years ago
Created attachment 628631 [details] [diff] [review]
v2

Added test.
Attachment #628631 - Flags: review?(past)
(Assignee)

Comment 10

5 years ago
Created attachment 628641 [details] [diff] [review]
v2.1

Argh, forgot to add an info to a tab test. One line change.
Attachment #628631 - Attachment is obsolete: true
Attachment #628631 - Flags: review?(past)
Attachment #628641 - Flags: review?(past)
(Assignee)

Updated

5 years ago
Attachment #628641 - Attachment is patch: true
Comment on attachment 628641 [details] [diff] [review]
v2.1

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

The test looks good to me, besides a small nit, but you forgot to address the nits in comment 6.
r=me with nits addressed.

::: browser/devtools/debugger/test/browser_dbg_debugger-tab-switch.js
@@ +68,5 @@
> +
> +    gNbox.addEventListener("AlertActive", function active() {
> +      gNbox.removeEventListener("AlertActive", active, true);
> +      executeSoon(function() {
> +        continueTest2();

Function declaration closer to the point of use, please. It doesn't look like the extra function buys you anything, though. Why not just inline it? (The same holds for the other continueTestX functions as well)
Attachment #628641 - Flags: review?(past) → review+
Attachment #627956 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 628652 [details] [diff] [review]
v2.2

Addressed comments.
I shall write "qfold" on my monitor to stop forgetting it.
Whiteboard: [land-in-fx-team]
(Reporter)

Comment 13

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/9448caa01cf3
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
(Reporter)

Comment 14

5 years ago
https://hg.mozilla.org/mozilla-central/rev/9448caa01cf3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Depends on: 760752
You need to log in before you can comment on or make changes to this bug.