The default bug view has changed. See this FAQ.

:-moz-system-metric(windows-default-theme) should treat Royale and Zune the same as Luna

VERIFIED FIXED in mozilla1.9

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: Kai Liu, Assigned: Kai Liu)

Tracking

Trunk
mozilla1.9
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041506 Minefield/3.0pre
Build Identifier: 

This is a follow-up to bug 425979 comment 36.  For people who installed Royale and Zune as separate standalone themes*, the detection mechanism added in bug 425979 treat them as third-party themes.  Since the goal of bug 425979 was to try to separate Classic from a themed appearance, both the standalone Royale and Zune themes should be detected, since they are basically Luna with a glassier texture, and they are both official Microsoft-signed themes.

* Note: AFAIK, the Royale that comes pre-installed is lumped in with Luna (I could be wrong about this), so only the standalone Royale would be affected.  While Microsoft no longer offers the standalone Royale for download, it has been mirrored in numerous places.  AFAIK, Microsoft is still offering the Zune theme as a standalone download.

Reproducible: Always

Steps to Reproduce:
N/A
(Assignee)

Comment 1

9 years ago
Created attachment 315815 [details]
testcase

You can use Microsoft's standalone Zune theme for this test.
http://go.microsoft.com/fwlink/?LinkID=75078
(Assignee)

Comment 2

9 years ago
Created attachment 315816 [details] [diff] [review]
patch

The patch for this is fairly straight-forward and trivial.  I changed wcsicmp (MSVCRT) to lstrcmpiW (kernel32) because Microsoft had deprecated the former and since we are already using kernel32's lstrlenW anyway.
(Assignee)

Updated

9 years ago
Version: unspecified → Trunk

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

9 years ago
Assignee: nobody → kliu.bugzilla.3c9f

Updated

9 years ago
Attachment #315816 - Flags: superreview?(dbaron)
Attachment #315816 - Flags: review?(dbaron)
(Assignee)

Comment 3

9 years ago
Just noticed that I had copied and pasted the wrong bug ID in the first comment.  Oops!  The correct bug ID is bug 426660, and the comment that I wanted to refer to was bug 426660 comment 36.  Sorry for the mixup.
Status: NEW → ASSIGNED
somebody else should review this
(Assignee)

Updated

9 years ago
Attachment #315816 - Flags: review?(dbaron) → review?(vladimir)
Comment on attachment 315816 [details] [diff] [review]
patch

Less code duplication, please; stick the strings in an array and iterate through it to see if any match.
Attachment #315816 - Flags: review?(vladimir) → review-
Attachment #315816 - Flags: superreview?(dbaron)
(Assignee)

Comment 6

9 years ago
Created attachment 315902 [details] [diff] [review]
patch v2

Yea, sorry, that first patch was a little too hastily thrown together.

We'll never see luna/royale/zune on Vista, nor aero on XP/2K3, so they can all handled by the same loop.  So with that change, the only purpose of the select is to throw a NS_ERROR_NOT_IMPLEMENTED for future versions of Windows (getCurrentThemeName is undefined on 2K).  Since the select isn't doing as much as it used to do, we could use an if instead, but it'll all look the same after the compiler is through with it.
Attachment #315816 - Attachment is obsolete: true
Attachment #315902 - Flags: review?(vladimir)
(Assignee)

Comment 7

9 years ago
Created attachment 315908 [details] [diff] [review]
patch v3

Ah, I didn't realize that GetWindowsVersion() returned an integer in the form of 0xMMmm (just found out after checking MXR; I'm still fairly new to Mozilla's code base).  So now that I can just use an inequality instead of testing for every version using select, I can simplify this patch even more...
Attachment #315902 - Attachment is obsolete: true
Attachment #315908 - Flags: review?(vladimir)
Attachment #315902 - Flags: review?(vladimir)
A comment would be nice stating why you don't handle Aero separately...
(Assignee)

Comment 9

