274852 - Chatzilla needs a better way to install plugins / scripts

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[:Gijs (he/him)]

Currently, to have scripts or plugins autoload on startup, they need to be (in a
directory which is) listed in the initialScripts preference. This is quite okay,
but starting users have trouble figuring out how to load scripts, and how to
load them by default, especially if the scripts start using more than one js
file. (ChattyTunes being an example of that)
Plugin writers are faced with this problem as there is no uniform way in which
chatzilla plugins should be installed. It is possible to make an xpi install for
the default scripts directory, but not all users want their plugins there, and
it's quite buggy.
In short, a menu item to install a plugin or script would be very nice :-)

Comment 1

[:Gijs (he/him)]

Silver created a plugin for this quite some time ago. I've found it very useful,
and I'm wondering if this will ever get into CZ (it shouldn't be *that* hard,
I'd imagine).
If Silver wouldn't want to do it himself, I'd volunteer to do so as soon as my
test week is  over. ;-)

Adding CC.

Comment 2

[:Gijs (he/him)]

Attachment: Patch, sort of

This is basically Silver's plugin but embedded into the chrome. I hope the hacks, uh, I mean, careful changes, I've made to make that happen are OK. I decided to put the js+xul file in a subdir, but in hindsight maybe that's not the best thing to do. (Ideally I would really like the channel, config, plugin, network and output-window things to all be in separate directories, to keep some organization, but maybe that's overkill)

And yes, I would ideally like to have a real plugin management thing as well (for enabling/disabling/uninstalling/reloading plugins, possibly for writing new ones), but I think this is a good start and we should take it to simplify our plugin story at least a little bit.

r? on James cause it's his code I'm hacking
r? on Samuel (soon) cause we need someone other than Silver and me to look at it.

Comment 3

[James Ross]

Comments on the attachment:
- There are two slightly different messages with the same name of
  msg.install.plugin.err.download.
- The code seems to use MSG_IP_ and MSG_INSTALL_PLUGIN_ interchangeably,
  though all the messages are defined with the longer form.
- I think the code for the dialog could do with a rewrite, in particular
  because I don't think it needs the client.newEvent indirection when it is
  part of ChatZilla (IIRC that part was a hack to gain the dialog more
  permissions).
- The <window> should probably be a <dialog> to get the standard OK/Cancel
  buttons and behaviours.

Comment 6

[:Gijs (he/him)]

Attachment: Better patch

Updated, no more destination files, fixed all the nits you mentioned, I think...

Comment 7

[James Ross]

Comment on attachment 388140 [details] [diff] [review]
Better patch

>+function getTempFile(path, name)
>+{
>+    var tempFile = new nsLocalFile(path);
>+    tempFile.append(name);
>+    tempFile.createUnique(0, 0600);
>+    return tempFile;
>+}
>+
>+

Nit: only one blank line needed.

>+cmd.install-plugin.params = [<url> [<name>]]
>+cmd.install-plugin.help   = Installs a ChatZilla plugin for you.
>+cmd.install-plugin.label  = Install Plugin…
>+cmd.install-plugin.accesskey = I

Use label = &Install Plugin… instead of the separate accesskey.

>+# Plugin installation
>+msg.install.plugin.err.download     = An error occured downloading the plugin: %S"

s/occured/occurred/ throughout, probably my mistake originally, I hate that word. :)

>+<!ENTITY windowtitle                    "Install Plugin...">

I think these should use the Unicode "…" thing, probably worth checking with someone as I can't see any examples in ChatZilla.

>+<!ENTITY browse.label                   "Browse...">

Same as windowtitle.

>+<!ENTITY ok.label                       "OK">
>+<!ENTITY ok.accesskey                   "O">
>+<!ENTITY cancel.label                   "Cancel">
>+<!ENTITY cancel.accesskey               "C">

Are these used? The dialog is a <dialog> so has its own OK and Cancel buttons.

>+        onStopRequest: function _onStopRequest(request, context, statusCode)
>+        {
>+            ctx.outFileH.close();
>+            

Nit: trailing whitespace.

>+                display(getMsg(MSG_INSTALL_PLUGIN_ERR_REMOVE_TEMP, ex),
>+                        MT_ERROR);
>+            }
>+        }
>+    };
>+    

Nit: trailing whitespace.

