Closed Bug 912057 Opened 6 years ago Closed 6 years ago

Move full toolbox into Browser Debugger

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

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.
Attached patch WIP v1 (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
Depends on: 915444
Attached patch WIP v2 (obsolete) — Splinter Review
* All tools now functioning
* Still many cleanups needed
* Still no tests
Attachment #798975 - Attachment is obsolete: true
Did we ever resolve the issues with debugging the debugger server, or was that a different bug?
(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!  :)
Depends on: 922823
Attached patch WIP v3 (obsolete) — Splinter Review
* 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.
Attachment #808039 - Attachment is obsolete: true
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.
No longer depends on: 922823
Attached patch Browser Toolbox (obsolete) — Splinter Review
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
Attachment #824362 - Attachment is obsolete: true
Attachment #829621 - Flags: feedback?(past)
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 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?
(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 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?
Attachment #829621 - Flags: feedback?(past) → feedback+
(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
Attachment #829621 - Attachment is obsolete: true
Attachment #8336596 - Flags: review?(past)
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.
Attachment #8336596 - Flags: review?(past) → review-
(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.
Blocks: 943177
(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?
Attachment #8336596 - Attachment is obsolete: true
Attachment #8338219 - Flags: review?(past)
This removes the Makefile.in in browser/devtools/debugger/test and moves the remaining entries into browser.ini.
Attachment #8338261 - Flags: review?(gps)
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?
hg blame tells me that test was disabled in bug 898199.
Now with more comments (and a consistent comment style)!
Attachment #8338261 - Attachment is obsolete: true
Attachment #8338261 - Flags: review?(gps)
Attachment #8338646 - Flags: review?(gps)
Last try run didn't seem to do anything... This is looking better: https://tbpl.mozilla.org/?tree=Try&rev=70b51f146f03
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!
Attachment #8338646 - Flags: review?(gps) → review+
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 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.
Attachment #8338219 - Flags: review?(past) → review+
Attachment #8338646 - Attachment is obsolete: true
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.
Attachment #8338219 - Attachment is obsolete: true
Attachment #8340961 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/677a76d18d50
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/677a76d18d50
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Blocks: 945450
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!
Depends on: 946797
Depends on: 946800
Depends on: 950046
Depends on: 950448
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.