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

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kai Liu]

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

Comment 1

[Kai Liu]

Attachment: testcase

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

Comment 2

[Kai Liu]

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

Comment 3

[Kai Liu]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

somebody else should review this

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 6

[Kai Liu]

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

Comment 7

[Kai Liu]

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

Comment 8

[(no longer active)]

A comment would be nice stating why you don't handle Aero separately...

Comment 9

[Kai Liu]

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

Comment 10

[Kai Liu]

Attachment: 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!

Comment 11

[(no longer active)]

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

Comment 12

[Kai Liu]

Attachment: patch v3.2

Now with comments.  :)

(hehe, sorry for the misinterpretation, Ehsan)

Comment 13

[Kai Liu]

Attachment: 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.)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 15

[Kai Liu]

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

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 17

[Kai Liu]

Attachment: patch v4.1

Comment 18

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

Comment on attachment 316736 [details] [diff] [review]
patch v4.1

a1.9=beltzner

Comment 19

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/windows/nsLookAndFeel.cpp 	1.71