781797 - dbg open and dbg close commands

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Kevin Dangoor]

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.

Comment 1

[tienthanh8490]

(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 :)

Comment 2

[Anton Kovalyov (:anton)]

Attachment: 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|.

Comment 3

[Anton Kovalyov (:anton)]

Eventually, I think we should also have stuff like 'dbg open my.js' and 'dbg set 4123' to open files and set breakpoints.

Comment 4

[Victor Porof [:vporof][:vp]]

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 :)

Comment 5

[Anton Kovalyov (:anton)]

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.

Comment 6

[Anton Kovalyov (:anton)]

Attachment: dbg open and dbg close

Removed |waitFor|.

Comment 7

[Victor Porof [:vporof][:vp]]

(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 8

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 9

[Anton Kovalyov (:anton)]

Attachment: 'dbg open' and 'dbg close'

Changed 'dbg close' to use dbg.close() directly per comments above.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/7b61da6b47de

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/7b61da6b47de