>+    if (!e.url)
>+    {
>+        window.openDialog(ipURL, "_blank", "dialog", client);

The trend seems to be "" for the second argument and "chrome,dialog" for the third.

>+        return;
>+    }
>+
>+    if (!e.name)
>+    {
>+        var ary = e.url.match(/([^\/]+?)((\..{0,3}){0,2})$/);

We do this twice, perhaps a local variable for the answer? Not fussed, though.

>+        if (ary)
>+            e.name = ary[1];
>+        else
>+            display(MSG_INSTALL_PLUGIN_ERR_NO_NAME, MT_ERROR);

Error but no return? That seems wrong.

>+        default:
>+            display(MSG_INSTALL_PLUGIN_ERR_PROTOCOL, MT_ERROR);
>+    }
>+}
>+
>+

Nit: only one blank line needed.

>+    function getZipEntry(reader, entryEnum)
>+    {
>+        var item = entryEnum.getNext();
>+        // nsIZipReader was rewritten...
>+        if (typeof item == "string")
>+            return reader.getEntry(item).QueryInterface(nsIZipEntry);

item = reader.getEntry(item);

Then we don't duplicate the QI code.

>+    if (!dest) {
>+        display(MSG_INSTALL_PLUGIN_ERR_INSTALL_TO, MT_ERROR);
>+        return;
>+    }
>+    usePluginNameInPath = true;

We don't appear to |var| 'dest' anywhere and usePluginNameInPath is always going to be 'true'. The latter can go, making sure to assume it is true when removing the 'if' later, and dest needs to be var'ed.

>+        if (typeof dest == "string")
>+            dest = getFileFromURLSpec(dest);

I'm not sure we'll reach this code path od typeof dest == "string" either. This must come from previously having 'dest' as an argument, but it isn't any more so cleanup time.

>+    if (source.path.match(/\.(jar|zip)$/i))

This is one bit I never liked, I don't suppose there's a good solution? MIME Service sniffing? Leave it unless you know easy improvements.

>+            //XXXgijs: this is fragile. Anyone got a better idea?
>+            ary = initJSData.match(/plugin\.id\s*=\s*(['"])(.*?)(\1)/);

We could subscript load it with the special sandbox scope things (possibly not supported too far back?).

>+            if (dest.exists())

This is a cheap test, but we won't be able to upgrade plugins. Follow-up work needed there.

>+                    var dirs = item.name.split("/");
>+                    // Remove common path if there is one.
>+                    if (zipPathBase)
>+                    {
>+                        dirs = item.name.substring(zipPathBase.length);
>+                        dirs = dirs.split("/");
>+                    }

var dirs = item.name;
if (zipPathBase)
    dirs = dirs.substring(zipPathBase.length);
dirs = dirs.split("/");

Slightly shorter and less splitting.

>+                    // Construct the full path for the extracted file...
>+                    var zipFile = dest.clone();
>+                    for (var i = 0; i < dirs.length; i++)
>+                    {
>+                        zipFile.append(dirs[i]);
>+                        if (i < dirs.length - 1)
>+                        {
>+                            // Creating dirs as needed.
>+                            if (!zipFile.exists())
>+                                zipFile.create(DIRECTORY_TYPE, 0700);
>+                        }
>+                    }

Maybe neater to do for (var i = 0; i < dirs.length - 1; i++) and then zipFile.append(dirs[dirs.length - 1]) after the loop. The comment in the middle is a little pointless too.

>+            var rv = dispatch("load " + getURLSpecFromFile(destInit));

We should totally be using the object style: dispatch("load", { url: ... });. If it fits, anyway. :) Same for .js files below.

>+        return;
>+    }
>+
>+    if (source.path.match(/\.(js)$/i))

The return 3 lines up can be removed and this made into an |else if|, I think?

>+        catch(ex)
>+        {
>+            if (ex != CZ_PI_ABORT)
>+            {
>+                display(getMSg(MSG_INSTALL_PLUGIN_ERR_INSTALLING, ex),
>+                        MT_ERROR);
>+            }
>+            return;

No need to return, we're at the end.

>+        display(MSG_INSTALL_PLUGIN_ERR_FORMAT, MT_ERROR);
>+        return;

No need to return, we're at the end.

>+client.newEvent =
>+function cli_newEvent(set, type, destObject, destMethod)
>+{
>+    return new CEvent(set, type, destObject, destMethod)
>+}

Never called, kill it.

>+function changeAutoName(event)
>+{
>+    var useAutoName = document.getElementById("chk-name-auto");
>+    var pluginName = document.getElementById("txt-name");
>+    if (useAutoName.checked)
>+    {
>+        if (document.getElementById("txt-name").value != "")

pluginName.value != ""

>+            pluginName.setAttribute("readonly", "true");
>+        else
>+            pluginName.setAttribute("disabled", "true");

Do we have any idea why it does this? Seems a bit weird to swap between readonly and disabled based on its value, and it's surely not going to sit well with other code updating the value (e.g. sourceChange).

>+function browseForSource(event)

>+    window.focus();

This should be unnecessary as pickOpen will parent the File Open dialog on this window, AFAICT.

>+function doOK(event)
>+{

Might as well move the document.getElementById calls to the top here, like the other functions.

>+    if (!document.getElementById("txt-name").value)
>+    {
>+        alert("The plugin name must be specified!");

Holy non-localised string, Batman!

>+    var ev = new Object();
>+    ev.name = document.getElementById("txt-name").value;
>+    ev.url = document.getElementById("txt-source").value;
>+    client.dispatch("install-plugin", ev);

Could use an inline object literal here, especially with d.gEBI moved up top.

We should CEIP-enable this dialog, too, like config/channels are. I don't think we should log the plugin name, but we should log dialog open and close (with OK vs Cancel). We could also log /install-plugin success or failure.

r=silver with appropriate changes or explanations.

Plenty of room for future expansion, too. :)

Comment 8

[:Gijs (he/him)]

Attachment: Updated patch w/ nits addressed and nsIZip* suckage dealt with

Woe be unto me. A lot of the previous patch now actually doesn't work anymore on latest Fx 3.5 because they changed the nsIZipReader/Entry APIs *still more*. Gah. I've added extra code to deal with this, and I've fixed all your nits, I believe. We can file followup bugs for mime detection of jar/zips, CEIP, and using sandboxes to extract the plugin id. I'll attach an interdiff after putting this one up, in the hope that that'll make it easier to review again.

Comment 9

[:Gijs (he/him)]

Attachment: Interdiff

Comment 10

[James Ross]

Comment on attachment 416255 [details] [diff] [review]
Updated patch w/ nits addressed and nsIZip* suckage dealt with

>diff --git a/xul/content/install-plugin/install-plugin.js b/xul/content/install-plugin/install-plugin.js
>+function doOK(event)

>+        client.mainWindow.MSG_INSTALL_PLUGIN_ERR_SPEC_NAME;

You meant to do alert(...) here, right?

>diff --git a/xul/content/static.js b/xul/content/static.js
>+    function getZipEntry(reader, entryEnum)

>+        return [itemName, item.QueryInterface(nsIZipEntry)];

You only ever seem to use the name (item [0]) from this function's return, so I'd suggest just returning the itemName alone.

The interdiff looks good, so with those two things fixed, r=silver.

Comment 11

[:Gijs (he/him)]

http://hg.mozilla.org/chatzilla/rev/93c4d89849b6

Comment 12

[James Ross]

The alert() doesn't work (it's imported from utils.js which needs MSG_ALERT at least in scope). Patch in a sec.

Comment 13

[James Ross]

Attachment: MSG_ALERT is now defined

I've found while testing this that the events doOK and doCancel are completely wrong for a <dialog>. Try opening it and clicking OK (with this patch): you get the alert, then the dialog closes.

Comment 14

[:Gijs (he/him)]

Comment on attachment 417107 [details] [diff] [review]
MSG_ALERT is now defined

r=me, 

> Created an attachment (id=417107) [details]
> MSG_ALERT is now defined
> 
> I've found while testing this that the events doOK and doCancel are completely
> wrong for a <dialog>. Try opening it and clicking OK (with this patch): you get
> the alert, then the dialog closes.

Gah. return false; should fix it, according to MDC. If it does, can you include that in this patch?

With that, or failure to work according to docs, r=me.

Comment 15

[James Ross]

I had to add a "return" to the XUL event attribute too, but it did work. Pushed --> (re)FIXED.