410377 - ChatZilla is not compatible with Songbird

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[Mitchell Field [:Mitch]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: ChatZilla 0.9.79-2007122412

Songbird is built on the Mozilla platform, ChatZilla runs on the Mozilla platform, but ChatZilla is not compatible with Songbird.

Reproducible: Always

Steps to Reproduce:
1. Attempt to install ChatZilla as a Songbird add-on.
Actual Results:  
ChatZilla won't install into Songbird.

Expected Results:  
ChatZilla should have installed into Songbird.

Comment 1

[Mitchell Field [:Mitch]]

Attachment: Modified ChatZilla nightly build with unchanged files removed

I have made some initial changes to ChatZilla, which allow it to install, make the menu item appear and fix the error message when ChatZilla is launched. I also added a necessary function which seems to be missing from Songbird.

Comment 2

[Mitchell Field [:Mitch]]

Attachment: Patch v1.0: Initial support for Songbird.

I've now made a patch against the ChatZilla trunk. Disregard the hacked installer files that I submitted earlier.

Comment 3

[:Gijs (he/him)]

Can someone explain me *why* we need to support Songbird? Why would one possibly want to run ChatZilla on Songbird as opposed to as a standalone program, in particular because Songbird already ships a version of XULRunner that one might use for this purpose?

Yes, we could add support for every xulrunner app on the planet, but the result would be an unsupportable mess of code that has so many dependencies on various applications that it would become an even more hideous nightmare to test and expand upon than it already is, whenever we touch something that might not be supported on the application the user randomly decided they wanted to install ChatZilla in.

Comment 4

[Mitchell Field [:Mitch]]

Attachment: Patch v2.0: Full Songbird compatibility (breaks other applications)

This patch, along with images created by wormsxulla, were part of my unofficial effort to port ChatZilla to Songbird. The patch is only provided as a reference, as it breaks support for other applications, and is differently themed.

Comment 5

[Mitchell Field [:Mitch]]

Attachment: Graphics

wormsxulla's user mode graphics replacements.

Comment 6

[Mitchell Field [:Mitch]]

Attachment: Symbols

wormsxulla's user mode symbol replacements.

Comment 7

[Mitchell Field [:Mitch]]

Attachment: Patch v1.1

This patch allows ChatZilla to install and run in Songbird, although some relatively minor bugs are unresolved. It also allows the ChatZilla output window to be used inside a Songbird pane, with or without another pane for the userlist. The pane support was written by Mook.

Comment 8

[:Gijs (he/him)]

Comment on attachment 299575 [details] [diff] [review]
Patch v1.1

General thing: please don't use hard tabs, but four spaces a tab. See 


>? sb
>? xul/content/sb
You should add these files to the tree using cvsdo add, ideally speaking. I'm not sure your makeshift patch combination approach will be apply-able.

<snip>

>Index: xpi/makexpi.sh
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xpi/makexpi.sh,v
>retrieving revision 1.15
>diff -d -p -u -6 -w -r1.15 makexpi.sh
>--- xpi/makexpi.sh	21 Dec 2007 15:17:08 -0000	1.15
>+++ xpi/makexpi.sh	27 Jan 2008 00:00:00 -0000
>@@ -200,12 +200,14 @@ cd "$CONFIGDIR"
> echo -n .
> 
> safeCommand $PERL make-jars.pl -v -z zip -p preprocessor.pl -s "$FEDIR" -d "$JARROOT" '<' "$FEDIR/jar.mn"
> echo -n .
> safeCommand $PERL make-jars.pl -v -z zip -p preprocessor.pl -s "$FEDIR/sm" -d "$JARROOT" '<' "$FEDIR/sm/jar.mn"
> echo -n .
>+safeCommand $PERL make-jars.pl -v -z zip -p preprocessor.pl -s "$FEDIR/sm" -d "$JARROOT" '<' "$FEDIR/sb/jar.mn"
Pretty sure you want $FEDIR/sb there.

<snip>

Please remove the Songbird display support from this patch and add it in a separate patch. It should be perfectly possible to launch ChatZilla separately, just like you do on Firefox. The display pane support is much trickier and I'd much prefer to review it separately. r- on that, sorry.


>Index: xul/content/chatzillaOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/content/chatzillaOverlay.js,v
>retrieving revision 1.2
>diff -d -p -u -6 -w -r1.2 chatzillaOverlay.js
>--- xul/content/chatzillaOverlay.js	19 Apr 2000 21:39:59 -0000	1.2
>+++ xul/content/chatzillaOverlay.js	27 Jan 2008 00:00:00 -0000
>@@ -1,7 +1,14 @@
> function toIRC() 
> {
>+	// toOpenWindowByType is not implemented in Songbird
>+	if (!toOpenWindowByType)
>+	{
>+		var toOpenWindowByType = function(inType, uri)

Nit: add a function name, please.

You don't actually use inType. Why not? Can't you reuse the Fx or SM implementation of this function?

>+		{
>+			var winopts = "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar";

This line is probably too long. See http://www.hacksrus.com/~ginda/pedant.html

>+			window.open(uri, "_blank", winopts);
>+		}
>+	}
> 
> 	toOpenWindowByType("irc:chatzilla", "chrome://chatzilla/content/chatzilla.xul");
>-
> }
>-

Nit: don't remove empty line after the function.

<snip>
>Index: xul/content/static.js
> <snip>
>@@ -453,12 +456,16 @@ function initApplicationCompatibility()
>         }
>         else if (url == "chrome://browser/content/browser.xul")
>         {
>             client.hostCompat.needToCopyIcons = true;
>             client.host = "Firefox";
>         }
>+        else if (url == "chrome://songbird/content/xul/mainScriptsOverlay.xul")
>+        {
>+            client.host = "Songbird";
>+        }

