884805 - Figure out if/how to best package the debugger as an extension

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Right now we have pushed the debugger code to mail/components/debugger. This means we won't be able to use it from Seamonkey. So the next idea was to put it into mailnews/. But what about other applications?

So the next good idea is to make it an extension. This would allow us to support all gecko apps at once! A few options we need to discuss here:

Option A: package extension as an extension within the app
 - Here we would have a directory in the build system that creates the debugger
   server glue as an extension
 - The extension would be installed and distributed with Thunderbird, similar to
   how Seamonkey packages Chatzilla et al.
 - The extension can be extracted from Thunderbird for uploading to AMO.

Option B: package files directly in app, extra extension directory
 - The extension files get installed into dist/bin, not in the extensions dir
 - This way the extension is installed natively, without an addon-manager entry
 - An extra directory exists in the build system to generate the same code as
   an extension, for uploading to AMO

If Seamonkey wants to package it too, code should move to mailnews/ in both cases.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: WIP Extension - v1

This (no-restart!) extension will allow you to enable the debugger server. Code is of course not complete, strings are hardcoded, no cleanup yet, etc. To use it, install and open the preferences dialog. If the Status says not supported, then you need to use a newer version of Thunderbird/Seamonkey, something failed loading the dbg-server module from devtools. Otherwise you can start the server using the button. Use a Firefox 24 nightly to connect.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

Some IRC discussion with mconley brought up yet another option:

Option C: package as an extension only
 - The code would be removed from comm-central altogether, instead added to my
   github account
 - I will take care of uploading to AMO periodically
 - MDC and friends should mention the extension
 - Seamonkey can package the extension with their builds, if they like


Upside is that we don't have any extra work for packaging and uploading, there will be no users that complain that an extension with a dubious name like "Remote Debugging Server" is auto-installed into their profile.

Downside is localization needs to be managed externally, visibility is not as high, version incompatibilities are harder to manage. Seamonkey might be more complicated since its yet another external repository and github is not hg.

Comment 3

[Alex Vincent [:WeirdAl]]

I'd love to help out with this; an extension that makes a XULRunner app into a debuggee is extremely important to me.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Do you have a XULRunner app that works on Gecko 24? I've been having trouble getting one running under my OSX installation. Could you check the attached extension and see if it makes it possible to connect to your xulrunner app?

Comment 5

[Alex Vincent [:WeirdAl]]

Unfortunately, XULRunner on MacOS is definitely broken.  On Mac, XR requires using --install-app, and the executable applications it generates can't launch.  See bug 448069 comment 4, and bug 747597 for some background.

What I end up doing on Mac is requiring a Firefox installation, and then using -app to launch the executable.  For all intents and purposes, this causes FF to behave like a XULRunner app (including application ID, etc.).

