Closed Bug 814655 Opened 10 years ago Closed 10 years ago

Re-enable chrome debugging outside of the toolbox

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 20

People

(Reporter: past, Assigned: past)

References

Details

(Whiteboard: [chrome-debug][fixed-in-fx-team])

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [chrome-debug]
Blocks: 788977
No longer depends on: 788977
Attached patch WIP (obsolete) — Splinter Review
Almost there.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #684778 - Attachment is obsolete: true
Attachment #684788 - Flags: review?(rcampbell)
Attachment #684788 - Flags: review?(dolske)
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?
Attachment #684788 - Flags: review?(rcampbell) → review+
Attached patch Patch v3Splinter Review
(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.
Attachment #684788 - Attachment is obsolete: true
Attachment #684788 - Flags: review?(dolske)
Attachment #685291 - Flags: review?(dolske)
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
Attachment #685291 - Flags: review?(dolske) → review+
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
Landed in fx-team along with bug 788977:
https://hg.mozilla.org/integration/fx-team/rev/c77b869c2025
Whiteboard: [chrome-debug] → [chrome-debug][fixed-in-fx-team]
Landed in m-c:
https://hg.mozilla.org/mozilla-central/rev/c77b869c2025
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.