Like I said before, you don't need this check. There are no songbird versions without nsIXULAppInfo.

>         else
>         {
>             client.host = ""; // We don't know this host. Show an error later.
>             client.unknownUID = url;
>         }
>     }
>Index: xul/skin/contents.rdf
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/skin/contents.rdf,v
>retrieving revision 1.2
>diff -d -p -u -6 -w -r1.2 contents.rdf
>--- xul/skin/contents.rdf	6 Dec 2003 00:27:12 -0000	1.2
>+++ xul/skin/contents.rdf	27 Jan 2008 00:00:00 -0000
>@@ -18,17 +18,23 @@
>       </RDF:Seq>
>     </chrome:packages>
>   </RDF:Description>
> 
>   <RDF:Seq about="urn:mozilla:stylesheets">
>     <RDF:li resource="chrome://browser/content/browser.xul"/>
>+    <RDF:li resource="chrome://songbird/content/xul/mainScriptsOverlay.xul"/>

This you cannot do. It will fail outside of Songbird, and the chrome.manifest takes care of only having it in the right version of the right app. You don't need contents.rdf support at all as far as I am aware.

>     <RDF:li resource="chrome://global/content/customizeToolbar.xul"/>
>   </RDF:Seq>
> 
>   <RDF:Seq about="chrome://browser/content/browser.xul">
>     <RDF:li>chrome://chatzilla/skin/browserOverlay.css</RDF:li>
>   </RDF:Seq>
> 
>+  <RDF:Seq about="chrome://songbird/content/xul/mainScriptsOverlay.xul">
>+    <RDF:li>chrome://chatzilla/skin/browserOverlay.css</RDF:li>
>+  </RDF:Seq>
>+
>   <RDF:Seq about="chrome://global/content/customizeToolbar.xul">
>     <RDF:li>chrome://chatzilla/skin/browserOverlay.css</RDF:li>
>   </RDF:Seq>
>+

Nit: don't add the empty line.

> </RDF:RDF>
>Index: sb/Makefile.in
>--- /dev/null
>+++ sb/Makefile.in
>@@ -0,0 +1,52 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is ChatZilla.
>+#
>+# The Initial Developer of the Original Code is James Ross.
>+# Portions created by the Initial Developer are Copyright (C) 2004
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   James Ross, [e-mail], initial version.

