363491 - Stop suiterunner builds needing to build in xpfe/global

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Mark Banner (:standard8)]

xulrunner doesn't build xpfe/global, and for suiterunner builds it should be fairly easy for us not to need to as well.

Things we have to do to achieve this:

buildconfig.html.in - no change necessary. Toolkit already has this
build.dtd - this contains the buildId.label, used http://lxr.mozilla.org/seamonkey/source/suite/locales/en-US/chrome/browser/navigator.dtd#7 here and line 9. Should we be using the url formatter for this? (I know they are not quite urls, but I think it may work)

hiddenWindow.xul - copy to suite/browser/mac? or suite/browser?
logo.gif - just move the jar.mn registration to suite/branding/jar.mn
aboutAbout.html - copy to suite/browser?
about.xul - we're already using the toolkit about.xhtml version
fullScreen.js - copy to suite/browser?

I still need to examine the os-specific directories xpfe/global/resources/content/{unix,mac,os2,win}

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
Some refinements:

> hiddenWindow.xul - copy to suite/browser/mac? or suite/browser?

- CVS copy to suite/browser.
- New URI chrome://browser/content/hiddenWindow.xul.
- Add pref browser.hiddenWindowChromeURL to point to new URI.
- Don't package xpfe/global/.../hiddenWindow.xul for any SeaMonkey just use the new suite/browser one.

> aboutAbout.html
Not sure about this one. docshell/base/nsAboutRedirector.cpp references it in global. So either:

a) Get it into toolkit (does/should ff use it?)
b) Change that reference in nsAboutRedirector
c) Drop the option in about redirectory and set one up for us to do explicitly (can we do that?)

> fullScreen.js

- move to suite/browser
- change uri to chrome://browser/content/...
- update one reference in navigator.xul

Comment 2

[Robert Kaiser]

This is connected to bug 336874 for sure :)

note that it's not chrome://browser but chrome://navigator ;-)

And we can safely ditch about.gif in suiterunner, as we're not using about:logo there any more, we can probably even remove that from msAboutRedirector once Camino is not using xpfe any more (they're using the old Mozilla about:logo)

Comment 3

[[e-mail]]

