dbg open and dbg close commands

RESOLVED FIXED in Firefox 18

Status

()

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

People

(Reporter: Kevin Dangoor, Assigned: anton)

Tracking

16 Branch
Firefox 18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
The Developer Toolbar gives users a good deal of control over the debugger. Type "break list", however, and you get a message that says "The debugger must be opened before setting breakpoints".

I propose that we have "dbg open" and "dbg close" commands to open and close the debugger, so that I don't have to hit the Debugger button or remember the keyboard shortcut.
(Reporter)

Updated

5 years ago
Version: 14 Branch → 16 Branch
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js]

Comment 1

5 years ago
(In reply to Kevin Dangoor from comment #0)
> The Developer Toolbar gives users a good deal of control over the debugger.
> Type "break list", however, and you get a message that says "The debugger
> must be opened before setting breakpoints".
> 
> I propose that we have "dbg open" and "dbg close" commands to open and close
> the debugger, so that I don't have to hit the Debugger button or remember
> the keyboard shortcut.

Hello,

I am new here and i am interested in working on this bug. Could anyone please assist me with this ? Thanks a lot :)
(Assignee)

Comment 2

5 years ago
Created attachment 665724 [details] [diff] [review]
dbg open and dbg close

So I was working on a different debugger bug today and the lack of 'dbg open' and 'dbg close' was rather annoying so I made this patch.

One note about tests: one my computer all tests were passing even when I used |DeveloperToolbarTest| in a synchronous way but I didn't want to risk failures on other platforms so I added a small helper |waitFor|.
Assignee: nobody → anton
Status: NEW → ASSIGNED
Attachment #665724 - Flags: review?(vporof)
(Assignee)

Comment 3

5 years ago
Eventually, I think we should also have stuff like 'dbg open my.js' and 'dbg set 4123' to open files and set breakpoints.
Comment on attachment 665724 [details] [diff] [review]
dbg open and dbg close

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

This is ok. f+ from me, but I'm sending it to past for review.

::: browser/devtools/commandline/CmdDbg.jsm
@@ +33,5 @@
> +    if (dbg) {
> +      if (dbg.ownerTab !== tab) {
> +        win.DebuggerUI.toggleDebugger();
> +      }
> +

Nit: why newline? :)

@@ +55,5 @@
> +    let dbg = win.DebuggerUI.findDebugger();
> +
> +    if (dbg && dbg.ownerTab === tab) {
> +      win.DebuggerUI.toggleDebugger();
> +    }

I think it'll be better if you did

if (dbg) {
  dbg.close();
}

I'm not sure if someone's also going to want to close the debugger from a different tab, but that's something which your approach doesn't handle.

::: browser/devtools/commandline/test/browser_dbg_cmd.js
@@ +7,5 @@
> +      typed: "dbg open",
> +      blankOutput: true
> +    });
> +
> +    waitFor(DebuggerUI.findDebugger, testDbgCmd);

You don't need to waitFor, because the actual creation of the container/iframe is synchronous. It's supper messy afterwards, but you don't need to worry about it in here.

@@ +52,5 @@
> +                          });
> +
> +                          waitFor(function () !DebuggerUI.findDebugger(), function () {
> +                            let dbg = DebuggerUI.findDebugger();
> +                            ok(!dbg, "Debugger was closed.");

Same here. If you look in the DebuggerPane close function,     "delete this.globalUI._scriptDebugger;" happens immediately on call, so no need to worry.

Run a few trys just to be sure though :) If you don't have commit access yet, I can push for you.

::: browser/devtools/commandline/test/head.js
@@ +31,5 @@
> +    aCallback();
> +  } else {
> +    setTimeout(function () waitFor(aFunction, aCallback), 100);
> +  }
> +}

As a general hint/note: I am very wary of timeouts/intervals in tests (or in code, anywhere else basically). They always end up biting someone's ass at some point. They're the devil.

For example, in this case a good event to listen for is Debugger:Shutdown on the chrome window, or maybe send a callback to DebuggerPane.close (well... not the same thing actually).

The good way of dealing with asyncnessness is to dive in the callback hell for now. Promises will make this much nicer at some point in the future when every tool finished adopting them.

But yeah, tl;dr: you don't need this now :)
Attachment #665724 - Flags: review?(vporof)
Attachment #665724 - Flags: review?(past)
Attachment #665724 - Flags: feedback+
(Assignee)

Comment 5

5 years ago
Thanks! Some replies:

> Nit: why newline? :)

Old habit of mine.

> I think it'll be better if you did

> if (dbg) {
>   dbg.close();
> }

> I'm not sure if someone's also going to want to close the debugger from a different tab, but that's something which your approach doesn't handle.

As far as I can see, |DebuggerUI.close| doesn't actually check if the debugger instance is open in the current tab. So just using |dbg.close()| will close debugger no matter what tab it was opened in. My code closes the debugger only if it's open within the current tab.
(Assignee)

Comment 6

5 years ago
Created attachment 666064 [details] [diff] [review]
dbg open and dbg close

Removed |waitFor|.
Attachment #665724 - Attachment is obsolete: true
Attachment #665724 - Flags: review?(past)
Attachment #666064 - Flags: review?(past)
(In reply to Anton Kovalyov (:anton) from comment #5)
> > I'm not sure if someone's also going to want to close the debugger from a different tab, but that's something which your approach doesn't handle.
> 
> As far as I can see, |DebuggerUI.close| doesn't actually check if the
> debugger instance is open in the current tab. So just using |dbg.close()|
> will close debugger no matter what tab it was opened in. My code closes the
> debugger only if it's open within the current tab.

Exactly. Don't we want to do this? I feel like we should, but I don't feel strongly about it. However, tying "dbg close" and then switching to a debugged tab to see the UI still open may be surprising. I'd be ok with at least a message saying "Debugger is not open in this tab".

Panos, what do you think?
Comment on attachment 666064 [details] [diff] [review]
dbg open and dbg close

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

(In reply to Victor Porof [:vp] from comment #7)
> (In reply to Anton Kovalyov (:anton) from comment #5)
> > > I'm not sure if someone's also going to want to close the debugger from a different tab, but that's something which your approach doesn't handle.
> > 
> > As far as I can see, |DebuggerUI.close| doesn't actually check if the
> > debugger instance is open in the current tab. So just using |dbg.close()|
> > will close debugger no matter what tab it was opened in. My code closes the
> > debugger only if it's open within the current tab.
> 
> Exactly. Don't we want to do this? I feel like we should, but I don't feel
> strongly about it. However, tying "dbg close" and then switching to a
> debugged tab to see the UI still open may be surprising. I'd be ok with at
> least a message saying "Debugger is not open in this tab".
> 
> Panos, what do you think?

Since we don't support debugging multiple tabs, I think closing any existing instance (regardless of the tab) makes more sense. You could argue that we should display the notification bar on 'dbg open' instead, but that would take the user's focus away from the command line, which is the exact opposite of what we are trying to optimize for.
Attachment #666064 - Flags: review?(past) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 667295 [details] [diff] [review]
'dbg open' and 'dbg close'

Changed 'dbg close' to use dbg.close() directly per comments above.
Attachment #666064 - Attachment is obsolete: true
Attachment #667295 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js] → [good first bug][mentor=vporof@mozilla.com][lang=js][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/7b61da6b47de
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js][land-in-fx-team] → [good first bug][mentor=vporof@mozilla.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7b61da6b47de
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=vporof@mozilla.com][lang=js]
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.