389140 - Stop building in mozilla/themes

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

CTho was poking on irc tonight about why we build in mozilla/themes.

As far as I can tell, the only thing we use from there are config.css and netError.css (which we already have pseudo-forked in embed-replacements) and the alert-exclam.gif used by netError.css (which we have pseudo-forked, but which we've even replaced with an inline image in our copy of netError.css).  In other words, whatever we need from there, we supply ourselves now.

I can't build anymore to test this hypothesis, but someone who can should wack that line from client.mk (currently the line in the URL field) in a fresh tree and see if 
1) we build successfully and 
2) if anything in the app seems broken (my guess is testing the various about: urls would cover anything we'd reasonably use, but this is a little afield of my knowledge)

(If all is well, we can stop building on branch, too, and save a bit of space that way.)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Besides the issue philippe uncovered in bug 390104, anything that's needed by any content-area XUL (e.g., XUL webapps) will need these files, I guess.

I wonder if we shouldn't try to pull pinstripe copies, though; for example, button.css in themes/classic/global/mac is pretty outdated....

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Actually, I misread philippe's comment in bug 390104; it seems SeaMonkey isn't even using those Mac theme/skin files on the trunk (which explains why they've been left to become outdated).

Comment 3

[Stefan [:stefanh]]

Yeah, I haven't touched those files for some time since SeaMonkey trunk now uses pinstripe/global files for xul widget theming. On branch, the themes/classic/global/mac files should be usable - they differ a bit from the pinstripe versions, though.

Does Camino pull any files in xpfe/global/resources/content/bindings/? Nearly all of those files are abandonded by SeaMonkey (trunk) in favour of toolkit/content/widgets/ files.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I assume we pull all of them; that is, we have files with those names in Embed.jar/content/global.  

The only place we really have any exposure to XUL is about:config (and that random xul:button in netError.xhtml), so for the most part stuff being abandoned doesn't affect us and we don't notice (only a limited subset of content-area XUL works).

It sounds like we need to move to pulling all the XUL stuff from toolkit, pull Pinstripe, but continue to override the files we override now.

Comment 5

[Stefan [:stefanh]]

Pulling xul widgets from toolkit is the right way - the old xpfe ones are pretty dead, afaik no one uses them anymore.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: sorta-hacky fix

I got annoyed with the wrong padding on the buttons in the safebrowsing error page, so I came back to poke this again with my new, expanded jar-fu.

This seems to fix it; we no longer pull or build in mozilla/themes, we fill embed.jar with files from toolkit/themes/pinstripe (and override the same bits  we always do, since the embed-replacements step runs after this step) and enjoy the benefits of proper button padding and whatnot :P

The jar.mn file is a copy of toolkit/themes/pinstripe/global/jar.mn, with paths fixed up to be absolute for most of those files.

There are some odd/follow-up bits: 

1) embed-jar.mn still provides us with registration for the skin package, but I've hacked the makefile (brazenly copied from mento's flashblock makefile) to register the right package (though not the skin selection part) in the event embed-jar.mn ever stops :P

2) about.css is a new file, and in toolkit/themes/pinstripe it comes from winstripe; we'll need to port a few changes from our netError.css/config.css style and make a new Camino-like about.css (used for about:credits).

3) I'm not sure why toolkit/themes/pinstripe is mapping plugins.css to a null file, given that a copy of the file exists.

4) We can move our handful of skin files from embed-replacements/ to pinstripe/global, if we want, and simply update the jar.mn.

Mark, will you take first crack at this?

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 326147 [details] [diff] [review]
sorta-hacky fix

Since I don't think I'll be able to get mento for a while and because the wrong XUL buttons have really been bothering me when testing all these new error pages...Stuart, when you get a chance, what do you think about this approach?

Comment 8

[Stuart Morgan]

Comment on attachment 326147 [details] [diff] [review]
sorta-hacky fix

> ifndef MOZ_XUL_APP
>-tier_toolkit_dirs += themes
>+# tier_toolkit_dirs += themes
> endif

This whole block should just be removed now.

Other than that, r=me with the caveat that I don't understand the jar system ;) We should still get mento to take a look.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Core-only part of the patch

mento, can I get your build-system-r on this part?

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Camino-only part of the patch [checked in]

Comment 11

[Mark Mentovai]

Comment on attachment 337498 [details] [diff] [review]
Core-only part of the patch

Did you check with other non-MOZ_XUL_APP clients (if there are any now) to see if they care about the toolkit-tiers.mk change?

Comment 12

[Mark Mentovai]

Comment on attachment 337499 [details] [diff] [review]
Camino-only part of the patch [checked in]

Looks right.

Comment 13

[Stuart Morgan]

Comment on attachment 337499 [details] [diff] [review]
Camino-only part of the patch [checked in]

Ship it!

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 337498 [details] [diff] [review]
Core-only part of the patch

