1279834 - Integrate devtools client UI into Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: Patch - v1

Right now we are using the server only, and since bug 1243550 part of the client. This bug aims to go a few steps further and use the remaining tools (that make sense to use).

I'm going to need some devtools changes to make it happen, please see dependent bugs. Aside from that, the mail/ patch is already looking pretty good. I've attached a work in progress in case you want to give it a spin (needs m-c patches to work of course)

Comment 1

[aleth [:aleth]]

As the dependencies have landed, should this WIP now be reviewed?

Comment 2

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 8762427 [details] [diff] [review]
Patch - v1

Sure thing, I think this is ready enough. Hope everything works as expected!

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 8762427 [details] [diff] [review]
Patch - v1

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

Not sure why, but the patch won't apply for me

Hunk #1 FAILED at 810
1 out of 1 hunks FAILED -- saving rejects to file mail/app/profile/all-thunderbird.js.rej
patching file mail/base/content/mailCore.js
Hunk #1 FAILED at 317
1 out of 1 hunks FAILED -- saving rejects to file mail/base/content/mailCore.js.rej
patching file mail/base/content/mailWindowOverlay.js
Hunk #1 FAILED at 2
1 out of 1 hunks FAILED -- saving rejects to file mail/base/content/mailWindowOverlay.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh devtools-toolbox.diff

::: mail/base/content/mailCore.js
@@ +328,5 @@
>  
> +function openAboutDebugging(hash)
> +{
> +  let url = "about:debugging" + (hash ? "#" + hash : "");
> +  document.getElementById('tabmail').openTab("contentTab", { contentPage: url });

nit: please be consistent with the quote types (prefer double)

::: mail/components/devtools/XULRootActor.js
@@ +63,5 @@
> +                          .getService(Ci.nsIURLParser);
> +        let baseName, bnpos = {}, bnlen = {};
> +        let path = aWindow.location.pathname;
> +        urlParser.parseFilePath(path, path.length, {}, {}, bnpos, bnlen, {}, {});
> +        baseName = path.substr(bnpos.value, bnlen.value);

nit: would declare baseName here instead of above since that's where it's first needed

@@ +70,5 @@
> +
> +    return getMainWindows().map((win) => {
> +        return win.document.documentElement.getAttribute("windowtype");
> +    });
> +}

nit (for the whole function): please use 2 space indention

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8762427 [details] [diff] [review]
Patch - v1

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

Could you attach a patch that applies cleanly to tip?

Comment 5

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Yes, will take care of this soon. Sorry about that!

Comment 6

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: Patch - v2

Here we go. I had to remove the Eyedropper functionality, it seems even Firefox no longer allows to use this on XUL windows. I've also applied some fixes to make opening the Learn More link possible in toolbox windows.

Comment 7

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: Patch - v3

And again with missing additions/removals

Comment 8

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

I see we may need to make the same changes that are coming up in bug 1301940. Marking this to depend on bug 1305206.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8794526 [details] [diff] [review]
Patch - v3

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

The app seems slow to run, but I'm not sure if it's because of this patch or not. 

The Developer Toolbox behaves a bit strangely. If a toolbox is already open it should preferably just focus the existing one - I thought it wasn't working at all with the current behaviour, since it was opened but I never noticed where the window went due to the alert about remote debugging stealing focus shortly after it opened.

The App menu menu entries are also missing.