Did James write this? =)

>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
>+
>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+CHATZILLA_VERSION=$(shell grep "const __cz_version" "$(srcdir)/../xul/content/static.js" | sed "s|.*\"\([^\"]\{1,\}\)\".*|\1|")

Do you really need to redefine this here? Seems unnecessary, but hey, maybe that's just me. Heck, do you really need this makefile to begin with? Why?

>+
>+XPI_NAME               = chatzilla
>+USE_EXTENSION_MANIFEST = 1
>+NO_JAR_AUTO_REG        = 1
>+INSTALL_EXTENSION_ID   = {59c81df5-4b7a-477b-912d-4e0fdf64e5f2}
>+XPI_PKGNAME            = chatzilla-$(CHATZILLA_VERSION)
>+
>+include $(topsrcdir)/config/rules.mk

Comment 9

[Mitchell Field [:Mitch]]

Attachment: Patch v1.2

This patch takes into account the feedback from the previous patch.

Comment 10

[:Gijs (he/him)]

Comment on attachment 302519 [details] [diff] [review]
Patch v1.2

>Index: xpi/locale-resources/install.rdf

Don't touch that file, it needs to be CVS removed and is unused.

>Index: xul/content/chatzillaOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/content/chatzillaOverlay.js,v
>retrieving revision 1.2
>diff -d -p -u -6 -r1.2 chatzillaOverlay.js
>--- xul/content/chatzillaOverlay.js	19 Apr 2000 21:39:59 -0000	1.2
>+++ xul/content/chatzillaOverlay.js	11 Feb 2008 00:00:00 -0000
>@@ -1,7 +1,28 @@
>-function toIRC() 
>+function toIRC()
> {
>-
>-	toOpenWindowByType("irc:chatzilla", "chrome://chatzilla/content/chatzilla.xul");
>-
>+    // toOpenWindowByType is not implemented in Songbird
>+    if (!toOpenWindowByType)
>+        var toOpenWindowByType = sb_toOpenWindowByType;
>+    
>+    toOpenWindowByType("irc:chatzilla",
>+                       "chrome://chatzilla/content/chatzilla.xul");

The variable declaration may conflict with the call you're making, as the variable will be declared (and set to undefined) anyway. Even though you tested it and apparently it worked at least on XULRunner, to be safe, and a bit more readable, why not do:

var url = "chrome://chatzilla/content/chatzilla.xul";
if (!("toOpenWindowByType" in window))
    sb_toOpenWindowByType("irc:chatzilla", url);
else
    toOpenWindowByType("irc:chatzilla", url);

(saving the interim variable to avoid having to use multiple lines, and using "in window" because the other method might throw a (strict) warning).

>+// toOpenWindowByType code stolen from mozilla/browser/base/content/browser.js
>+function sb_toOpenWindowByType(inType, uri, features)
>+{
>+    const WM_CID = "@mozilla.org/appshell/window-mediator;1";
>+    const nsIWM = Components.interfaces.nsIWindowMediator;
>+    
>+    var windowManager = Components.classes[WM_CID].getService();
>+    var windowManagerInterface = windowManager.QueryInterface(nsIWM);
>+    var topWindow = windowManagerInterface.getMostRecentWindow(inType);
>+    
>+    if (topWindow)
>+        topWindow.focus();
>+    else if (features)
>+        window.open(uri, "_blank", features);
>+    else
>+        window.open(uri, "_blank", "chrome,extrachrome,menubar,resizable," +
>+                                   "scrollbars,status,toolbar");

Using a multiline statement in an if block without braces is not proper style. Try doing:

var winFeatures = features ? features : "chrome.....";
(this statement will probably be multiline)

Then you can remove the "else if" block, and suffice with just calling:

window.open(uri, "_blank", winFeatures);

in the else block.


>Index: xul/content/config.css
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/content/config.css,v
>retrieving revision 1.1
>diff -d -p -u -6 -r1.1 config.css
>--- xul/content/config.css	9 Jun 2004 03:33:49 -0000	1.1
>+++ xul/content/config.css	11 Feb 2008 00:00:00 -0000
>@@ -47,15 +47,24 @@
> #pref-objects {
> 	min-width: 25ex;
> }
> 
> /* Set min-width and min-height on tabs container. */
> #pref-object-deck {
>-	min-width: 65ex;
>+	min-width: 71ex;
> 	min-height: 32em;
>-	width: 65ex;
>+	width: 71ex;
> 	height: 32em;
> }
> 
> scroller {
> 	overflow: auto;
> }
>+
>+#pref-header > .dialogheader-description {
>+	display: none;
>+}
>+
>+#pref-header > .dialogheader-title {
>+	display: -moz-box;
>+	border: none;
>+}

