1128334 - Integrate tH's xulrunner patch into the main chatzilla repository

Status: NEW

(Other Applications :: ChatZilla, defect)

Description

[:Gijs (out for now; he/him)]

Attachment: xrmakeM.diff.txt

As per the attachment... :-)

Comment 1

[Stoyan Dimitrov [:stoyan]]

This should be blocker for bug 1128334.

Comment 2

[Stoyan Dimitrov [:stoyan]]

Ah, sorry. Got so excited. The correct number is bug 898808.

Comment 3

[Rob Marshall [tH]]

Attachment: Build script, resources, and overlay

Updated, and some stuff that shouldn't go in stripped out (updater pointed at my server).

Comment 4

[Rob Marshall [tH]]

Attachment: [committed] Add commands

Adds Add-ons, JS Console, and about:config to the main menu. Includes the toOpenWindowByType method used in bug 898808.

Comment 6

[:Gijs (out for now; he/him)]

Per bug 1128455, can we use toOpenWindowByType for the about:config opening as well, so it doesn't open multiple times?

Comment 7

[Stoyan Dimitrov [:stoyan]]

Attachment: [committed] Fix corresponding window types

The patch removes the ability to open several windows of Advanced Configuration (a.k.a. about:config) and the Addons Manager.

Comment 8

[Stoyan Dimitrov [:stoyan]]

James, I don't want to be rude but the patches are getting out-of-date and keeping them updated (off-line) gets tedious now. I need the patches update to work on #1128353. So every time I want to do some work I have to sync those three patches first.

Comment 9

[James Ross]

Comment on attachment 8557795 [details] [diff] [review]
Build script, resources, and overlay

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

Generally this seems sane. I've forgotten a a fair bit about how jar.mn vs .manifest works and what we'll need in today's world, so maybe Gijs can sanity-check just those bits.

::: xpi/makexpi.py
@@ +18,5 @@
>  import shutil
>  import re
>  import zipfile
>  from os.path import join as joinpath
> +from subprocess import call, PIPE

I don't see either 'call' or 'PIPE' being used; did I miss them or is this unnecessary?

@@ +350,4 @@
>      progress_mkdir(joinpath(xpiroot, 'components'))
>      print '            done'
>      
> +    progress_echo('  Checking XULRunner structure')

Do we need to make any of this conditional? Maybe it doesn't matter.

::: xpi/resources/brand.dtd
@@ +1,5 @@
> +<!ENTITY  brandShortName        "ChatZilla">
> +<!ENTITY  brandFullName         "ChatZilla @REVISION@">
> +<!ENTITY  vendorShortName       "">
> +<!ENTITY  logoTrademark         "">
> +<!ENTITY  releaseURL            "http://chatzilla.rdmsoft.com/">

Probably don't want this URL in the repo. :)

::: xpi/resources/chatzilla-prefs.xr.js
@@ +31,5 @@
> +// make the external protocol service happy
> +pref("network.protocol-handler.expose-all", false);
> +pref("network.protocol-handler.expose.irc", true);
> +pref("network.protocol-handler.expose.ircs", true);
> +pref("security.dialog_enable_delay", 0);

This seems like something we shouldn't be setting...

::: xpi/resources/chrome.xr.manifest
@@ +1,1 @@
> +content chatzilla       jar:chatzilla.jar!/content/chatzilla/ xpcnativewrappers=yes

Isn't this whole file generated by building the jar.mn files?

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 8557795 [details] [diff] [review]
Build script, resources, and overlay

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

What's the aim here, now that xulrunner is dead? Do we just want to generate a zip file that, when unzipped, creates a dir structure that works with firefox -app or something?

r- for the security issues James and I flagged up.

::: .hgignore
@@ +1,3 @@
>  (^|/)Makefile$
>  
> +^xpi/chatzilla-.*(\.xpi|\.xulapp)$

Is it worth creating a xulapp at this point? XULRunner is dead, and I didn't think firefox -app took xulapps.