(In reply to comment #11)
> (From update of attachment 337498 [details] [diff] [review])
> Did you check with other non-MOZ_XUL_APP clients (if there are any now) to see
> if they care about the toolkit-tiers.mk change?

Camino is the only non-MOZ_XUL_APP client on 1.9.0, and kreeger said even if he gets Correo working on 1.9.0, he doesn't care about this ifndef.  (This code has already been removed from 1.9.1 and above.)

Requesting approval to land this build system-only change for 1.9.0.8; it's a Camino-only change that makes Camino stop pulling and building obsolete files in mozilla/themes.

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 337499 [details] [diff] [review]
Camino-only part of the patch [checked in]

Camino part checked in to cvs trunk; leaving this open until we actually stop building in mozilla/themes (the Core part).

Comment 16

[Eiichi]

After landing attachment 337499 [details] [diff] [review], many files are added in "Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/", for example;
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/10pct_transparent_grey.png
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/10pct_transparent_pixel.png
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/20pct_transparent_pixel.png
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/50pct_transparent_grey.png
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/50pct_transparent_white.png
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/about.css
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/appPicker.css
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/arrow.css
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/closetab.png
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/console/
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/customizeToolbar.css
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/datetimepicker.css
Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/findBar.css
,and the rest.

I clear my tree, and that's the same.
Is this correct?

Comment 17

[Eiichi]

Just for reference.
file size of "Camino.app/Contents/MacOS/chrome/embed.jar" of my test build;
Before landing the patch: 1,506,616 bytes
After landing the patch :  1,670,763 bytes

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Yes, there are files in pinstripe that did not exist in mozilla/themes, and it's certainly possible that the newer pinstripe files are larger than the old "counterparts" in mozilla/themes.  

Also, unless you've done a clobber build (or deleted embed.jar from inside Camino and from dist/), there are going to be two sets of files in embed.jar, the old ones and the new ones, when the filenames are not the same.

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #18)
> Also, unless you've done a clobber build (or deleted embed.jar from inside
> Camino and from dist/), there are going to be two sets of files in embed.jar,
> the old ones and the new ones, when the filenames are not the same.

Er, until the other patch on this bug is checked in, we'll still have extra files where there's no toolkit match for a file in mozilla/themes.  Sorry, my brain not working correctly this late.

Comment 20

[Eiichi]

(In reply to comment #19)
> Er, until the other patch on this bug is checked in, we'll still have extra
> files where there's no toolkit match for a file in mozilla/themes.  Sorry, my
> brain not working correctly this late.

Thanks, Smokey.
By the way, are all files "Camino.app/Contents/MacOS/chrome/embed/" needful for Camino?
It seems that files in "Camino.app/Contents/MacOS/chrome/embed/skin/classic/global/icons" (e.g. blacklist_64.png) aren't used for Camino.

Comment 21

[Eiichi]

Just for reference.

These files and folders are in 2009-03-10-00-2.0-M1.9 and aren't in 2009-03-09-00-2.0-M1.9.
-------------
/chrome/embed/skin/classic/global/10pct_transparent_grey.png
/chrome/embed/skin/classic/global/10pct_transparent_pixel.png
/chrome/embed/skin/classic/global/20pct_transparent_pixel.png
/chrome/embed/skin/classic/global/50pct_transparent_grey.png
/chrome/embed/skin/classic/global/50pct_transparent_white.png
/chrome/embed/skin/classic/global/about.css
/chrome/embed/skin/classic/global/appPicker.css
/chrome/embed/skin/classic/global/arrow.css
/chrome/embed/skin/classic/global/closetab.png
/chrome/embed/skin/classic/global/console/
/chrome/embed/skin/classic/global/customizeToolbar.css
/chrome/embed/skin/classic/global/findBar.css
/chrome/embed/skin/classic/global/listbox_highlight.png
/chrome/embed/skin/classic/global/menu/
/chrome/embed/skin/classic/global/menulist/
/chrome/embed/skin/classic/global/notification/
/chrome/embed/skin/classic/global/notification.css
/chrome/embed/skin/classic/global/passwordmgr.css
/chrome/embed/skin/classic/global/preferences.css
/chrome/embed/skin/classic/global/richlistbox.css
/chrome/embed/skin/classic/global/scale/
/chrome/embed/skin/classic/global/scrollbox/
/chrome/embed/skin/classic/global/splitter/
/chrome/embed/skin/classic/global/statusbar-background-inactive.gif
/chrome/embed/skin/classic/global/statusbar-background.gif
/chrome/embed/skin/classic/global/toolbar/
/chrome/embed/skin/classic/global/tree/
/chrome/embed/skin/classic/global/wizardOverlay.css


These files and folders are in 2009-03-09-00-2.0-M1.9 and aren't in 2009-03-10-00-2.0-M1.9.
-------------
/chrome/embed/skin/classic/global/console.css
/chrome/embed/skin/classic/global/preview.gif
/chrome/embed/skin/classic/global/printing.css
/chrome/embed/skin/classic/global/scrollbar/

And files in "/chrome/embed/skin/classic/global/icons/" are different.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 337498 [details] [diff] [review]
Core-only part of the patch

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Core patch checked in on cvs trunk, so closing this out.