814655 - Re-enable chrome debugging outside of the toolbox

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

The toolbox landing removed the Browser Debugger menu item in favor of a more generic 'Browser Debugging' option that would get all of the tools in the toolbox to target the browser as a whole. However getting other tools besides the JS debugger and the web console to work against chrome will take some time, and even then, only the debugger is plagued with the requirement to launch as a separate process.

For these reasons we decided that it would be best to reinstate the Browser Debugger in its previous form in the meantime, while we work towards getting a more comprehensive solution in place.

Note also that as a workaround, one can already target the browser by launching a separate instance (in a separate profile) and selecting Tools -> Web Developer -> Connect. After connecting to the other browser instance, the list of tabs will be accompanied by a 'Remote Process' option. Clicking on that will open the toolbox with a Global Console and a Browser Debugger.

Comment 1

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

Attachment: WIP

Almost there.

Comment 2

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

Attachment: Patch v2

This is no new code, just a reversal of a small part of the patch for bug 788977 that brings the Browser Debugger back, for the reasons stated in comment 0. It would be nice to land this in fx-team before we merge the toolbox to m-c so that there won't be any nightlies shipped without the Browser Debugger.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 684788 [details] [diff] [review]
Patch v2

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

Looks good. Going to want a try run to make sure this doesn't introduce any leakage.

Also, the changes to browser.js had to be qref'd a little. There was some problem applying the patch.

::: browser/base/content/browser-appmenu.inc
@@ +155,5 @@
>            <menuseparator id="appmenu_devtools_separator"/>
>            <menuitem id="appmenu_devToolbar"
>                      observes="devtoolsMenuBroadcaster_DevToolbar"/>
> +          <menuitem id="appmenu_chromeDebugger"
> +                    observes="devtoolsMenuBroadcaster_ChromeDebugger"/>

I guess you're able to replace the remoteWebConsole entries now because it's take care of by the Connect menu item?

::: browser/base/content/browser-doctype.inc
@@ +19,5 @@
>  #endif
>  <!ENTITY % aboutHomeDTD SYSTEM "chrome://browser/locale/aboutHome.dtd">
>  %aboutHomeDTD;
> +<!ENTITY % debuggerDTD SYSTEM "chrome://browser/locale/devtools/debugger.dtd">
> +%debuggerDTD;

they took it out but you keep putting it back in...

(it's fine. That's what this patch does. I get that, but thought the comment was funny).

::: browser/devtools/debugger/DebuggerPanel.jsm
@@ +93,5 @@
>      this._isReady = true;
>      this.emit("ready");
>    },
>  
>    destroy: function() {

no more destroy? I guess we can't do that with the devtools window patches applied, can we?

Comment 4

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

Attachment: Patch v3

(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Looks good. Going to want a try run to make sure this doesn't introduce any
> leakage.

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

> Also, the changes to browser.js had to be qref'd a little. There was some
> problem applying the patch.

Yup, attaching the rebased version.

> ::: browser/base/content/browser-appmenu.inc
> @@ +155,5 @@
> >            <menuseparator id="appmenu_devtools_separator"/>
> >            <menuitem id="appmenu_devToolbar"
> >                      observes="devtoolsMenuBroadcaster_DevToolbar"/>
> > +          <menuitem id="appmenu_chromeDebugger"
> > +                    observes="devtoolsMenuBroadcaster_ChromeDebugger"/>
> 
> I guess you're able to replace the remoteWebConsole entries now because it's
> take care of by the Connect menu item?

Yes, there is no longer a need for a remote web console entry. I believe it was inadvertently left behind in the big patch and I took the opportunity to clean it up.

> ::: browser/base/content/browser-doctype.inc
> @@ +19,5 @@
> >  #endif
> >  <!ENTITY % aboutHomeDTD SYSTEM "chrome://browser/locale/aboutHome.dtd">
> >  %aboutHomeDTD;
> > +<!ENTITY % debuggerDTD SYSTEM "chrome://browser/locale/devtools/debugger.dtd">
> > +%debuggerDTD;
> 
> they took it out but you keep putting it back in...
> 
> (it's fine. That's what this patch does. I get that, but thought the comment
> was funny).

<3

> ::: browser/devtools/debugger/DebuggerPanel.jsm
> @@ +93,5 @@
> >      this._isReady = true;
> >      this.emit("ready");
> >    },
> >  
> >    destroy: function() {
> 
> no more destroy? I guess we can't do that with the devtools window patches
> applied, can we?

This was actually another bit of cleanup from my leak-fixing patch in the devtools-window repo. The code in destroy() didn't accomplish anything, I just didn't have the time to remove it before merging.

Comment 5

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

Try run was green, but for some reason I omitted optimized builds, so I had to run them separately:

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

Comment 6

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

Since bug 788977 was backed out, I landed the patch in the devtools-window repo, so that it will be in the updated patch that will reland:

https://github.com/joewalker/devtools-window/pull/317

Comment 7

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

Landed in fx-team along with bug 788977:
https://hg.mozilla.org/integration/fx-team/rev/c77b869c2025

Comment 8

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

Landed in m-c:
https://hg.mozilla.org/mozilla-central/rev/c77b869c2025