I do have some XR apps in progress, yes, combining the Jasmine test framework (http://pivotal.github.com/jasmine ) with XULRunner.

hg clone http://hg.code.sf.net/p/verbosio/jasmine/code jasmine
cd jasmine
python project.py --update-sdk=aurora # downloads XULRunner from ftp
python project.py test-xul

That should almost get you going, though for some reason it's failing on my Windows machine right now.  For Mac, you'll need another line:

python project.py --browser (/path/to/Nightly.app/Contents/MacOS/firefox)

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Update: I have cleaned up the extension a bit so it would be ready for general use. For now I'm going to put it on my github repository so it can be improved.

Alex, since you probably have a better grip on your projects, could you do the testing yourself? From a functional perspective, the wip extension should work just as well.

I will post the github link as soon as I upload. Another thing we would need to do is figure out what to do with the code that was already committed. I guess we could back out the code easily on affected branches.

Comment 7

[Alex Vincent [:WeirdAl]]

Honestly, it might take me some time, as I have summer college courses (and now a local transportation union strike) to deal with.  Weekends are best.

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Trying to get a more broad opinion here: https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/oHwY42gCinI

I'm going to target Seamonkey and Thunderbird for now, I believe that xulrunner apps will just be a matter of minor bugfixes.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

It seems there are more votes for the in-app variant, see this post:

https://groups.google.com/d/msg/mozilla.dev.apps.thunderbird/oHwY42gCinI/UOHzhkkFU3AJ

This means I will take care of adding the extension/ directory in this bug.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Extension Packaging - v1

Ok, this should do it. Quite a bit of changes, but this makes it possible to package both in-app and as an extension.

I've also taken the liberty to move from mail/components/debugger to mail/components/devtools, since this is not just the debugger.

This patch applies on top of the patches in bug 880930 and bug 897476.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Extension Packaging - v1

Comment 12

[Alex Vincent [:WeirdAl]]

Comment on attachment 804910 [details] [diff] [review]
Extension Packaging - v1

>diff --git a/mail/components/devtools/extension/moz.build b/mail/components/devtools/extension/moz.build
>new file mode 100644
>--- /dev/null
>+++ b/mail/components/devtools/extension/moz.build

Peanut gallery observation:  This would still require someone download comm-central to build the extension.  Is there any reasonable chance of moving this into mozilla-central/extensions, or perhaps a separate mini-repository like http://hg.mozilla.org/ipccode/ ?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 804910 [details] [diff] [review]
Extension Packaging - v1

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

This looks great, Philipp! Just a few style nits, but nothing major. Great job!

::: mail/components/devtools/extension/bootstrap.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function startup(data, reason) {

aData, aReason

@@ +5,5 @@
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function startup(data, reason) {
> +  // Register the resource:// location
> +  let resource = Services.io.getProtocolHandler("resource")

Nit - I generally prefer:

let resource = Services.io
                       .getProtocolHandler("resource")
                       .QueryInterface(blah)

Also, using Cc, Ci and Cu could terse this up a bit.

@@ +19,5 @@
> +  };
> +  RemoteDebuggerServer.startstop(remoteEnabled);
> +}
> +
> +function shutdown(data, reason) {

aData, aReason

@@ +31,5 @@
> +  // Unload our debug server
> +  Components.utils.unload("resource://dbgserver/modules/RemoteDebuggerServer.jsm");
> +
> +  // Unregister the dbgserve resource:// location
> +  let resource = Services.io.getProtocolHandler("resource")

Nit - I generally prefer:

let resource = Services.io
                       .getProtocolHandler("resource")
                       .QueryInterface(blah)

@@ +36,5 @@
> +                         .QueryInterface(Components.interfaces.nsIResProtocolHandler);
> +  resource.setSubstitution("dbgserver", null);
> +}
> +
> +function install(data, reason) {

aData, aReason. And this could be a one-liner, no?

function install(aData, aReason) {}
function uninstall(aData, aReason) {}

::: mail/components/devtools/extension/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXTRA_JS_MODULES += ["../modules/RemoteDebuggerServer.jsm", "../modules/XULRootActor.js"]

Please break over two lines, like:

EXTRA_JS_MODULES += ["../modules/RemoteDebuggerServer.jsm",
                     "../modules/XULRootActor.js"]

::: mail/components/devtools/modules/RemoteDebuggerServer.jsm
@@ +52,5 @@
> +/**
> + * The Frontend for the remote debugger, starts, stops and initializes the
> + * actors.
> + */
> +var RemoteDebuggerServer = {

let, not var. I'm pretty anal about it - even for globals. :)

@@ +77,5 @@
> +    }
> +
> +    return this._chromeWindowTypes || [];
> +  },
> +  set chromeWindowTypes(v) this._chromeWindowTypes  = v,

Nit - extra space before =

@@ +86,5 @@
> +   * callers changing the DebuggerServer's function, so it should be used
> +   * with caution.
> +   */
> +  get onConnectionChange() DebuggerServer.onConnectionChange,
> +  set onConnectionChange(func) {

aFunc

::: mail/components/devtools/moz.build
@@ +4,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ["extension"]
> +
> +EXTRA_JS_MODULES += ["modules/RemoteDebuggerServer.jsm", "modules/XULRootActor.js"]

Please break the list elements over two lines, like:

EXTRA_JS_MODULES += ["modules/RemoveDebuggerServer.jsm",
                     "modules/XULRootActor.js"]

::: mail/locales/en-US/chrome/messenger/devtools/dbgserver.properties
@@ +7,5 @@
> +options.listening.tooltip=The debugger server is running and waiting for connections.
> +options.idle.label=Not Running
> +options.idle.tooltip=The debugger server is not running. You can start it from this dialog.
> +options.unsupported.label=Unsupported
> +options.unsupported.tooltip=There was an error loading the built-in debugger server. Make sure it is packaged and check your error console for messages

Missing a period at the end.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Alex Vincent [:WeirdAl] from comment #12)
> Comment on attachment 804910 [details] [diff] [review]
> Extension Packaging - v1
> 
> >diff --git a/mail/components/devtools/extension/moz.build b/mail/components/devtools/extension/moz.build
> >new file mode 100644
> >--- /dev/null
> >+++ b/mail/components/devtools/extension/moz.build
> 
> Peanut gallery observation:  This would still require someone download
> comm-central to build the extension.  Is there any reasonable chance of
> moving this into mozilla-central/extensions, or perhaps a separate
> mini-repository like http://hg.mozilla.org/ipccode/ ?

This would mean Thunderbird needs to check out the extra repository to build, which we decided against in the newsgroups. I will be releasing latest versions of the extension on addons.mozila.org, possibly I can also have the TB build automation build this extension on a nightly basis.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

I've fixed the nits and also an untranslated string and some wording in the strings. Push is imminent, thanks for the review!

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Extension packaging - v2

Patch for checkin, which is metered atm.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Bitrotted. Please rebase :)

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Are you sure? it applies cleanly for me, it might have required bug 897476 to apply, which is now checked in.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/9791b8bc1e64