Closed Bug 1279834 Opened 3 years ago Closed 3 years ago

Integrate devtools client UI into Thunderbird

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set

Tracking

(thunderbird52+ fixed)

RESOLVED FIXED
Thunderbird 52.0
Tracking Status
thunderbird52 + fixed

People

(Reporter: Fallen, Assigned: Fallen)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Attached patch Patch - v1 (obsolete) β€” β€” Splinter Review
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)
Depends on: 1279784
As the dependencies have landed, should this WIP now be reviewed?
Comment on attachment 8762427 [details] [diff] [review]
Patch - v1

Sure thing, I think this is ready enough. Hope everything works as expected!
Attachment #8762427 - Attachment description: WiP - v1 → Patch - v1
Attachment #8762427 - Flags: review?(mkmelin+mozilla)
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 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?
Attachment #8762427 - Flags: review?(mkmelin+mozilla)
Yes, will take care of this soon. Sorry about that!
Flags: needinfo?(philipp)
Attached patch Patch - v2 (obsolete) β€” β€” Splinter Review
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.
Attachment #8762427 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8794524 - Flags: review?(mkmelin+mozilla)
Attached patch Patch - v3 (obsolete) β€” β€” Splinter Review
And again with missing additions/removals
Attachment #8794524 - Attachment is obsolete: true
Attachment #8794524 - Flags: review?(mkmelin+mozilla)
Attachment #8794526 - Flags: review?(mkmelin+mozilla)
I see we may need to make the same changes that are coming up in bug 1301940. Marking this to depend on bug 1305206.
Depends on: 1305206
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
Attachment #8794526 - Flags: review?(mkmelin+mozilla) → review-
We should try to get this in for 52.
Attached patch Patch - v4 β€” β€” Splinter Review
(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.
Attachment #8794526 - Attachment is obsolete: true
Attachment #8809775 - Flags: review?(mkmelin+mozilla)
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
Attachment #8809775 - Flags: review?(mkmelin+mozilla) → review+
https://hg.mozilla.org/comm-central/rev/f85659ef3cf0f7064d8aad906fd70ee14ea8d9d9
Bug 1279834 - Integrate devtools client UI into Thunderbird. r=mkmelin
I've not looked into the pref value issue in comment 12.
Flags: needinfo?(philipp)
Straight port of bug 1301940.
Attachment #8811240 - Flags: review?(philipp)
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+.
(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.
Flags: needinfo?(philipp)
Attachment #8811240 - Flags: review?(philipp) → review+
(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...
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 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
Attachment #8811240 - Flags: approval-comm-aurora?
Let's close this bug and tackle any regressions/snags in followups.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
Depends on: 1318737
uplift ping ;)
Flags: needinfo?(jorgk)
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.
Flags: needinfo?(jorgk)
Attachment #8811240 - Flags: approval-comm-aurora? → approval-comm-aurora+
(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.
Depends on: 1323389
Depends on: 1427590
You need to log in before you can comment on or make changes to this bug.