912057 - Move full toolbox into Browser Debugger

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

During the DevTools work week, I demoed a complete toolbox OOP in the Browser Debugger window.  This could be quite useful for the DevTools team and FF / add-on developers generally.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: WIP v1

This is the rough version of the browser toolbox I demoed.  Still needs a fair amount of work to ensure all tools make sense / work correctly, plus tests too.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: WIP v2

* All tools now functioning
* Still many cleanups needed
* Still no tests

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Did we ever resolve the issues with debugging the debugger server, or was that a different bug?

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> Did we ever resolve the issues with debugging the debugger server, or was
> that a different bug?

That is separate (bug 910184, relanded in bug 911127), but it is resolved.  You should be able to debug actors from the Browser Debugger currently.  File a bug if you can't!  :)

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: WIP v3

* Rebased

I was hoping this would help bgrins target the Dev Tools, and it does get close, but I couldn't seem to find the frame containing the toolbox in the inspector.

Comment 6

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

I was about to file a new bug, but I realized that one other thing this bug will fix is the ability to toggle the browser debugger's theme.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Browser Toolbox

This is now in a pretty good state.  There still aren't tests yet, so let me know what you think should be done there.  It doesn't seem like we really had them for browser debugger either...

There's no way to select different windows, but that's something that can be worked on after this.

Try: https://tbpl.mozilla.org/?tree=Try&rev=a7654bfd6aba

Comment 8

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

I want to apply and play with this patch in order to provide meaningful feedback, but I won't be able to get at it before Thursday, as I don't want to mess with my local builds that I'll be using for my talk :-)

Comment 9

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

Comment on attachment 829621 [details] [diff] [review]
Browser Toolbox

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

::: browser/base/content/browser-appmenu.inc
@@ +148,5 @@
>            <menuitem id="appmenu_devToolbar"
>                      observes="devtoolsMenuBroadcaster_DevToolbar"/>
>            <menuitem id="appmenu_devAppMgr"
>                      observes="devtoolsMenuBroadcaster_DevAppMgr"/>
> +          <menuitem id="appmenu_browserToolbox"

Just curious: do we need both a browser toolbox and a browser console?

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Victor Porof [:vp] from comment #9)
> 
> Just curious: do we need both a browser toolbox and a browser console?

Well, this browser toolbox currently opens in a separate process, just like the browser debugger today.

So, the browser console menu option is a bit simpler to use, since it opens in the same process.  But yes, they should contain the same information once they are both open.

Comment 11

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

Comment on attachment 829621 [details] [diff] [review]
Browser Toolbox

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

This looks good to me.

For tests, I think we should at least convert or expand browser_dbg_chrome_create.js to cover the entire toolbox. As you said though, we didn't really have broad test coverage for the browser debugger, mainly due to the inability to run the debugger with debuggee frames on the stack. This wouldn't preclude testing the other tools in the browser toolbox though, so maybe we could at least make sure that the basic functionality is there, even if the test never opens the debugger panel.

Also, could you do a "hg mv" for DebuggerProcess.jsm and ToolboxProcess.jsm, so that it's easier to see the changes?