::: xpi/resources/application.ini
@@ +5,5 @@
> +ID={59c81df5-4b7a-477b-912d-4e0fdf64e5f2}
> +
> +[Gecko]
> +MinVersion=17.0
> +MaxVersion=33.*

This should be updated, I guess.

::: xpi/resources/chatzilla-prefs.xr.js
@@ +16,5 @@
> +pref("extensions.dss.switchPending", false);
> +pref("extensions.ignoreMTimeChanges", false);
> +pref("extensions.logging.enabled", false);
> +pref("general.skins.selectedSkin", "classic/1.0");
> +pref("extensions.checkUpdateSecurity", false);

This is basically "hello, could you MITM my update checks", which is a bad idea. I don't think we should do this. I'm a bit confused by this anyway, because AIUI this patch doesn't indicate where to look for updates, so I doubt it works either way. Am I missing something?

@@ +31,5 @@
> +// make the external protocol service happy
> +pref("network.protocol-handler.expose-all", false);
> +pref("network.protocol-handler.expose.irc", true);
> +pref("network.protocol-handler.expose.ircs", true);
> +pref("security.dialog_enable_delay", 0);

Concur with James here.

::: xpi/resources/chrome.xr.manifest
@@ +1,1 @@
> +content chatzilla       jar:chatzilla.jar!/content/chatzilla/ xpcnativewrappers=yes

It should be, yes. If there are things in here in addition to what we normally do we should include it in (a separate) jar.mn and preprocess it, or reuse the generated chrome.manifest and append any extra stuff to it that we need (AFAICT, just the overlay registration?) otherwise this'll just get out of sync.

::: xr/Makefile.in
@@ +15,5 @@
> +USE_EXTENSION_MANIFEST = 1
> +INSTALL_EXTENSION_ID   = {59c81df5-4b7a-477b-912d-4e0fdf64e5f2}
> +XPI_PKGNAME            = chatzilla-$(CHATZILLA_VERSION)
> +
> +include $(topsrcdir)/config/rules.mk

I don't think we need this makefile at all - we only have one for seamonkey, and that clearly doesn't care about any of this. This also looks copy-pasted from the main makefile, which is almost certainly wrong. You also haven't added the xr dir to the moz.build or makefile in the parent dir, so I think right now this all just gets ignored. We should remove it.

::: xr/jar.mn
@@ +1,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/.
> +
> +# The registration of the overlays specific to Firefox below should match

This comment is out of date.

::: xul/content/xr/overlay.xul
@@ +2,5 @@
> +
> +<overlay id="ChatzillaXROverlay"
> +         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +<script type="application/x-javascript" src="chrome://global/content/findUtils.js"/>

Meh. We could also just do this by loading this with the substring loader at runtime, if it's really needed.

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8557872 [details]
[committed] Fix corresponding window types

rs=me for this

Comment 12

[James Ross]

Comment on attachment 8557797 [details] [diff] [review]
[committed] Add commands

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

r=silver with the toOpenWindowByType(inType, url, features) function moved to utils.js next to getWindowByType(windowType).

::: xul/content/commands.js
@@ +4712,5 @@
> +function toOpenWindowByType(inType, url, features)
> +{
> +    var wm = getService("@mozilla.org/appshell/window-mediator;1",
> +                        "nsIWindowMediator");
> +    var topWindow = wm.getMostRecentWindow(inType);

The above three lines can use the "getWindowByType(windowType)" in utils.js, probably where this whole function should go too.

Comment 13

[James Ross]

I've committed the two parts that have approvals; the remaining part, attachment 8557795 [details] [diff] [review], needs some work.

(In reply to :Gijs Kruitbosch from comment #10)
> What's the aim here, now that xulrunner is dead? Do we just want to generate
> a zip file that, when unzipped, creates a dir structure that works with
> firefox -app or something?

Yes, I believe that's the best option at present to run ChatZilla.