Closed Bug 429176 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: kliu, Assigned: kliu)

Details

Attachments

(2 files, 7 obsolete files)

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
Attached file testcase
You can use Microsoft's standalone Zune theme for this test.
http://go.microsoft.com/fwlink/?LinkID=75078
Attached patch patch (obsolete) — Splinter Review
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.
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → kliu.bugzilla.3c9f
Attachment #315816 - Flags: superreview?(dbaron)
Attachment #315816 - Flags: review?(dbaron)
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
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-
Attached patch patch v2 (obsolete) — Splinter Review
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)
Attached patch patch v3 (obsolete) — Splinter Review
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...
(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.
Attached patch patch v3.1 (obsolete) — Splinter Review
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.  ;-)
Attached patch patch v3.2 (obsolete) — Splinter Review
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)
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?
Attached patch patch v4 (obsolete) — Splinter Review
(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.
Attached patch patch v4.1Splinter Review
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+
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+
Keywords: checkin-needed
mozilla/widget/src/windows/nsLookAndFeel.cpp 	1.71
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.