Closed Bug 781797 Opened 7 years ago Closed 7 years ago

dbg open and dbg close commands

Categories

(DevTools :: Debugger, defect, P3)

16 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: dangoor, Assigned: anton)

Details

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

Attachments

(1 file, 2 obsolete files)

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.
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]
(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 :)
Attached patch dbg open and dbg close (obsolete) — Splinter Review
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)
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+
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.
Attached patch dbg open and dbg close (obsolete) — Splinter Review
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+
Changed 'dbg close' to use dbg.close() directly per comments above.
Attachment #666064 - Attachment is obsolete: true
Attachment #667295 - Flags: review+
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
Closed: 7 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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.