User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041506 Minefield/3.0pre
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.
Steps to Reproduce:
Created attachment 315815 [details]
You can use Microsoft's standalone Zune theme for this test.
Created attachment 315816 [details] [diff] [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.
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.
somebody else should review this
Comment on attachment 315816 [details] [diff] [review]
Less code duplication, please; stick the strings in an array and iterate through it to see if any match.
Created attachment 315902 [details] [diff] [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.
Created attachment 315908 [details] [diff] [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...
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.
Created attachment 315910 [details] [diff] [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!
(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. ;-)
Created attachment 315917 [details] [diff] [review]
Now with comments. :)
(hehe, sorry for the misinterpretation, Ehsan)
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.)
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?
Created attachment 316728 [details] [diff] [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.
+ 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.
Created attachment 316736 [details] [diff] [review]
Comment on attachment 316736 [details] [diff] [review]