We need to start using the landed aero style icons on Vista, for instance: http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/Toolbar-aero.png As opposed to: http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/Toolbar.png Details on using chrome overrides can be found in bug 397073 and here: http://developer.mozilla.org/en/docs/Chrome_Registration#osversion

This site has some very nice userchrome.css tweaks if it helps? http://121212.no-ip.info/w/index.php/Firefox_3.0b3pre_and_3.0b4pre_Vista_icons_and_toolbar_tweaks_css

(In reply to comment #1) > This site has some very nice userchrome.css tweaks if it helps? > > http://121212.no-ip.info/w/index.php/Firefox_3.0b3pre_and_3.0b4pre_Vista_icons_and_toolbar_tweaks_css > Chrome overrides. Not userchrome.css.

This patch adds the aero icons which have landed so far to the appropriate jar manifest files, and injects chrome overrides for Windows Vista to replace those with their classic counterparts. I'll attach a screenshot of a build with this patch applied on vista shortly.

I think this deserves a simple test in litmus which involves opening the browser on XP and Vista and verifying that the skin in each instance is correctly chosen for that platform.

Nominating for the b4 release. This type of change needs to testing from QA, and we would prefer to get it in earlier in the cycle so it can be properly tested.

>Nominating for the b4 release. This type of change needs to testing from QA It is also very important that we get feedback from users on the icons themselves. We should try to get this into the nightly builds as soon as possible.

Comment on attachment 303476 [details] [diff] [review] Patch (v1) we used to have this bug about chrome overrides pointing to another chrome uri being randomly broken, sending this request to benjamin.

06:01] <reed> bsmedberg: when do you think you'll be able to review 416531? [06:01] <bsmedberg> firebot: bug 416531 [06:02] <firebot> bsmedberg: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=416531 nor, --, Firefox 3 beta4, ehsan.akhgari@gmail.com, ASSI, Use chrome overrides to display Aero style icons on Vista [06:02] <bsmedberg> reed: Tuesday [06:02] <reed> k, cool [06:12] <Mano> bsmedberg: i set that review request as "you can explain why this wouldn't work" better than me fwiw. [06:13] <Mano> as in, why override chrome:// chrome:// doesn't always work. [06:13] <bsmedberg> hrm, doesn't it? I thought we fixed that [06:13] <bsmedberg> as long as you take care not to introduce loops [06:14] <Mano> bsmedberg: i fixed that last time by duplicating the file in jar.mn [06:14] <bsmedberg> Mano: huh, I could sworn I saw a bug go by which fixed overrides so chrome->chrome would work [06:14] <Mano> bsmedberg: see https://bugzilla.mozilla.org/show_bug.cgi?id=380232 [06:14] <Mano> note "occasionally" [06:19] <bsmedberg> Mano: that's crazy! bah humbug, I hate that dveditz forced me to keep those bogus channel checks [06:20] <Mano> bsmedberg: i think i debugged this a bit back then, and if i called resolve for the resolved uri it always worked [06:21] <Mano> but there's no clear STR anyway :( [06:22] <bsmedberg> well, I see what was happening there... we're double-wrapping the URI, but the channel check didn't expect a cached-chrome-channel [06:22] <bsmedberg> so it would only be a problem in the case where you fastloaded the XUL file and then loaded it again [06:23] <bsmedberg> which shouldn't happen most of the time, but does

So, should we assume that the chrome override bug does not get fixed? The only other workaround that comes to my mind is having things like this in classic.manifest: skin browser classic/1.0 jar:classic.jar!/skin/classic/browser/ os=WINNT osversion<6 skin browser classic/1.0 jar:classic.jar!/skin/classic/browser-aero/ os=WINNT osversion>=6 skin browser classic/1.0 jar:classic.jar!/skin/classic/browser/ os!=WINNT And then fill skin/classic/browser-aero in the jar file with aero icons, and use normal icons for those which don't have aero icons landed yet. Other theme content (.css, .xml, etc) should also be duplicated in these categories. Since this leads to a hard to maintain jar.mn, I prefer someone to confirm that the override problem won't get fixed, before I modify the patch.

tbh, that's not that much more painful than the current situation. Probably not ideal, but workable enough for now...