The changes to this file are definitely NOT okay without some explanation. Why do you need this? What does it do? Why can't the songbird theme do this itself?

<snip>

>Index: xul/content/prefs.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/content/prefs.js,v
>retrieving revision 1.49
>diff -d -p -u -6 -r1.49 prefs.js
>--- xul/content/prefs.js	20 Jan 2008 09:44:31 -0000	1.49
>+++ xul/content/prefs.js	11 Feb 2008 00:00:00 -0000
>@@ -89,12 +89,18 @@ function initPrefs()
>                  "goto-url-newtab", "goto-url-newtab"];
>     if (client.host == "XULrunner")
>     {
>         gotos = ["goto-url-external", "goto-url-external",
>                  "goto-url-external", "goto-url-external"];
>     }
>+    else if (client.host == "Songbird")
>+    {
>+        // Songbird has a browser, but only supports a single browser window
>+        gotos = ["goto-url-newtab", "goto-url-newtab",
>+                 "goto-url-newtab", "goto-url-newtab"];
>+    }

The first item in this list should still be "goto-url" to open things in the same tab.

<snip>

r-, but only because of the config.css changes. If you can explain those and address the nits, I think that'd suffice.

Comment 11

[Mitchell Field [:Mitch]]

Attachment: Patch v1.3

Due to requested changes in Songbird, ChatZilla's existing Firefox overlay can now be reused for it. This patch is therefore much simpler.

The ChatZilla CSS changes are necessary to fix graphical issues within the ChatZilla preferences dialog, when using it on Songbird. The more dramatic visual changes to the dialog are commented inline. The simple width change of the dialog window itself was necessary to overcome horizontal placement issues with the scrollbar (when running on Songbird), and has a negligible effect on its display within other applications (including XULRunner).

The only outstanding issue with running ChatZilla on Songbird (to my knowledge) is the lack of tab-text state-indication colouring, for which I will file a follow-up bug report.

Comment 12

[Mitchell Field [:Mitch]]

Attachment: Patch v1.4

This patch has been adjusted in accordance with bug 418748.

Comment 13

[Mitchell Field [:Mitch]]

Attachment: Patch v1.5

Comment 14

[:Gijs (he/him)]

Comment on attachment 304694 [details] [diff] [review]
Patch v1.5

This looks good, assuming you've tested it.

r=me

Comment 15

[Mitchell Field [:Mitch]]

Attachment: Patch v1.6

This patch has retargeted overlays to stay up-to-date with Songbird.

Comment 16

[:Gijs (he/him)]

Comment on attachment 304956 [details] [diff] [review]
Patch v1.6

r=me

Comment 17

[:Gijs (he/him)]

Checking in mozilla/extensions/irc/jar.mn: 1.38
Checking in mozilla/extensions/irc/locales/generic/install.rdf: 1.3
Checking in mozilla/extensions/irc/xpi/resources/install.rdf: 1.15
Checking in mozilla/extensions/irc/xul/content/commands.js: 1.149
Checking in mozilla/extensions/irc/xul/content/contents.rdf: 1.35
Checking in mozilla/extensions/irc/xul/content/prefs.js: 1.50
Checking in mozilla/extensions/irc/xul/content/static.js: 1.268
Checking in mozilla/extensions/irc/xul/skin/contents.rdf: 1.3

Mitch: please file a new bug on the display pane support, and one on the tab colouring, if you haven't already.