::: browser/devtools/framework/ToolboxProcess.jsm
@@ +190,5 @@
> +}
> +
> +let wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
> +
> +Services.prefs.addObserver("devtools.debugger.log", {

I know that this is not new to this patch, but this observer should be removed at some point in time.

::: browser/devtools/framework/toolbox-hosts.js
@@ +339,5 @@
> +  destroy: function IWH_destroy() {
> +    if (!this._destroyed) {
> +      this._destroyed = true;
> +
> +      this._window.removeEventListener("unload", this._boundUnload);

Why not call this._unload(), so that "window-closed" listeners get a chance to be notified?

::: browser/devtools/framework/toolbox-process-window.xul
@@ +30,5 @@
> +         command="toolbox-cmd-close"
> +         modifiers="accel"/>
> +  </keyset>
> +
> +  <iframe id="toolbox-iframe" flex="1" forceOwnRefreshDriver=""></iframe>

Could you remind me why forceOwnRefreshDriver is here for?

::: browser/devtools/framework/toolbox.js
@@ +107,1 @@
>    CUSTOM: "custom"

I don't see custom in fx-team tip, where does that come from?

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Replace Browser Debugger with Browser Toolbox v2

(In reply to Panos Astithas [:past] from comment #11)
> For tests, I think we should at least convert or expand
> browser_dbg_chrome_create.js to cover the entire toolbox. 

This test has been converted to work with the browser toolbox.

> As you said
> though, we didn't really have broad test coverage for the browser debugger,
> mainly due to the inability to run the debugger with debuggee frames on the
> stack. This wouldn't preclude testing the other tools in the browser toolbox
> though, so maybe we could at least make sure that the basic functionality is
> there, even if the test never opens the debugger panel.

At the moment, I have not added additional tests, so let me know if that seems like a requirement for r+.

> Also, could you do a "hg mv" for DebuggerProcess.jsm and ToolboxProcess.jsm,
> so that it's easier to see the changes?

Yep, sorry about that.  Should be easier to read now.

> ::: browser/devtools/framework/ToolboxProcess.jsm
> @@ +190,5 @@
> > +}
> > +
> > +let wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
> > +
> > +Services.prefs.addObserver("devtools.debugger.log", {
> 
> I know that this is not new to this patch, but this observer should be
> removed at some point in time.

Hmm, why don't we want this?  It seemed like it was added relatively recently to DebuggerProcess.jsm...

> ::: browser/devtools/framework/toolbox.js
> @@ +107,1 @@
> >    CUSTOM: "custom"
> 
> I don't see custom in fx-team tip, where does that come from?

This is from Paul's toolbox in a tab (bug 912891) which was backed out, but has now landed again.

> ::: browser/devtools/framework/toolbox-hosts.js
> @@ +339,5 @@
> > +  destroy: function IWH_destroy() {
> > +    if (!this._destroyed) {
> > +      this._destroyed = true;
> > +
> > +      this._window.removeEventListener("unload", this._boundUnload);
> 
> Why not call this._unload(), so that "window-closed" listeners get a chance
> to be notified?

I am now using Paul's CustomHost instead, which seemed better than adding yet another host variant, so this code is gone now. 

> ::: browser/devtools/framework/toolbox-process-window.xul
> @@ +30,5 @@
> > +         command="toolbox-cmd-close"
> > +         modifiers="accel"/>
> > +  </keyset>
> > +
> > +  <iframe id="toolbox-iframe" flex="1" forceOwnRefreshDriver=""></iframe>
> 
> Could you remind me why forceOwnRefreshDriver is here for?

Good catch, it's actually not needed is this case.  It was added in bug 832920 to fix transitions after changing to between toolbox types, but that never happens in this out of process setup.  Removed.

Try: https://tbpl.mozilla.org/?tree=Try&rev=28948a639c2b

Comment 13

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

Comment on attachment 8336596 [details] [diff] [review]
Replace Browser Debugger with Browser Toolbox v2

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

I found a couple of issues with the patch that need to be resolved first, but this is very close.

::: browser/devtools/devtools-clhandler.js
@@ +67,5 @@
>      }
>    },
>  
>    helpInfo : "  -jsconsole         Open the Browser Console.\n" +
> +             "  -jsdebugger        Open the Browser Toolbox.\n",

This breaks debugging mochitests with --jsdebugger, since the user now has to remember to switch to the debugger tab first. We can just make the toolbox open the debugger by default in this case to avoid that.

::: browser/devtools/framework/toolbox-process-window.js
@@ +56,5 @@
> +   raise();
> +}
> +
> +function bindToolboxHandlers() {
> +  gToolbox.on("destroyed", quitApp);

Let's remove the event listener in quitApp as well for consistency. Or even better, use once().

::: browser/devtools/webconsole/hudservice.js
@@ +341,5 @@
>    {
> +    // In some cases, like the Browser Toolbox, there is no browser window.
> +    if (!this.browserWindow) {
> +      return null;
> +    }

Now it looks like network requests can no longer be inspected in the web console, as I'm getting this error:

System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/network-panel.js:35 - TypeError: aParent is null

Also, network monitor has some issues as well:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.readAndConvertFromStream is not a function" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js" line: 258}]' when calling method: [nsIRequestObserver::onStopRequest]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************
System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/network-panel.js:35 - TypeError: aParent is null

::: toolkit/devtools/server/main.js
@@ +362,5 @@
>      if ("nsIProfiler" in Ci)
>        this.addActors("resource://gre/modules/devtools/server/actors/profiler.js");
>  
>      this.addActors("resource://gre/modules/devtools/server/actors/styleeditor.js");
> +    this.addGlobalActor(this.StyleEditorActor, "styleEditorActor");

This belongs in the style editor actor definition.

Comment 14

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

(In reply to J. Ryan Stinnett [:jryans] from comment #12)
> (In reply to Panos Astithas [:past] from comment #11)
> > For tests, I think we should at least convert or expand
> > browser_dbg_chrome_create.js to cover the entire toolbox. 
> 
> This test has been converted to work with the browser toolbox.

Where is that change? I don't see it in this patch. Moreover, it looks like the test is no longer referenced in browser.ini at all, so it won't even try to run.

> > As you said
> > though, we didn't really have broad test coverage for the browser debugger,
> > mainly due to the inability to run the debugger with debuggee frames on the
> > stack. This wouldn't preclude testing the other tools in the browser toolbox
> > though, so maybe we could at least make sure that the basic functionality is
> > there, even if the test never opens the debugger panel.
> 
> At the moment, I have not added additional tests, so let me know if that
> seems like a requirement for r+.

No, I wouldn't block this patch on such tests, but I'd like a followup at least.

> > ::: browser/devtools/framework/ToolboxProcess.jsm
> > @@ +190,5 @@
> > > +}
> > > +
> > > +let wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
> > > +
> > > +Services.prefs.addObserver("devtools.debugger.log", {
> > 
> > I know that this is not new to this patch, but this observer should be
> > removed at some point in time.
> 
> Hmm, why don't we want this?  It seemed like it was added relatively
> recently to DebuggerProcess.jsm...

What I meant was that this addObserver call should have a corresponding removeObserver call somewhere.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Replace Browser Debugger with Browser Toolbox v3

(In reply to Panos Astithas [:past] from comment #13)
> ::: browser/devtools/devtools-clhandler.js
> @@ +67,5 @@
> >      }
> >    },
> >  
> >    helpInfo : "  -jsconsole         Open the Browser Console.\n" +
> > +             "  -jsdebugger        Open the Browser Toolbox.\n",
> 
> This breaks debugging mochitests with --jsdebugger, since the user now has
> to remember to switch to the debugger tab first. We can just make the
> toolbox open the debugger by default in this case to avoid that.

Okay, as discussed I've changed the Browser Toolbox to always open the Debugger tab first.

> ::: browser/devtools/framework/toolbox-process-window.js
> @@ +56,5 @@
> > +   raise();
> > +}
> > +
> > +function bindToolboxHandlers() {
> > +  gToolbox.on("destroyed", quitApp);
> 
> Let's remove the event listener in quitApp as well for consistency. Or even
> better, use once().

Changed to use once().

> ::: browser/devtools/webconsole/hudservice.js
> @@ +341,5 @@
> >    {
> > +    // In some cases, like the Browser Toolbox, there is no browser window.
> > +    if (!this.browserWindow) {
> > +      return null;
> > +    }
> 
> Now it looks like network requests can no longer be inspected in the web
> console, as I'm getting this error:
> 
> System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js ->
> resource:///modules/devtools/webconsole/network-panel.js:35 - TypeError:
> aParent is null

After talking Mihai a bit more about how to best handle this, I am now supplying the utilities that the web console used to rely on a browser window for.  To check this part out, look at:

* browser/devtools/webconsole/hudservice.js (relies on toolbox-process-window.xul when no browser)
* browser/devtools/framework/toolbox-process-window.xul (extra script tags to import more utils)

> Also, network monitor has some issues as well:
> 
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'[JavaScript Error: "this.readAndConvertFromStream is not a
> function" {file: "resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js" line:
> 258}]' when calling method: [nsIRequestObserver::onStopRequest]"  nsresult:
> "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native
> frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
> ************************************************************
> System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js ->
> resource:///modules/devtools/webconsole/network-panel.js:35 - TypeError:
> aParent is null

I have not been able to duplicate such errors with the network monitor.  It appears to be working fine in my tests.  If you can still reproduce them with this patch, could you give me STR?

> ::: toolkit/devtools/server/main.js
> @@ +362,5 @@
> >      if ("nsIProfiler" in Ci)
> >        this.addActors("resource://gre/modules/devtools/server/actors/profiler.js");
> >  
> >      this.addActors("resource://gre/modules/devtools/server/actors/styleeditor.js");
> > +    this.addGlobalActor(this.StyleEditorActor, "styleEditorActor");
> 
> This belongs in the style editor actor definition.

Moved into the style editor actor.

(In reply to Panos Astithas [:past] from comment #14)
> (In reply to J. Ryan Stinnett [:jryans] from comment #12)
> > (In reply to Panos Astithas [:past] from comment #11)
> > > For tests, I think we should at least convert or expand
> > > browser_dbg_chrome_create.js to cover the entire toolbox. 
> > 
> > This test has been converted to work with the browser toolbox.
> 
> Where is that change? I don't see it in this patch.

The only changes for the test were in browser/devtools/debugger/test/head.js to rename BrowserDebuggerProcess to BrowserToolboxProcess.  But maybe you wanted it to test more things too?

> Moreover, it looks like
> the test is no longer referenced in browser.ini at all, so it won't even try
> to run.

It's still enabled, but just hanging out as one of the few entries in Makefile.in.  I'll attach a separate patch to convert the remaining entries to browser.ini, as I agree it's confusing with the two separate manifest files.

> > > As you said
> > > though, we didn't really have broad test coverage for the browser debugger,
> > > mainly due to the inability to run the debugger with debuggee frames on the
> > > stack. This wouldn't preclude testing the other tools in the browser toolbox
> > > though, so maybe we could at least make sure that the basic functionality is
> > > there, even if the test never opens the debugger panel.
> > 
> > At the moment, I have not added additional tests, so let me know if that
> > seems like a requirement for r+.
> 
> No, I wouldn't block this patch on such tests, but I'd like a followup at
> least.

Filed bug 943177.

> > > ::: browser/devtools/framework/ToolboxProcess.jsm
> > > @@ +190,5 @@
> > > > +}
> > > > +
> > > > +let wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
> > > > +
> > > > +Services.prefs.addObserver("devtools.debugger.log", {
> > > 
> > > I know that this is not new to this patch, but this observer should be
> > > removed at some point in time.
> > 
> > Hmm, why don't we want this?  It seemed like it was added relatively
> > recently to DebuggerProcess.jsm...
> 
> What I meant was that this addObserver call should have a corresponding
> removeObserver call somewhere.

What's the best practice here?  Add another observer on "quit-application" that removes itself and this lingering observer?

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Convert debugger test list to manifest

This removes the Makefile.in in browser/devtools/debugger/test and moves the remaining entries into browser.ini.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=97e4544ad690

Comment 18

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

Comment on attachment 8338261 [details] [diff] [review]
Convert debugger test list to manifest

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

Drive-by: I very much like this change!

::: browser/devtools/debugger/test/browser.ini
@@ +105,5 @@
>  [browser_dbg_debugger-statement.js]
>  [browser_dbg_editor-contextmenu.js]
>  [browser_dbg_editor-mode.js]
> +[browser_dbg_event-listeners.js]
> +skip-if = os == "mac"

Can you please add a comment here as well?

Comment 19

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

hg blame tells me that test was disabled in bug 898199.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Convert debugger test list to manifest v2

Now with more comments (and a consistent comment style)!

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Last try run didn't seem to do anything... This is looking better: https://tbpl.mozilla.org/?tree=Try&rev=70b51f146f03

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8338646 [details] [diff] [review]
Convert debugger test list to manifest v2

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

You don't need my review on this since it only touches test file declarations. But I'll give you review!

Comment 23

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

Comment on attachment 8338646 [details] [diff] [review]
Convert debugger test list to manifest v2

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

This is now redundant after bug 939271 landed.

Comment 24

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

Comment on attachment 8338219 [details] [diff] [review]
Replace Browser Debugger with Browser Toolbox v3

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

Everything seems to work fine now, great!

(In reply to J. Ryan Stinnett [:jryans] from comment #15)
> (In reply to Panos Astithas [:past] from comment #13)
> > > > ::: browser/devtools/framework/ToolboxProcess.jsm
> > > > @@ +190,5 @@
> > > > > +}
> > > > > +
> > > > > +let wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
> > > > > +
> > > > > +Services.prefs.addObserver("devtools.debugger.log", {
> > > > 
> > > > I know that this is not new to this patch, but this observer should be
> > > > removed at some point in time.
> > > 
> > > Hmm, why don't we want this?  It seemed like it was added relatively
> > > recently to DebuggerProcess.jsm...
> > 
> > What I meant was that this addObserver call should have a corresponding
> > removeObserver call somewhere.
> 
> What's the best practice here?  Add another observer on "quit-application"
> that removes itself and this lingering observer?

I think removing it in BrowserToolboxProcess.prototype.close should suffice, but we don't need to block this patch on adding this if it's too much trouble.

::: browser/devtools/framework/ToolboxProcess.jsm
@@ +21,3 @@
>  
>  /**
>   * Constructor for creating a process that will hold a chrome debugger.

s/chrome debugger/chrome toolbox/

@@ +37,5 @@
>    this._create();
>  };
>  
>  /**
>   * Initializes and starts a chrome debugger process.

Same here.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Replace Browser Debugger with Browser Toolbox v4

Carrying over Panos' r+ from attachment 8338219 [details] [diff] [review].

(In reply to Panos Astithas [:past] from comment #24)
> I think removing it in BrowserToolboxProcess.prototype.close should suffice,
> but we don't need to block this patch on adding this if it's too much
> trouble.

I looked into moving it there, but it's somewhat awkward, since it becomes associated with an instance of BrowserToolboxProcess, but yet the observer modifies a global value.  I think I'll leave it as-is for now.

> ::: browser/devtools/framework/ToolboxProcess.jsm
> @@ +21,3 @@
> >  
> >  /**
> >   * Constructor for creating a process that will hold a chrome debugger.
> 
> s/chrome debugger/chrome toolbox/
> 
> @@ +37,5 @@
> >    this._create();
> >  };
> >  
> >  /**
> >   * Initializes and starts a chrome debugger process.
> 
> Same here.

Updated the patch to make these and other similar string replacements.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/677a76d18d50

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/677a76d18d50

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

This is a big step forward for those of hacking on non-primary content; thanks so much to :jryans and everyone else who made this happen!