Last Comment Bug 429176 - :-moz-system-metric(windows-default-theme) should treat Royale and Zune the same as Luna
: :-moz-system-metric(windows-default-theme) should treat Royale and Zune the s...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla1.9
Assigned To: Kai Liu
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-15 12:13 PDT by Kai Liu
Modified: 2010-08-14 03:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.14 KB, text/html)
2008-04-15 12:16 PDT, Kai Liu
no flags Details
patch (2.85 KB, patch)
2008-04-15 12:20 PDT, Kai Liu
vladimir: review-
Details | Diff | Splinter Review
patch v2 (2.30 KB, patch)
2008-04-15 21:54 PDT, Kai Liu
no flags Details | Diff | Splinter Review
patch v3 (2.40 KB, patch)
2008-04-15 22:26 PDT, Kai Liu
no flags Details | Diff | Splinter Review
patch v3.1 (2.39 KB, patch)
2008-04-15 23:09 PDT, Kai Liu
no flags Details | Diff | Splinter Review
patch v3.2 (2.73 KB, patch)
2008-04-16 00:11 PDT, Kai Liu
no flags Details | Diff | Splinter Review
patch v3.2.1, adjusted hanging else indent (2.67 KB, patch)
2008-04-19 18:54 PDT, Kai Liu
no flags Details | Diff | Splinter Review
patch v4 (2.63 KB, patch)
2008-04-20 14:45 PDT, Kai Liu
no flags Details | Diff | Splinter Review
patch v4.1 (2.62 KB, patch)
2008-04-20 16:47 PDT, Kai Liu
roc: review+
roc: superreview+
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description Kai Liu 2008-04-15 12:13:29 PDT
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 2008-04-15 12:16:12 PDT
Created attachment 315815 [details]
testcase

You can use Microsoft's standalone Zune theme for this test.
http://go.microsoft.com/fwlink/?LinkID=75078
Comment 2 Kai Liu 2008-04-15 12:20:11 PDT
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.
Comment 3 Kai Liu 2008-04-15 13:37:51 PDT
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-10 2008-04-15 13:39:04 PDT
somebody else should review this
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2008-04-15 20:29:08 PDT
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 2008-04-15 21:54:21 PDT
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.
Comment 7 Kai Liu 2008-04-15 22:26:41 PDT
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...
Comment 8 :Ehsan Akhgari 2008-04-15 22:42:50 PDT
A comment would be nice stating why you don't handle Aero separately...
Comment 9 Kai Liu 2008-04-15 22:57:42 PDT
(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 2008-04-15 23:09:09 PDT
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!
Comment 11 :Ehsan Akhgari 2008-04-15 23:39:14 PDT
(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 2008-04-16 00:11:46 PDT
Created attachment 315917 [details] [diff] [review]
patch v3.2

Now with comments.  :)

(hehe, sorry for the misinterpretation, Ehsan)
Comment 13 Kai Liu 2008-04-19 18:54:05 PDT
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.)
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-04-20 14:13:09 PDT
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 2008-04-20 14:45:57 PDT
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.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-04-20 16:07:58 PDT
+            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 2008-04-20 16:47:34 PDT
Created attachment 316736 [details] [diff] [review]
patch v4.1
Comment 18 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-21 14:25:02 PDT
Comment on attachment 316736 [details] [diff] [review]
patch v4.1

a1.9=beltzner
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-04-22 09:55:50 PDT
mozilla/widget/src/windows/nsLookAndFeel.cpp 	1.71

Note You need to log in before you can comment on or make changes to this bug.