::: mail/base/content/contentAreaClick.js
@@ +194,5 @@
> + * this to the external browser for now, since in most cases this is meant to
> + * open an actionable tab.
> + */
> +function openUILinkIn(url, where, options) {
> +    openLinkExternally(url);

nit: 2 space indent

::: mail/components/devtools/tb-root-actor.js
@@ +38,5 @@
> +  return rootActor;
> +}
> +createRootActor.isMailRootActor = true;
> +
> +function getChromeWindowTypes() {

nit: 2 space indent in this function too please

@@ +77,5 @@
> +/**
> + * Returns the window type of the passed window.
> + */
> +function appShellDOMWindowType(aWindow) {
> +  /* This is what nsIWindowMediator's enumerator checks. */

nit: // style comment please

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +495,5 @@
>  
> +<!-- Developer Tools Submenu -->
> +<!ENTITY devtoolsMenu.label "Developer Tools">
> +<!ENTITY devtoolsMenu.accesskey "o">
> +<!ENTITY devToolboxCmd.label "Open Developer Toolbox">

Not sure why this entry would have "Open" in front. The other entries open things too

@@ +499,5 @@
> +<!ENTITY devToolboxCmd.label "Open Developer Toolbox">
> +<!ENTITY devToolboxCmd.accesskey "T">
> +<!ENTITY devToolboxCmd.commandkey "i">
> +<!ENTITY addonDebugCmd.label "Add-on Debugger">
> +<!ENTITY addonDebugCmd.accesskey  "A">

nit: double space

@@ +501,5 @@
> +<!ENTITY devToolboxCmd.commandkey "i">
> +<!ENTITY addonDebugCmd.label "Add-on Debugger">
> +<!ENTITY addonDebugCmd.accesskey  "A">
> +<!ENTITY tabsDebugCmd.label "Content Frame Debugger">
> +<!ENTITY tabsDebugCmd.accesskey  "C">

nit: double space

Comment 10

[Magnus Melin [:mkmelin]]

We should try to get this in for 52.

Comment 11

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: Patch - v4

(In reply to Magnus Melin from comment #9)
> The app seems slow to run, but I'm not sure if it's because of this patch or
> not.
I don't think it is anything specific to this patch, but it may be devtools in general or the fact that a new Thunderbird process is opened.

> The Developer Toolbox behaves a bit strangely. If a toolbox is already open
> it should preferably just focus the existing one - I thought it wasn't
> working at all with the current behaviour, since it was opened but I never
> noticed where the window went due to the alert about remote debugging
> stealing focus shortly after it opened.
This is the same way in Firefox. Given the toolbox is a new Thunderbird process instead of a simple window, we can't just focus this without toolkit changes. I'd suggest filing a bug in Firefox Devtools for this.


> The App menu menu entries are also missing.
Fixed, along with the other nits.

Hope we can get at least the strings in for the merge on Monday.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8809775 [details] [diff] [review]
Patch - v4

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

Works very nicely, thx! r=mkmelin

::: mail/app/profile/all-thunderbird.js
@@ +808,5 @@
>  
>  // Developer Tools related preferences
>  pref("devtools.debugger.log", false);
>  pref("devtools.chrome.enabled", true);
> +pref("devtools.debugger.remote-enabled", true);

Any reason not to keep this the default (false) like Firefox?

In particular, wouldn't this expose "debugging" of other peoples instances in multi-user systems, by default? With or without it set to true I do get the dialog asking to confirm incoming remote connection though.

::: mail/components/devtools/content/dbg-messenger-overlay.xul
@@ -13,5 @@
> -    <menuitem id="devtoolsDebugger"
> -              type="checkbox"
> -              insertafter="menu_inspector,javascriptConsole,javaScriptConsole"
> -              label="&allowRemoteDebugging.label;"
> -              accesskey="&allowRemoteDebugging.accesskey;"

allowRemoteDebugging.label etc are still in the dtd

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/f85659ef3cf0f7064d8aad906fd70ee14ea8d9d9
Bug 1279834 - Integrate devtools client UI into Thunderbird. r=mkmelin

Comment 14

[aleth [:aleth]]

I've not looked into the pref value issue in comment 12.

Comment 15

[aleth [:aleth]]

Doesn't this need something like https://hg.mozilla.org/releases/comm-aurora/rev/b55b11a021eacc22460a25708e9d3584279277b2 ?

Comment 16

[aleth [:aleth]]

Attachment: Add devtools-client to l10n.ini

Straight port of bug 1301940.

Comment 17

[aleth [:aleth]]

It seems the devtools console is currently broken (at least it just opens to a blank window in my local build), so something must have changed since it got r+.

Comment 18

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

(In reply to Magnus Melin from comment #12)
> >  // Developer Tools related preferences
> >  pref("devtools.debugger.log", false);
> >  pref("devtools.chrome.enabled", true);
> > +pref("devtools.debugger.remote-enabled", true);
> 
> Any reason not to keep this the default (false) like Firefox?
In Firefox, there is UI in the docked developer tools for a website to enable browser debugging, which is also disabled by default. Browser debugging requires remote debugging. Given we have no such UI and expect debugging to be enabled by default, we need to have remote-enabled set.

I guess this would open the port each time Thunderbird starts, so we could consider adding a checkmark item in the menu that when enabling unhides the remaining menuitems.

I'll see if I can reproduce the blank window issue. I know on mac sometimes remote debugging fails with NS_ERROR_CONNECTION_REFUSED the first time, but magically it works the second time just after. Same on Firefox.

Comment 19

[aleth [:aleth]]

(In reply to Philipp Kewisch [:Fallen] from comment #18)
> I'll see if I can reproduce the blank window issue. I know on mac sometimes
> remote debugging fails with NS_ERROR_CONNECTION_REFUSED the first time, but
> magically it works the second time just after. Same on Firefox.

I've never seen NS_ERROR_CONNECTION_REFUSED anywhere, but sometimes you do get an additional dialog asking to accept an incoming connection. If you do so, the console works fine after. It's very confusing though...

Comment 20

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/00d54e057f2f8685381ee88a5d48476ad4208e6e
Bug 1279834 - Add devtools-client to l10n.ini. r=philipp a=bustage-fix CLOSED TREE DONTBUILD

Comment 21

[aleth [:aleth]]

Comment on attachment 8811240 [details] [diff] [review]
Add devtools-client to l10n.ini

[Approval Request Comment]
Regression caused by (bug #): devtools client support landed in 52

Comment 22

[aleth [:aleth]]

Let's close this bug and tackle any regressions/snags in followups.

Comment 23

[aleth [:aleth]]

uplift ping ;)

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8811240 [details] [diff] [review]
Add devtools-client to l10n.ini

I didn't see this since my query looks at Target Milestone 53 to uplift to Aurora 52. You managed to land two parts on two different versions, hmm.

Anyway:
Aurora (TB 52) for the second part:
https://hg.mozilla.org/releases/comm-aurora/rev/59836fa2fa53015022f5680a7539a6f16407181e

Did you want this build? Well, the next blocklist update will build it for you.

Comment 25

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #24)
> Comment on attachment 8811240 [details] [diff] [review]
> Add devtools-client to l10n.ini
> 
> I didn't see this since my query looks at Target Milestone 53 to uplift to
> Aurora 52. You managed to land two parts on two different versions, hmm.

Yes, Fallen's r+ patch had strings that had to go in before the merge.