(In reply to comment #11) > tbh, that's not that much more painful than the current situation. Probably > not ideal, but workable enough for now... OK, here is a patch implementing the approach in comment 10. mano: can you please review this, and if you think this is OK, feel free to make v1 obsolete. Since it's desired for this to land quickly, we can get this patch in, and modify it later to use the override mechanism in v1 when the bug with chrome override is fixed.

We definitely need this for Beta 4. Although Vista users don't currently make up a large proportion of Firefox users, as Alex has pointed out, they make up about 100% of tech reviewers.

Agree with Comment #7 and #13. Do we have an ETA on when this is landing on the nightly? We could use the extra days to look this over before code freeze.

This should allow through the cached chrome channel, which should allow us to reliably override chrome URIs with chrome URIs. fingers-crossed...

Comment on attachment 303476 [details] [diff] [review] Patch (v1) f=mano

Comment on attachment 304593 [details] [diff] [review] Fix cached chrome channels, rev. 1 the warning string probably needs a little update. r=mano otherwise, thanks!!

Comment on attachment 303897 [details] [diff] [review] Patch (v2) Let's not give this a try.

Thanks for the patch, Benjamin! This should be ready to go with mano's warning change request...

--> P1; we need this for Beta 4

I used "Remote chrome not allowed! Only file:, resource:, jar:, and cached chrome channels are valid!" for the warning. If that's wrong, please commit a fix. Checking in chrome/src/nsChromeProtocolHandler.cpp; /cvsroot/mozilla/chrome/src/nsChromeProtocolHandler.cpp,v <-- nsChromeProtocolHandler.cpp new revision: 1.123; previous revision: 1.122 done Checking in browser/themes/winstripe/browser/jar.mn; /cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v <-- jar.mn new revision: 1.69; previous revision: 1.68 done Checking in toolkit/themes/winstripe/global/jar.mn; /cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v <-- jar.mn new revision: 1.41; previous revision: 1.40 done Checking in toolkit/themes/winstripe/mozapps/jar.mn; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v <-- jar.mn new revision: 1.24; previous revision: 1.23 done

Comment on attachment 304593 [details] [diff] [review] Fix cached chrome channels, rev. 1 >+ NS_DECLARE_STATIC_IID_ACCESSOR(nsCachedChromeChannel) #define NS_DECLARE_STATIC_IID_ACCESSOR(the_iid) \ nsCachedChromeChannel isn't an iid...

verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre ID:2008022104

Comment on attachment 304938 [details] [diff] [review] Fix IID Thanks Neil!

Comment on attachment 304938 [details] [diff] [review] Fix IID This is a blocker. No approval needed.

I believe you have chosen an unwise way to proceed with this. Please look at this thread: http://forums.mozillazine.org/viewtopic.php?t=630829 Basically, you have broken every third party theme for the Vista environment. I proposed an addition to chrome.manifest that would reverse the classic.manifest overrides and restore our themes to working condition. It did not work. What this means is that all themers will have to: 1. Discover from unhappy Vista users that certain buttons - some obvious, some obscure - do not work. 2. Figure out what is wrong. 3. Figure out how to fix it. In the short term, we themers will have to duplicate, for each of our themes, each of the files you have reassigned. Each developer, each theme. That's a lot of extra MB running through Moz Update, and a lot of man-hours spent finding, figuring and fixing by all the many theme developers. So I ask: why break everybody else's themes? Is there another method? One thing that occurs to me is that you could split the Firefox builds. You have three now - Linux, OSX and Windows. You could have four: Linux, OSX, Windows and Vista. OK, I know that's a bit harsh, but what you are doing to third party developers is also harsh; don't forget, we haven't heard from extension developers yet. Or you could do something else. Like fixing the chrome overrides bug. Or - heaven forfend - leave the current default as the default, and supply the Aero theme as an Add-on at AMO. Or even package the Aero theme as an add-on that ships with each windows build. That way, a user - even a non-Vista user - could choose the Aero theme from the Add-ons window. Heck, you could even use your classic.manifest file to choose which theme would be the default, based on OS. This would give the Vista users the Aero experience ab initio without breaking everybody else's themes. But please withdraw your current fix.

(In reply to comment #27) > Basically, you have broken every third party theme for the Vista environment. Thanks for the report. I filed bug 419380 on this issue for better tracking.

Comment on attachment 303897 [details] [diff] [review] Patch (v2) Trying this instead.

overrides patch backed out: Checking in browser/themes/winstripe/browser/jar.mn; /cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v <-- jar.mn new revision: 1.70; previous revision: 1.69 done Checking in toolkit/themes/winstripe/global/jar.mn; /cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v <-- jar.mn new revision: 1.42; previous revision: 1.41 done Checking in toolkit/themes/winstripe/mozapps/jar.mn; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v <-- jar.mn new revision: 1.25; previous revision: 1.24 done

Requesting checkin of attachment 303897 [details] [diff] [review].

(In reply to comment #29) > (From update of attachment 303897 [details] [diff] [review]) > Trying this instead. This solution would remedy the problem mentioned in <http://forums.mozillazine.org/viewtopic.php?t=630829>, but it doesn't give theme designers any chance to ship Windows themes which can have different looks on XP and Vista. Not sure if that's something we should be considering though, just though I'd mention this.

Theme designers can use the same rules we are here to ship themes that differ on XP and Vista with no problem.

Oops, I was under the impression that flags can't be used in themes' manifests. I stand corrected.

This will be fixed by bug 419319

Bug 419319 is fixed, marking this one fixed, too.

*** VERIFIED Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008030607 Minefield/3.0b5pre -Mike

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre