416531 - Use chrome overrides to display Aero style icons on Vista

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

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

Comment 1

[Recall]

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

Comment 2

[Victor Bielawski]

(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.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

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.

Comment 4

[(no longer active)]

Attachment: Screenshot of Firefox with this patch applied on Vista

Comment 5

[(no longer active)]

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.

Comment 6

[Marcia Knous [:marcia]]

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.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

>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 8

[Asaf Romano (gone)]

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.

Comment 9

[Asaf Romano (gone)]

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, [e-mail], 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

Comment 10

[(no longer active)]

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.

Comment 11

[Mike Connor [:mconnor]]

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

Comment 12

[(no longer active)]

Attachment: Patch (v2)

(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.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 14

[Tony Chung [:tchung]]

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.

Comment 15

[Benjamin Smedberg]

Attachment: Fix cached chrome channels, rev. 1

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

Comment 16

[Asaf Romano (gone)]

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

f=mano

Comment 17

[Asaf Romano (gone)]

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 18

[Asaf Romano (gone)]

Comment on attachment 303897 [details] [diff] [review]
Patch (v2)

Let's not give this a try.

Comment 19

[(no longer active)]

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

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

--> P1; we need this for Beta 4

Comment 21

[Reed Loden [:reed]]

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 22

[[e-mail]]

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...

Comment 23

[[e-mail]]

Attachment: Fix IID

Comment 24

[Gabriel Chadwick]

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

Comment 25

[Asaf Romano (gone)]

Comment on attachment 304938 [details] [diff] [review]
Fix IID

Thanks Neil!

Comment 26

[Reed Loden [:reed]]

Comment on attachment 304938 [details] [diff] [review]
Fix IID

This is a blocker. No approval needed.

Comment 27

[Ed Hume]

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.

Comment 28

[Reed Loden [:reed]]

(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 29

[Asaf Romano (gone)]

Comment on attachment 303897 [details] [diff] [review]
Patch (v2)

Trying this instead.

Comment 30

[Asaf Romano (gone)]

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

Comment 31

[(no longer active)]

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

Comment 32

[(no longer active)]

(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.

Comment 33

[Dave Townsend [:mossop]]

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

Comment 34

[(no longer active)]

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

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

This will be fixed by bug 419319

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 37

[Mike Lierman]

*** VERIFIED

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

-Mike

Comment 38

[Stephen Donner [:stephend] Not actively reading bugmail]

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