(In reply to comment #0)
>build.dtd - this contains the buildId.label, used
>http://lxr.mozilla.org/seamonkey/source/suite/locales/en-US/chrome/browser/navigator.dtd#7
>here and line 9. Should we be using the url formatter for this? (I know they
>are not quite urls, but I think it may work)
For that matter we could get the build ID from the XUL app info. Don't forget to update all three places a) document.title [normally automatically set from document.documentelement.getAttribute("title")] b) title and titlemodifier
>logo.gif - just move the jar.mn registration to suite/branding/jar.mn
Make about:logo point to chrome://branding/content/logo.png ?

Comment 4

[Robert Kaiser]

(In reply to comment #3)
> >logo.gif - just move the jar.mn registration to suite/branding/jar.mn
> Make about:logo point to chrome://branding/content/logo.png ?

Would be problematic for Camino, which is actually the only user of the old Mozilla-branded logo.gif

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #2)
> And we can safely ditch about.gif in suiterunner, as we're not using about:logo
> there any more, we can probably even remove that from msAboutRedirector once
> Camino is not using xpfe any more (they're using the old Mozilla about:logo)

Why aren't we using it there any more? Is there a particular reason you're thinking that?

Comment 6

[Robert Kaiser]

(In reply to comment #5)
> Why aren't we using it there any more? Is there a particular reason you're
> thinking that?

Suiterunner uses toolkit's about: page which uses chrome://branding/content/about.png instead of about:logo and so makes about:logo unused.

Comment 7

[[e-mail]]

See bug 363725 for my take an about:logo (which, by the way, is logo.gif for the xpfe version or about.png for the toolkit version but never about.gif).

Comment 8

[Mark Banner (:standard8)]

Here's my revised take on things now Neil's done the logo alteration:

logo.gif - only include for xpfe apps (drop for suiterunner)
buildconfig.html.in - drop for suiterunner
about.xul - drop for suiterunner
build.dtd - update navigator files to take the id and relevant info from the xul app info.
hiddenWindow.xul and fullScreen.js - cvs copy to suite/browser and change uri to chrome://navigator etc
aboutAbout.html - see if we can get into toolkit.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> aboutAbout.html - see if we can get into toolkit.

Ok, things may have changed but see bug 220253 :-( and bug 349451

Comment 10

[Mark Banner (:standard8)]

Attachment: cvs copies v1

Two cvs copies required for this bug. aboutAbout.html is still up in the air (bug 220253 may be reopened now about:config has a warning dialog).

Main patch coming up.

Comment 11

[Mark Banner (:standard8)]

Attachment: Patch v1

This deals with most of the files, excluding build.dtd and buildconfig.html generation and aboutAbout.html.

The generation of build* files will be excluded when we exclude xpfe/global which will be when we sort out what's happening with aboutAbout.html

Note that hiddenWindow.xul is modified in place, but I would get the cvs copies done first, then apply the modification.

Comment 12

[Mark Banner (:standard8)]

Attachment: Patch v1a

I forgot the addition of a pref in the previous version of the patch.

Comment 13

[[e-mail]]

Comment on attachment 250070 [details] [diff] [review]
Patch v1a

>+* locale/@AB_CD@/navigator/navigator.dtd                                    (%chrome/browser/navigator.dtd)
Can't preprocess locale (the old build.dtd was in content).

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #13)
> (From update of attachment 250070 [details] [diff] [review])
> >+* locale/@AB_CD@/navigator/navigator.dtd                                    (%chrome/browser/navigator.dtd)
> Can't preprocess locale (the old build.dtd was in content).

brand.dtd & brand.properties are preprocessed - is that a mistake? If its not a mistake, how about moving that build id into brand.dtd?

Comment 15

[[e-mail]]

(In reply to comment #14)
>(In reply to comment #13)
>>Can't preprocess locale (the old build.dtd was in content).
>brand.dtd & brand.properties are preprocessed - is that a mistake? If its not a
>mistake, how about moving that build id into brand.dtd?
brand.properties isn't preprocessed, is it? brand.dtd might be a bug, although as brand.dtd always matches contents.rdf it isn't a big bug.

Comment 16

[Benjamin Smedberg]

Localized files (including brand.dtd) should not contain non-localizable strings. The buildid should be in a content package, not in a locale package.

Comment 17

[Robert Kaiser]

Exactly. Please bring back the build.dtd in a content package, there is no good reason to put this into locale.

The preprocessing of brand stuff is something we should also try to get rid of, and for suiterunner it should be possible - the main reason for this was to bring the version number into release URLs, but that can now be done with urlformatter in a much better way.

Comment 18

[Mark Banner (:standard8)]

Attachment: Patch v2

This patch creates a new build.dtd for SeaMonkey in suite/common and references it in the appropriate places. Its still also moving fullScreen.js & hiddenWindow.xul as per v1.

brand.properties doesn't actually need to be preprocessed, so this version of the patch removes that processing.

brand.dtd still needs to be preprocessed (to set the language pack version for xpfe versions of SeaMonkey) but we'll be raising another bug to stop that by the time we become a toolkit app if not before.

Comment 19

[Mark Banner (:standard8)]

Attachment: Patch v3

Following discussions on other bugs, toolkit doesn't want a global about:about - we have to extend it for suite. Therefore, I've updated the patch with an extension of about so that about:about will work for us. This will also mean that we want to copy aboutAbout.html to suite/browser along with the others.

I think this patch works, but I've still got some verification to do.

Comment 20

[Mark Banner (:standard8)]

Attachment: Patch v3 (proper version)

Opps, attached the wrong patch last time.

Comment 21

[Mark Banner (:standard8)]

Attachment: Patch v4

The proper v3 had missed a couple of things. This one should work correctly if the required cvs copies are also done.

Comment 22

[Mark Banner (:standard8)]

Attachment: cvs copies v2

The required cvs copies for this bug. Once approved I'll raise a specific cvs copies bug for them. See Patch v4 for how things are moving.

Comment 23

[[e-mail]]

Comment on attachment 250774 [details] [diff] [review]
Patch v4

>-ifneq (,$(MOZ_PHOENIX)$(MOZ_XULRUNNER))
>+# XXX When SeaMonkey switches to being an MOZ_XUL_APP this needs to be
>+# simplified.
>+MOZ_SUITERUNNER=
>+ifeq ($(MOZ_SUITE),$(MOZ_XUL_APP))
>+MOZ_SUITERUNNER=1
>+endif
>+
>+ifneq (,$(MOZ_PHOENIX)$(MOZ_XULRUNNER)$(MOZ_SUITERUNNER))
Hmm... so MOZ_SUITERUNNER isn't accurate here, because it will be set for any non-suite non-XUL app. (Not that it all works out in the wash, but...)

> pref("browser.chromeURL","chrome://navigator/content/navigator.xul");
>-
>+pref("browser.hiddenWindowChromeURL", "chrome://navigator/content/hiddenWindow.xul");
Leave the blank line in?

>+  getURIFlags : function getURIFlags(aURI) {
>+    return 0;
>+  },
You'll need ALLOW_SCRIPT here. (Is that enough to allow you to link to the other about: pages?)

>+#expand <!ENTITY  buildId.label "Build ID: 0000000000">
Oops ;-)

Comment 24

[Robert Kaiser]

Comment on attachment 250776 [details] [diff] [review]
cvs copies v2

looks good after clearing up that all of those fit well in the browser/ part of the suite. r=me.

Comment 25

[Dave Miller [:justdave]]

Comment on attachment 250776 [details] [diff] [review]
cvs copies v2

for future reference, the copy scripts need to include the destination filename on each line, not just the directory it's going into.  The script isn't quite *that* smart.

Comment 26

[Mark Banner (:standard8)]

Attachment: Patch v5

I'm still testing this, but I think this is the final patch - note that I'm separating out the actual about:about redirector change as I'll probably request a different set of people to review that.

Comment 27

[Mark Banner (:standard8)]

Attachment: Patch v5 p2 - nsAboutAbout module

This is the nsAboutAbout module that we'll need to go with the v5 patch.

Comment 28

[Phil Ringnalda (:philor)]

>+ * The Original Code is the Mozilla Firefox browser.

Me, I'd say the boilerplate for a component in JS has gotten far enough away from Fx's nsBrowserContentHandler.js to no longer be Firefox.

>+ *   Neil Rashbook <[e-mail]>

And far enough from suiterunner's nsBrowserContentHandler.js to not require Neil's typoed name/literary alias.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 251432 [details] [diff] [review]
Patch v5

Review requesting time ;-)

Comment 30

[Mark Banner (:standard8)]

Attachment: Patch v5 p2 - nsAboutAbout module (checked in)

Corrected the license headers on the nsAboutAbout module. First I'm going to check Neil's happy with this, then I'll get review from either dveditz (probably) or bz (maybe) to approve from a security aspect.

Comment 31

[[e-mail]]

Comment on attachment 251432 [details] [diff] [review]
Patch v5

>-ifneq (,$(MOZ_PHOENIX)$(MOZ_XULRUNNER))
>+# XXX When SeaMonkey switches to being an MOZ_XUL_APP this needs to be
>+# simplified.
>+MOZ_SUITERUNNER=
>+ifeq (11,$(MOZ_SUITE)$(MOZ_XUL_APP))
>+MOZ_SUITERUNNER=1
>+endif
>+
>+ifneq (,$(MOZ_PHOENIX)$(MOZ_XULRUNNER)$(MOZ_SUITERUNNER))
> DIRS += \
>         browser/public \
>         browser/src \
Bah, xpfe is too confusing for its own good.
XULrunner: browser (excluding samples)
Firefox: browser (excluding samples)
Sunbird: (none)
Camino: browser communicator global
Thunderbird: browser communicator global
SeaMonkey: browser global
Suiterunner: browser

Looking at global, most of it is #ifdef MOZ_XUL_APP anyway. If it turns out that Thunderbird doesn't need those few remaining files then I think it would be worthwhile moving global into its own ifdef block in the makefile.

Comment 32

[[e-mail]]

-DIRS += browser global
+DIRS += browser

...

+ifndef MOZ_XUL_APP
+DIRS += global
+endif
+

Comment 33

[[e-mail]]

Comment on attachment 251448 [details] [diff] [review]
Patch v5 p2 - nsAboutAbout module (checked in)

>+    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService().QueryInterface(nsIIOService);
Nit: .getService(nsIIOService);

I guess I need to clobber and build with the previous patch to be able to demonstrate the effect of this patch?

Comment 34

[Mark Banner (:standard8)]

(In reply to comment #33)
> I guess I need to clobber and build with the previous patch to be able to
> demonstrate the effect of this patch?
> 

You'll need both patches. To be fully sure, you'll possibly want to temporarily modify the title of suite/browser/aboutAbout.html so that you know which one its picked up.

I've noted the makefile comments - will look at them later.

Comment 35

[Mark Banner (:standard8)]

Attachment: Stops XUL apps building in xpfe/global (checked in)

This patch stops XUL Apps building xpfe/global and simplifies some of the ifdefs - this only really affects Thunderbird and SeaMonkey (suiterunner) as they are the only XUL apps still building in xpfe/global (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/Makefile.in&rev=1.35&mark=49-61#49 and comment 31).

SeaMonkey (suiterunner) issues with this are addressed by other patches on this bug.

Thunderbird currently still includes some files, however I don't think they are necessary for Thunderbird:

build.dtd - defines a string not necessary for tb (http://lxr.mozilla.org/seamonkey/search?string=buildId.label)
buildconfig.html - generated within toolkit (http://lxr.mozilla.org/seamonkey/search?string=buildconfig.html)

From xpfe/global/jar.mn (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/jar.mn&rev=1.104&mark=77-86#77) Thunderbird includes:

hiddenWindow.xul - tb has its own version at http://lxr.mozilla.org/seamonkey/source/mail/base/content/hiddenWindow.xul
logo.gif - No longer required (http://lxr.mozilla.org/seamonkey/search?string=logo.gif - the nsAboutRedirector requirement is only for non-xul apps)
aboutAbout.html - requirement will be removed from nsAboutRedirector (see discussion on bug 365852), and can't easily be got to in tb anyway.
about.xul - tb already uses toolkit/its own version.
fullScreen.js - only required for SeaMonkey (http://lxr.mozilla.org/seamonkey/search?string=fullScreen.js)

Note that the changes around the MOZ_SUITE ifdef if xpfe/global/jar.mn are due to SeaMonkey's restructuring around not building xpfe/global (to be reviewed by Neil).

Requesting reviews from Scott (Thunderbird), Benjamin (General build config) and Neil (SeaMonkey/xpfe).

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 251460 [details] [diff] [review]
Stops XUL apps building in xpfe/global (checked in)

Benjamin, please also refer to comment 35 when reviewing this. Thanks.

Comment 37

[Mark Banner (:standard8)]

Attachment: Patch v6

Sorry for the mess-around. This is the v5 patch with the "stops xul apps building in xpfe/global" parts removed.

Comment 38

[[e-mail]]

Comment on attachment 251461 [details] [diff] [review]
Patch v6

>+pref("browser.tabs.closeButtons", 3);
>+pref("browser.tabs.tabMinWidth", 100);
>+pref("browser.tabs.tabClipWidth", 140);
I'm not sure about some of your prefs changes but these are definitely wrong.

Comment 39

[Mark Banner (:standard8)]

(In reply to comment #38)
> (From update of attachment 251461 [details] [diff] [review])
> >+pref("browser.tabs.closeButtons", 3);
> >+pref("browser.tabs.tabMinWidth", 100);
> >+pref("browser.tabs.tabClipWidth", 140);
> I'm not sure about some of your prefs changes but these are definitely wrong.

Please ignore everything in the browser-prefs.js files apart from:

+pref("browser.hiddenWindowChromeURL", "chrome://navigator/content/hiddenWindow.xul");

I forgot to cut the rest out.

Comment 40

[Mark Banner (:standard8)]

(In reply to comment #39)
> (In reply to comment #38)
> > (From update of attachment 251461 [details] [diff] [review] [details])
> I forgot to cut the rest out.

@@ -2,6 +2,9 @@ comm.jar:
 #ifdef MOZ_XUL_APP
 % content navigator %content/navigator/ xpcnativewrappers=yes
 % overlay chrome://navigator/content/pageInfo.xul chrome://pippki/content/PageInfoOverlay.xul
+% style chrome://navigator/content/navigator.xul chrome://navigator/content/xul.css
+  content/navigator/bindings/tabbrowser.xml                           (tabbrowser.xml)
+  content/navigator/xul.css                                           (xul.css)
 % content navigator-platform %content/navigator-platform/ platform xpcnativewrappers=yes
 % content navigator-region %content/navigator-region/ xpcnativewrappers=yes
 #else

is another chunk that shouldn't be there. Apart from those three (2 browser-prefs.js, 1 jar.mn) problems the rest of it should be ok ;-)

Comment 41

[Robert Kaiser]

Comment on attachment 251461 [details] [diff] [review]
Patch v6

r=me taking comment #39 and comment #40 into account.

Comment 42

[[e-mail]]

Comment on attachment 251461 [details] [diff] [review]
Patch v6

sr=me if this has been tested on a Mac.

Comment 43

[Mark Banner (:standard8)]

Attachment: Patch v6a (checked into trunk)

This is what I've just checked into trunk. It'll mean we're including some duplicate files in global and communicator at the moment, but I don't see that's a problem.

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 251448 [details] [diff] [review]
Patch v5 p2 - nsAboutAbout module (checked in)

Requesting standard/security review as  will. Please also see comment 23, but not that the existing about:about page has chrome privs as well (http://lxr.mozilla.org/seamonkey/source/docshell/base/nsAboutRedirector.cpp#85), and hopefully I've made it the same here.

Comment 45

[Daniel Veditz [:dveditz]]

Comment on attachment 251448 [details] [diff] [review]
Patch v5 p2 - nsAboutAbout module (checked in)

r=dveditz

Comment 46

[[e-mail]]

Comment on attachment 251448 [details] [diff] [review]
Patch v5 p2 - nsAboutAbout module (checked in)

>+var nsAboutAbout = {
>+
Nit: spurious blank line here

>+  newChannel : function newChannel(aURI) {
Nit: no space before :

>+    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService().QueryInterface(nsIIOService);
Nit: .getService(nsIIOService);

>+  getURIFlags : function getURIFlags(aURI) {
Nit: no space before :

>+  /* nsIFactory */
>+  createInstance: function createInstance(outer, iid) {
>+    if (outer != null)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>+
>+    return this.QueryInterface(iid);
>+  },
Nit: missing lockFactory implementation (although the comma is there!)

>+const ABOUTABOUT_CONTRACTID = "@mozilla.org/network/protocol/about;1?what=about";
Please define consts equivalent to the string values in nsIAboutModule.idl and concatenate them as appropriate to create this const. That way if someone lxrs for one of those names then this will show up in the results.

sr=me with these fixed.

Comment 47

[Mark Banner (:standard8)]

Comment on attachment 251448 [details] [diff] [review]
Patch v5 p2 - nsAboutAbout module (checked in)

Checked in with Neil's nits addressed.

Comment 48

[Mark Banner (:standard8)]

Attachment: Unbreak about:about in xpfe builds

Fix about:about for xpfe builds - this'll mean that we'll have 2 versions of aboutAbout.html in xpfe SeaMonkey but seeing as its going soon... and it'll keep our testers happy in the meantime.

Comment 49

[[e-mail]]

Comment on attachment 264018 [details] [diff] [review]
Unbreak about:about in xpfe builds

about:about works for me, and I only have dist/bin/chrome/comm/content/navigator/aboutAbout.html

Maybe it would be better to delete line 85 of nsAboutRedirector.cpp?

Comment 50

[[e-mail]]

Oh, I bet the installer isn't shipping nsAboutAbout.js is it?

Comment 51

[Mark Banner (:standard8)]

Attachment: Unbreak about:about in installer builds (checked in)

Includes nsAboutAbout.js in packaging lists.

Comment 52

[Andrew Schultz]

Comment on attachment 264110 [details] [diff] [review]
Unbreak about:about in installer builds (checked in)

>Index: xpinstall/packager/packages-win
>+bin/components/nsAboutAbout.js

windows slashes go the other way

Comment 53

[[e-mail]]

Comment on attachment 264110 [details] [diff] [review]
Unbreak about:about in installer builds (checked in)

That's so obvious now that it's been pointed out ;-)

Comment 54

[Steffen Wilberg]

In bug 220253, I ported about:about to Toolkit.
Follow-up bug 538419 filed for Seamonkey.