9 years ago
(In reply to comment #8)
> A comment would be nice stating why you don't handle Aero separately...
> 

I did, in comment 6.  ;)  Vista's aero.msstyles isn't compatible with XP's theming engine, likewise for the other styles and Vista, so we can be pretty sure that we'll only see aero.msstyles in Vista/2K8 and the others in XP/2K3.
(Assignee)

Comment 10

9 years ago
Created attachment 315910 [details] [diff] [review]
patch v3.1

Same as patch v3, except using the NS_ARRAY_LENGTH macro (another thing that I discovered on MXR :P).  Sorry for the bug spam!
Attachment #315908 - Attachment is obsolete: true
Attachment #315910 - Flags: review?(vladimir)
Attachment #315908 - Flags: review?(vladimir)
(In reply to comment #9)
> (In reply to comment #8)
> > A comment would be nice stating why you don't handle Aero separately...
> > 
> 
> I did, in comment 6.  ;)  Vista's aero.msstyles isn't compatible with XP's
> theming engine, likewise for the other styles and Vista, so we can be pretty
> sure that we'll only see aero.msstyles in Vista/2K8 and the others in XP/2K3.

Yeah, I know!  /A comment/A comment in the patch/s.  ;-)
(Assignee)

Comment 12

9 years ago
Created attachment 315917 [details] [diff] [review]
patch v3.2

Now with comments.  :)

(hehe, sorry for the misinterpretation, Ehsan)
Attachment #315910 - Attachment is obsolete: true
Attachment #315917 - Flags: review?(vladimir)
Attachment #315910 - Flags: review?(vladimir)
(Assignee)

Comment 13

9 years ago
Created attachment 316643 [details] [diff] [review]
patch v3.2.1, adjusted hanging else indent

Tweaked formatting of the "else"

I also want to correct something that I incorrectly stated in comment 0.  According to someone who uses XP Media Center, in a standard MCE installation, the Royale theme is located in its own file separate from Luna, and Royale is the default theme for the system, not Luna.  (I had previously thought that Royale was in a separate file only if Royale was installed as an add-on to non-MCE machines.)
Attachment #315917 - Attachment is obsolete: true
Attachment #316643 - Flags: superreview?(roc)
Attachment #316643 - Flags: review?(vladimir)
Attachment #315917 - Flags: review?(vladimir)
so if there was a theme called "myluna", this would guess it's a default theme? that seems wrong. How about just extracting the file name by finding the trailing path separator and then doing a plain compare against each candidate name?
(Assignee)

Comment 15

9 years ago
Created attachment 316728 [details] [diff] [review]
patch v4

(In reply to comment #14)
> so if there was a theme called "myluna"

Good point.  I hadn't thought of that scenario, and the existing code will falter in that case, too.
Attachment #316643 - Attachment is obsolete: true
Attachment #316728 - Flags: superreview?(roc)
Attachment #316728 - Flags: review?(vladimir)
Attachment #316643 - Flags: superreview?(roc)
Attachment #316643 - Flags: review?(vladimir)
+            LPWSTR curTheme = wcsrchr(themeFileName, L'\\') + 1;
+            if (curTheme < themeFileName) curTheme = themeFileName;

This seems dodgy. I'd prefer a direct check for wcsrchr returning failure.

Otherwise looks good.
(Assignee)

Comment 17

9 years ago
Created attachment 316736 [details] [diff] [review]
patch v4.1
Attachment #316728 - Attachment is obsolete: true
Attachment #316736 - Flags: superreview?(vladimir)
Attachment #316736 - Flags: review?(roc)
Attachment #316728 - Flags: superreview?(roc)
Attachment #316728 - Flags: review?(vladimir)
Attachment #316736 - Flags: superreview?(vladimir)
Attachment #316736 - Flags: superreview+
Attachment #316736 - Flags: review?(roc)
Attachment #316736 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #316736 - Flags: approval1.9?
Comment on attachment 316736 [details] [diff] [review]
patch v4.1

a1.9=beltzner
Attachment #316736 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
mozilla/widget/src/windows/nsLookAndFeel.cpp 	1.71
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
(Assignee)

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.