Closed Bug 753311 Opened 8 years ago Closed 8 years ago

Prevent the Debugger from opening on more than one tab

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 15

People

(Reporter: rcampbell, Assigned: vporof)

References

Details

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

Attachments

(2 files, 2 obsolete files)

For now, we should prevent the debugger UI from opening on more than one tab.
Blocks: minotaur
Blocks: 710258
Priority: -- → P2
Can you expand on why you want to do that?  Is it mostly the nasty interaction of multiple nested event loops?
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: nobody → vporof
Status: NEW → ASSIGNED
I'll use a notification box with a "switch to debugged tab" button.
(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?
Attached patch v1 (obsolete) — Splinter Review
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+
Comment on attachment 627956 [details] [diff] [review]
v1

How many beers do I owe you, Rob?
Attachment #627956 - Flags: feedback?(rcampbell)
Comment on attachment 627956 [details] [diff] [review]
v1

I defer to the greater wisdom of The Panosaur.
Attachment #627956 - Flags: feedback?(rcampbell)
Attached patch v2 (obsolete) — Splinter Review
Added test.
Attachment #628631 - Flags: review?(past)
Attached patch v2.1Splinter Review
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)
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
Attached patch v2.2Splinter Review
Addressed comments.
I shall write "qfold" on my monitor to stop forgetting it.
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/9448caa01cf3
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9448caa01cf3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Depends on: 760752
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.