Closed Bug 613037 Opened 9 years ago Closed 9 years ago

Add Zune and Royale themes to the default theme list

Categories

(Core :: Widget: Win32, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jimm, Assigned: jimm)

Details

Attachments

(1 file)

I'm not sure why we restrict this list to just aero and luna. The Fx button looks pretty crummy on Royale and Zune without this.
Attached patch patchSplinter Review
Attachment #491327 - Flags: review?(faaborg)
Comment on attachment 491327 [details] [diff] [review]
patch

can't do a code review, but in terms of the UI this looks like something we definitely want to have.
Attachment #491327 - Flags: review?(faaborg) → ui-review+
Comment on attachment 491327 [details] [diff] [review]
patch

super simple code change.
Attachment #491327 - Flags: review?(neil)
Comment on attachment 491327 [details] [diff] [review]
patch

I can see that this affects :-moz-system-metric(windows-default-theme) but I don't see that a lot in the tree, or am I overlooking something?
(In reply to comment #4)
> Comment on attachment 491327 [details] [diff] [review]
> patch
> 
> I can see that this affects :-moz-system-metric(windows-default-theme) but I
> don't see that a lot in the tree, or am I overlooking something?

Nope that's it. I see it used in a number of css files related to the browser, about 40 hits in 15 files according to mxr.
Comment on attachment 491327 [details] [diff] [review]
patch

OK, so I was misreading some of the CSS and I thought it was only checking the default theme for Aero when it was in fact checking it for Luna.

>   if (theme == WINTHEME_UNRECOGNIZED)
>     return;
> 
>-  if (theme == WINTHEME_AERO || theme == WINTHEME_LUNA)
>+  if (theme == WINTHEME_AERO ||
>+      theme == WINTHEME_LUNA ||
>+      theme == WINTHEME_ZUNE ||
>+      theme == WINTHEME_ROYALE)
>     sIsDefaultWindowsTheme = PR_TRUE;
Since the only recognised themes are Aero, Luna, Zune and Royale, then this condition will always be true, won't it?
(In reply to comment #6)
> Comment on attachment 491327 [details] [diff] [review]
> Since the only recognised themes are Aero, Luna, Zune and Royale, then this
> condition will always be true, won't it?

I suppose. Just setting sIsDefaultWindowsTheme = PR_TRUE though doesn't seem 'correct' since we could add additional themes. I guess if we assume than any recognized theme is automatically a default theme, it's ok to do that.
Comment on attachment 491327 [details] [diff] [review]
patch

OK, dao's convinced me that this is correct.

Is it worth adding a comment to the enumeration to update this list?
Attachment #491327 - Flags: review?(neil) → review+
(In reply to comment #8)
> Comment on attachment 491327 [details] [diff] [review]
> patch
> 
> Is it worth adding a comment to the enumeration to update this list?

Sure, will do.
Attachment #491327 - Flags: approval2.0?
Attachment #491327 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/68881eaeeebb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Any reason not to include Aero Basic (which is default on Win7 Starter versions) and Classic?
(In reply to comment #11)
> Any reason not to include Aero Basic (which is default on Win7 Starter
> versions) and Classic?

That's included in WINTHEME_AERO. We have separate metrics for detecting glass.
Could we also add the official "Windows Embedded Theme".

http://en.wikipedia.org/wiki/Windows_XP_themes
(In reply to comment #8)
> Comment on attachment 491327 [details] [diff] [review]
> patch
> 
> OK, dao's convinced me that this is correct.

Huh, how would I have done that?

We hardcode stuff based on windows-default-theme. Having more themes in that set makes this harder. I have no intention to test all Luna-targetting stuff (present or coming) with Zune and Royale, let alone Embedded.

Please back this out and file a bug on the Firefox button looking crummy. That's a bug on its own that potentially affects any non-default theme, so a generic fix is preferable -- if this doesn't work, though, there are queries to target zune and royale specifically.
Attachment #491327 - Flags: review-
(In reply to comment #14)
> (In reply to comment #8)
> > Comment on attachment 491327 [details] [diff] [review]
> > patch
> > 
> > OK, dao's convinced me that this is correct.
> 
> Huh, how would I have done that?
> 
> We hardcode stuff based on windows-default-theme. Having more themes in that
> set makes this harder. I have no intention to test all Luna-targetting stuff
> (present or coming) with Zune and Royale, let alone Embedded.
> 
> Please back this out and file a bug on the Firefox button looking crummy.
> That's a bug on its own that potentially affects any non-default theme, so a
> generic fix is preferable -- if this doesn't work, though, there are queries to
> target zune and royale specifically.

Maybe windows-default-theme should be depreciated? What it applies to seems confusing and as you point out, we have the theme metrics now.

backed out:
http://hg.mozilla.org/mozilla-central/rev/898e308926dc
Resolution: FIXED → WONTFIX
(In reply to comment #14)
> (In reply to comment #8)
> > Comment on attachment 491327 [details] [diff] [review]
> > patch
> > 
> > OK, dao's convinced me that this is correct.
> 
> Huh, how would I have done that?

My bad; what you actually did was convince me that comment #6 was incorrect.
You need to log in before you can comment on or make changes to this bug.