543910 - [Windows] Improve Windows OS theme detection for Firefox.next

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

We currently have a CSS selector to determine if a Windows users is running a Windows 2000 style classic theme, or one of the many default themes in newer versions of Windows (including variants of Luna, Royale, and Aero).

This doesn't give us the level of control that we are going to need with the new Firefox theme work to visually integrate.

As a follow up to bug 426660, we should use the GetCurrentThemeName function of uxtheme in Windows (or an alternative approach) to more directly identify which OS theme is being used.  There is some overlap between themes and different versions of Windows, so I'll attach a diagram showing the ways in which we will need to be able to differentiate.

I should note that just because we have the ability to tell which OS theme the user is running, doesn't mean we are planning on having a much larger number of Firefox theme variations.  We will likely only occasionally need to leverage this functionality, but some specific examples include:

-Drawing the correct toolbar style in the bookmarks organizer between Vista and 7 (black vs. light blue)
-Drawing a custom toolbar on XP to match the general feel of Luna (but perhaps with a vertical gradient instead of a horizontal one).
-Drawing the application button, and possibly window controls
-Drawing a small window when performing a tab tear off
-Specialized high contrast icons for the Black and White accessibility themes

Those are just the scenarios off the top of my head, there might be a few more.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Visual guide to current windows themes

This shows the current set of OS themes across all Windows versions we support grouped into the sets that we will need to select on.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Nominating to block Firefox.next since we can't complete the new theme work without the functionality.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Examples of how the new theme will leverage this capability

Here are three quick examples of areas where we will have to know the OS theme in order to correctly style the new Firefox window.

Comment 4

[Brian Polidoro]

One theme that is missing is the XP Zune theme.  That's similar to the XP Royale theme.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

>One theme that is missing is the XP Zune theme.

Yeah, since Zune never actually shipped with Windows (unlike Royale which was included in Media Center and Tablet editions), I didn't initially consider it.  And then shortly later I'm in my dentist's office and they are running the Zune theme.

Not sure where we want to draw the line with theme detection, we could for instance fall back to the same set of icons planned for personas when we don't know the theme (for instance Zune, or something from a third party modification like window blinds).  Or we could group Zune in as a special case, but generally only consider themes that we believe have a certain market share.

Also, (and I don't mean this as negatively as it sounds) I wouldn't be surprised if every single one of the shipping windows OS themes had more users than OS X's aqua.  Perhaps with the exception of Luna Olive.

Comment 6

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

(In reply to comment #0)
> We currently have a CSS selector to determine if a Windows users is running a
> Windows 2000 style classic theme, or one of the many default themes in newer
> versions of Windows (including variants of Luna, Royale, and Aero).

For the record, all of our :-moz-system-metric() selectors now double as -moz-* CSS media queries.

So the patch to fix this should presumably touch:
 * nsILookAndFeel.h and its Windows implementation in widget/src/windows/ (and likely stubs for the other implementations)
 * InitSystemMetrics in nsCSSRuleProcessor.cpp
 * nsMediaFeatures::features in nsMediaFeatures.cpp

However, if we have the ability to determine the theme name (rather than just detecting certain known themes), we *might* (although not necessarily) want to have a selector with a parameter, and likewise a media query that takes values.  (Or, optionally, maybe only a media query.)

See also bug 546816.  We might want the CSS side to be similar to whatever we do there.

Comment 7

[Damon Sicore (:damons)]

dbaron, who's the best person to fix this?

Comment 8

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

Somebody who knows the Windows APIs for themes.  I could help with the style system end if needed, but that end is pretty simple.

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

>Somebody who knows the Windows APIs for themes.

Or alternatively we just look at the moz- colors being extracted and infer the OS theme from that.  The Windows API for themes may group some of these together because they are stored in the same file (although I'm not entirely sure I remember that correctly).  Ehsan worked on the last attempt of this (https://bugzilla.mozilla.org/show_bug.cgi?id=426660#c15 ), so he might know more

Comment 10

[(no longer active)]

I'm not sure what this bug is really about.  We are already using GetCurrentThemeName in order to implement those media queries.  Is there anything else remaining to do here?

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

We need to differentiate between the following 9 cases:

classic
aero (on 7)
aero (on vista)
royale
luna blue
luna silver
luna olive
high contrast black (all three types)
high contrast white

current we can only differentiate on 2 cases:

classic
not classic

Comment 12

[(no longer active)]

So, how do you want these exposed to CSS?

GetCurrentThemeName returns a file name, a color scheme and a size. <http://msdn.microsoft.com/en-us/library/bb773365%28VS.85%29.aspx>  We currently only use the file name and match it against three known files: <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsLookAndFeel.cpp#519>.  I mean, the code change here is trivial - no special knowledge of the Windows API is required.  We just need to figure out how we want to expose this information to CSS.

Comment 13

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

What does GetCurrentThemeName return for the 9 cases Alex cares about?

Comment 14

[(no longer active)]

(In reply to comment #13)
> What does GetCurrentThemeName return for the 9 cases Alex cares about?

> classic

I think it just returns an error in this case.

> aero (on 7)

aero.msstyles.  We have the Windows version using GetWindowsVersion.

> aero (on vista)

aero.msstyles.  We have the Windows version using GetWindowsVersion.

> royale

royale.msstyles.

> luna blue
> luna silver
> luna olive

luna.msstyles.  I think we could use the color returned from the API to differentiate between these three cases.

> high contrast black (all three types)
> high contrast white

For this, we should call SystemParametersInfo with SPI_GETHIGHCONTRACT <http://msdn.microsoft.com/en-us/library/ms724947%28VS.85%29.aspx>.  The color can be determined through the lpszDefaultScheme member of the HICHCONTRACT data structure: <http://msdn.microsoft.com/en-us/library/dd318112%28v=VS.85%29.aspx>.


I haven't personally tested any of the above, though.  Maybe someone with access to XP and Vista machines can?  CCing jimm and robarnold to see if they can help with this.

Comment 15

[Rob Arnold [:robarnold]]

(In reply to comment #14)
> I haven't personally tested any of the above, though.  Maybe someone with
> access to XP and Vista machines can?  CCing jimm and robarnold to see if they
> can help with this.

I'm afraid I don't have easy access to an XP or Vista machine but your approach looks good.

How can we efficiently expose this to the style system? a -moz-theme-name pseudo-class that takes the theme name as a string? I don't think we should hardcode the expected theme names into the build system if possible but I also worry that string comparison may be too slow for performance.

Comment 16

[Johnathan Nightingale [:johnath]]

Dao, Gavin, Shorlander - how do we want this exposed to CSS? Ehsan thinks the work is pretty straightforward once we know how we want the CSS to look.

Comment 17

[Dão Gottwald [:dao]]

This bug has too many goals, all of which either don't need style system changes, aren't fleshed out yet or don't block upcoming theme work.

> -Drawing the correct toolbar style in the bookmarks organizer between Vista and
> 7 (black vs. light blue)

Distinguishing between Vista and 7 can be done with manifest flags.

> -Drawing a custom toolbar on XP to match the general feel of Luna (but perhaps
> with a vertical gradient instead of a horizontal one).

The -moz-Dialog color is all we need here, I think.

> -Drawing the application button, and possibly window controls

No special detection needed for Vista/7, and AFAIK work on drawing in the title bar on XP hasn't started yet, so it's not clear what would be needed there.

> -Drawing a small window when performing a tab tear off

Need someone actually working on this before we can talk about how it could be styled.

> -Specialized high contrast icons for the Black and White accessibility themes

Don't think this is needed, the current toolbar buttons on trunk are designed to work with dark and light personas and already have a higher contrast compared to 3.6.

Comment 18

[Stephen Horlander [:shorlander]]

(In reply to comment #17)
> > -Specialized high contrast icons for the Black and White accessibility themes
> 
> Don't think this is needed, the current toolbar buttons on trunk are designed
> to work with dark and light personas and already have a higher contrast
> compared to 3.6.

The new style does work acceptably on black backgrounds. A specific set of white glyphs and darker toolbar buttons would be better though.


What I am specifically interested in is native theme targeted graphics/colors that adapt to the system theme on the fly.

For example a different set of glyphs for LunaBlue/Olive vs. Silver/Royale vs. Zune. Brownish, silverish and black respectively. Also there are certain instances where translucent white/black fills just don't work well enough. Background tabs in particular could stand to have some targeted colors.

So maybe a something like:

-moz-win-xptheme(luna-blue)
-moz-win-xptheme(luna-olive)
-moz-win-xptheme(silver)
-moz-win-xptheme(zune)
-moz-win-xptheme(royale)

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

>This bug has too many goals, all of which either don't need style system
>changes, aren't fleshed out yet or don't block upcoming theme work.

This isn't complicated, Stephen has graphics designed for a variety of different windows OS themes, and we want to be able to use them.

>The -moz-Dialog color is all we need here, I think.

Limiting ourselves to extracted system colors and layering white and black gradients on top of them ties our hands.  We've had this debate for far too long, it's time to end the purple toolbar approach to windows theme development.  Without this bug much of Stephen's work will be effectively useless.

>and AFAIK work on drawing in the title
>bar on XP hasn't started yet, so it's not clear what would be needed there.

We could just as easily go over to that bug and claim that we don't need to make progress because even if we have the ability to draw in the title bar we won't be able to match the OS theme, and that work isn't complete yet.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> >This bug has too many goals, all of which either don't need style system
> >changes, aren't fleshed out yet or don't block upcoming theme work.
> 
> This isn't complicated, Stephen has graphics designed for a variety of
> different windows OS themes, and we want to be able to use them.

Many of the presented goals aren't related to Stephen's mockups, which was actually part of my criticism...

> >The -moz-Dialog color is all we need here, I think.
> 
> Limiting ourselves to extracted system colors and layering white and black
> gradients on top of them ties our hands.  We've had this debate for far too
> long, it's time to end the purple toolbar approach to windows theme
> development.  Without this bug much of Stephen's work will be effectively
> useless.

This is mixing different things. The purple toolbar is on Vista, and it's because we don't use Glass. There's no such issue on XP. I don't think anyone has looked into adjusting the toolbar gradient on XP yet, so assuming we'd need extra theme detection for this lacks substance.

> >and AFAIK work on drawing in the title
> >bar on XP hasn't started yet, so it's not clear what would be needed there.
> 
> We could just as easily go over to that bug and claim that we don't need to
> make progress because even if we have the ability to draw in the title bar we
> won't be able to match the OS theme, and that work isn't complete yet.

No, because that wouldn't make sense.

Comment 21

[Alex Faaborg [:faaborg] (Firefox UX)]

>Many of the presented goals aren't related to Stephen's mockups, which was
>actually part of my criticism...

We don't have any mockups of a new tab tear off appearance yet (well, that I know of), and we might not be fixing that for Firefox 4.  However, many aspects of Stephen's mockups from toolbar colors to drawing window controls are currently dependent on this ability. 

>This is mixing different things. The purple toolbar is on Vista, and it's
>because we don't use Glass.

It seems like we are really on different pages on this, I'll attach some crops of the mockups I'm thinking about.

>that wouldn't make sense.

Our ability to draw native appearing window controls over a persona has already been raised over in that bug, and is a very reasonable and significant concern.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Our ability to draw native appearing window controls over a persona has already
> been raised over in that bug, and is a very reasonable and significant concern.

All the recent comments in that bug seemed to be about Vista and 7.

Comment 23

[Alex Faaborg [:faaborg] (Firefox UX)]

>All the recent comments in that bug seemed to be about Vista and 7.

Drawing native appearing window controls over a persona is something we want to be able to do for every Windows OS theme.

Comment 24

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Non-extractable toolbar color

This image shows a mockup of our toolbar for Windows 7.  The colors in the toolbar range from (245, 249, 252) on the top to (227, 237, 246) at the bottom.  There are two reasons why this bug comes into play:

1) It is (as far as I can tell) impossible to derive these specific color values through white and black transparent overlays on top of an extractable system color, (like the grey dialog box, or the purple toolbar)

2) Once we use these values for Windows 7, they will carry over to XP Luna, XP Royale, etc.  In those contexts, we would like to use other gradients.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #23)
> >All the recent comments in that bug seemed to be about Vista and 7.
> 
> Drawing native appearing window controls over a persona is something we want to
> be able to do for every Windows OS theme.

We don't want to. We may have to for technical reasons, but that has only been discussed for Vista+. Besides, I don't think a persona in the title bar makes sense on XP, as the title bar completes the window border. And again, no mockup backs this idea.

(In reply to comment #24)
> Created an attachment (id=446847) [details]
> Non-extractable toolbar color
> 
> This image shows a mockup of our toolbar for Windows 7.  The colors in the
> toolbar range from (245, 249, 252) on the top to (227, 237, 246) at the bottom.
>  There are two reasons why this bug comes into play:
> 
> 1) It is (as far as I can tell) impossible to derive these specific color
> values through white and black transparent overlays on top of an extractable
> system color, (like the grey dialog box, or the purple toolbar)
> 
> 2) Once we use these values for Windows 7, they will carry over to XP Luna, XP
> Royale, etc.  In those contexts, we would like to use other gradients.

The difference between (227, 237, 246) and (245, 249, 252) is a white overlay, so the only issue is the base color, which is extractable on XP.

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

>We don't want to....no mockup backs this idea.

This is a miscommunication on our XP mockups, but I believe we very much do want to apply the persona to the title bar across every platform.  Personas can be a wide range of colors and styles and don't really mesh well with highly saturated blue or olive title bars.  Similarly they don't really mesh well highly saturated blue or olive window controls, in which case we would need to design native appearing (but transparent) versions.

>the only issue is the base color

Right, and light blue is commonly used throughout Windows 7, despite the fact that this is apparently a non-extractable base color.

Comment 27

[Stephen Horlander [:shorlander]]

Attachment: Windows XP Tab/Toolbar Gradients

The light-to-dark tab and toolbar gradient could be achieved by a white overlay on an extracted color. Or in the case of Vista/7 an established base color.

What can't be achieved by just overlaying universal translucent gradients/colors are things like the background tabs, borders and shadows. Well, it could, but it would be flat :) That is probably a good fallback if they are using a non-official theme.

If we could affect the hue and saturation of extracted colors then that would be viable. Or if we had the option to use different blend modes (multiply, screen, etc.)

One-size-fits-all also doesn't account for theme specific glyphs, variants on the Firefox Button style, theme specific button hovers or really any type of system theme specific tweaks that would be needed. Right now for instance the toolbar button gradients have a slight blue hue to them because they were designed for Vista/7 Aero Glass first. This probably isn't ideal for most XP themes.

Regardless we need the ability to target a specific theme for best possible design fidelity. That could be through a -moz CSS attribute or a separate CSS file like we do for Aero.

Comment 28

[Jim Mathies [:jimm]]

FWIW, rendering in the nc client area has mostly been fleshed out on XP and up. There are some plumbing issues being worked on with widgets, but the rendering side of things is pretty well understood.

> We don't want to. We may have to for technical reasons, but that has only been
> discussed for Vista+. Besides, I don't think a persona in the title bar makes
> sense on XP, as the title bar completes the window border. And again, no mockup
> backs this idea.

I'd be interested in seeing a personas mockup like this on XP for reference in my work.

Comment 29

[d]

(In reply to comment #21)
> >Many of the presented goals aren't related to Stephen's mockups, which was
> >actually part of my criticism...
> 
> We don't have any mockups of a new tab tear off appearance yet (well, that I
> know of), and we might not be fixing that for Firefox 4.

There is, actually. See: http://www.stephenhorlander.com/images/blog-posts/tab-animation/animation-tab-tear-off.html from http://blog.stephenhorlander.com/2010/01/29/tab-animation

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #26)
> >the only issue is the base color
> 
> Right, and light blue is commonly used throughout Windows 7, despite the fact
> that this is apparently a non-extractable base color.

We can target Windows 7 already. However, if that blue is actually used commonly, I'd expect it to be accessible. Has someone looked into that?

Comment 31

[Jim Mathies [:jimm]]

Attaching some extracted colors from various styles. The hierarchy of settings is as follows:

windows theme
  visual style
    system colors

System colors are relied on for all classic desktops. Visual styles are usually made up of a set of system colors and additional custom colors. Themes incorporate both styles and system colors, as well as other more physical resources like texture bitmaps, sounds, cursors, icons, and desktop images.

Note, on Vista and Win7, glass colorization is part of the theme, and can be pulled from the registry. 

reference: http://msdn.microsoft.com/en-us/library/bb773194(VS.85).aspx

I'm not sure what we need here, but if it's based on these color settings we can or already do expose them.

Comment 32

[Jim Mathies [:jimm]]

Attachment: colors zip

Comment 33

[Rob Arnold [:robarnold]]

(In reply to comment #31)
> Note, on Vista and Win7, glass colorization is part of the theme, and can be
> pulled from the registry. 

No need for the fragile registry - DwmGetColorizationColor does what we want and there's the WM_DWMCOLORIZATIONCOLORCHANGED message for live changes so we can cache it.

Comment 34

[Dão Gottwald [:dao]]

On <jar:https://bug543910.bugzilla.mozilla.org/attachment.cgi?id=446922!/aero-basic.html>, the page for aero basic, what sounds promising from its name and is close to (227, 237, 246) is VSCLASS_CONTROLPANEL/CPANEL_NAVIGATIONPANE/TMT_GRADIENTCOLOR1 with (220, 230, 244) and ~/TMT_FILLCOLORHINT with (226, 236, 249).

Comment 35

[Alex Faaborg [:faaborg] (Firefox UX)]

If we can base part of the theme on extractable resources, that's ideal.  The purpose of this bug however is to have a fall back, to be able to use resources that match the OS theme in cases where they are not directly extractable (modified textured bitmaps, modified window controls, etc.)

Comment 36

[Paul Buonopane]

If we want to draw over the titlebar, which I highly recommend working toward, we're going to need to really refine how we style the windows.  We need to draft up a specific policy defining "best practices" for drawing under specific circumstances.  This wasn't as much of a problem before, as there wasn't alpha transparency to worry about much.  With the introduction of glass, though, we have a whole new set of problems.

Google Chrome as already run into problems in this area.  They incorrectly replace the titlebar, triggering the bug that I've outlined at http://www.earth2me.com/development/dwm.  Window styling is becoming more and more fickle as other APIs, such as DWM, progress.  Really, our best bet is to completely avoid native controls and instead define our own controls based on the theme.  Microsoft has already started doing this with WPF; a perfect example is Visual Studio 2010, which visibly replaces all controls.

A good model for titlebar redrawing is Microsoft Office.  The tricky part is marking the titlebar as part of the client area while still drawing the native minimize, maximize, and close controls.  The ribbon does this rather well in Office 2007, though there's still a slight cosmetic bug in 2010.

To make a long story short, theme detection doesn't have to be so complex.  If we pick a method of drawing our windows and stick to it, we shouldn't run into any problems.  We need to pick a technique that is EXTENSIBLE, will have PREDICTABLE results in the future, and will NOT APPEAR HACKED UP.  If we're leaning toward redrawing the title area, let's just do it.  Get the hard part out of the way and open it up to future extensibility.  We just need to make sure we do it right the first time--no hacks or stuff that are going to come back to hit us in the face, like what happened with Google Chrome.  Bugs didn't surface until people started using strange DPI settings.  We don't want a repeat of that.

Comment 37

[Allison Naaktgeboren :ally]

If no one objects, I'd like to take this.

Comment 38

[Jim Mathies [:jimm]]

Attachment: theme detection routines

Not sure where we are on this, but this is a snippet of code I'm using based on the LookAndFeel code to detect themes for more accurate minimum widget dimensions in bug 574454. If we could expose something like this through the work here that would be great. It would be easy to expose the various color schemes as well on top of this. (LookAndFeel code should probably call into nsUXThemeData for this info, rather than generate it's own.)

Alex - why do we need high contrast detection? AFAICT, those are just color schemes for the classic theme.

Comment 39

[Allison Naaktgeboren :ally]

Jim, 'high contrast' is an overloaded term. There is the mode and themes-that-happen-to-be-high-contrast.   It is important to detect it for accessability.  The web isn't just for seeing folks with good joints :).  

The mode is indicated by Hcf_highcontraston. Programs are supposed detect the flag and adapt  for the visually impaired user and/or screen reading and speech software.  For designers this typically means eliminating gradients, backgrounds, a new set of glyphs, etc.  

There are also high contrast color themes, which are in theory like any other theme except they happen to have high key color choices.  

In xp, a high contrast scheme is selected independenty of the mode. ( from
 the accesability and display manager respectively), but in vista ( and Im guessing win7), when a high contrast color scheme is selected, windows set the mode flag too.

Comment 40

[Jim Mathies [:jimm]]

(In reply to comment #39)
> Jim, 'high contrast' is an overloaded term. There is the mode and
> themes-that-happen-to-be-high-contrast.   It is important to detect it for
> accessability.  The web isn't just for seeing folks with good joints :).  
> 
> The mode is indicated by Hcf_highcontraston. Programs are supposed detect the
> flag and adapt  for the visually impaired user and/or screen reading and speech
> software.  For designers this typically means eliminating gradients,
> backgrounds, a new set of glyphs, etc.  
> 
> There are also high contrast color themes, which are in theory like any other
> theme except they happen to have high key color choices.  
> 
> In xp, a high contrast scheme is selected independenty of the mode. ( from
>  the accesability and display manager respectively), but in vista ( and Im
> guessing win7), when a high contrast color scheme is selected, windows set the
> mode flag too.

Understood. We provide that flag through look and feel, but I'm not sure it's exposed through css. Independent of that issue though the high contrast color schemes do not need to be exposed via the theme detection (this bug) since all they are are color schemes, something we already have access to through system colors and css. (Which is why I was curious since we seem to handle high contrast color schemes pretty well.)

Comment 41

[Alex Faaborg [:faaborg] (Firefox UX)]

>glass colorization is part of the theme, and can be
>pulled from the registry. 
>
>reference: http://msdn.microsoft.com/en-us/library/bb773194(VS.85).aspx
>
>I'm not sure what we need here, but if it's based on these color settings we
>can or already do expose them.

We would likely make use of this information if we can make it available to the theme (for instance progress bars might be based on the glass color instead of being green).  Should we file a follow up bug for getting access to the glass color or merge it with this bug's work?

Comment 42

[Rob Arnold [:robarnold]]

I think a separate bug would be better.

Comment 43

[Alex Faaborg [:faaborg] (Firefox UX)]

Filed bug 578780

Comment 44

[Allison Naaktgeboren :ally]

Attachment: work in progress patch

wip patch, for your disapproval.  Among other things, there are soft tabs, stray comments, and some inelegant plumbing including hardcoded strings.  

Tested only on XP, because I dont have a win7/vista box.

Comment 45

[(no longer active)]

You need to set your code editor to use 2 spaces instead of tabs, and also use the unix line endings (LF).  The patch is hard to read without these changes.

Comment 46

[Allison Naaktgeboren :ally]

(In reply to comment #45)
> You need to set your code editor to use 2 spaces instead of tabs, and also use
> the unix line endings (LF).  The patch is hard to read without these changes.

I will reformat and repost tonight.

Comment 47

[Allison Naaktgeboren :ally]

Attachment: round, hopefully with legible formatting

whitespace should be better, look s good in my vim anyway.

Comment 48

[Rob Arnold [:robarnold]]

Comment on attachment 460441 [details] [diff] [review]
round, hopefully with legible formatting

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -1682,24 +1682,26 @@ GK_ATOM(scrollbar_end_forward, "scrollba
> GK_ATOM(scrollbar_thumb_proportional, "scrollbar-thumb-proportional")
> GK_ATOM(images_in_menus, "images-in-menus")
> GK_ATOM(images_in_buttons, "images-in-buttons")
> GK_ATOM(windows_default_theme, "windows-default-theme")
> GK_ATOM(mac_graphite_theme, "mac-graphite-theme")
> GK_ATOM(windows_compositor, "windows-compositor")
> GK_ATOM(windows_classic, "windows-classic")
> GK_ATOM(touch_enabled, "touch-enabled")
> GK_ATOM(maemo_classic, "maemo-classic")
>+GK_ATOM(moz_windows_color_theme, "moz-windows-color-theme")
> 
> // And the same again, as media query keywords.
> GK_ATOM(_moz_scrollbar_start_backward, "-moz-scrollbar-start-backward")
> GK_ATOM(_moz_scrollbar_start_forward, "-moz-scrollbar-start-forward")
> GK_ATOM(_moz_scrollbar_end_backward, "-moz-scrollbar-end-backward")
> GK_ATOM(_moz_scrollbar_end_forward, "-moz-scrollbar-end-forward")
> GK_ATOM(_moz_scrollbar_thumb_proportional, "-moz-scrollbar-thumb-proportional")
> GK_ATOM(_moz_images_in_menus, "-moz-images-in-menus")
> GK_ATOM(_moz_images_in_buttons, "-moz-images-in-buttons")
> GK_ATOM(_moz_windows_default_theme, "-moz-windows-default-theme")
> GK_ATOM(_moz_mac_graphite_theme, "-moz-mac-graphite-theme")
> GK_ATOM(_moz_windows_compositor, "-moz-windows-compositor")
> GK_ATOM(_moz_windows_classic, "-moz-windows-classic")
> GK_ATOM(_moz_touch_enabled, "-moz-touch-enabled")
> GK_ATOM(_moz_maemo_classic, "-moz-maemo-classic")
>+GK_ATOM(_moz_windows_color_theme, "-moz-windows-color-theme")

Why are these defined twice? If this only is intended to support media queries, then why do we have the first definition?

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp
>+++ b/layout/style/nsCSSParser.cpp
>@@ -1861,18 +1861,21 @@ CSSParserImpl::ParseMediaQueryExpression
>         expr->mValue.SetFloatValue(mToken.mNumber, eCSSUnit_Centimeter);
>       } else {
>         rv = PR_FALSE;
>       }
>       break;
>     case nsMediaFeature::eEnumerated:
>       rv = ParseVariant(expr->mValue, VARIANT_KEYWORD,
>                         feature->mData.mKeywordTable);
>       break;
>+	case nsMediaFeature::eString:
>+      rv = ParseVariant(expr->mValue, VARIANT_STRING, nsnull);
>+      break;

The case line still has a hard tab.

>diff --git a/layout/style/nsCSSStyleSheet.cpp b/layout/style/nsCSSStyleSheet.cpp
>--- a/layout/style/nsCSSStyleSheet.cpp
>+++ b/layout/style/nsCSSStyleSheet.cpp
>@@ -300,18 +300,30 @@ nsMediaExpression::Matches(nsPresContext
>         NS_ASSERTION(required.GetUnit() == eCSSUnit_Enumerated,
>                      "bad required value");
>         NS_ASSERTION(mFeature->mRangeType == nsMediaFeature::eMinMaxNotAllowed,
>                      "bad range"); // we asserted above about mRange
>         // We don't really need DoCompare, but it doesn't hurt (and
>         // maybe the compiler will condense this case with eInteger).
>         cmp = DoCompare(actual.GetIntValue(), required.GetIntValue());
>       }
>       break;
>+	case nsMediaFeature::eString:
>+		{
>+        NS_ASSERTION(actual.GetUnit() == eCSSUnit_String,
>+                     "bad actual value");
>+        NS_ASSERTION(required.GetUnit() == eCSSUnit_String,
>+                     "bad required value");
>+        NS_ASSERTION(mFeature->mRangeType == nsMediaFeature::eMinMaxNotAllowed,
>+                     "bad range"); 
>+		cmp = !(required == actual);
>+		// anaaktge cmp needs to be 0 to indicate success 
>+		}
>+		break;

More strange indentation here still - hard tabs can be replaced by 2 spaces in vim with :%s/^I/  /g where ^I is generated by pressing the tab key.

What's up with the comment?

>+static NS_DEFINE_CID(kLookAndFeelCID, NS_LOOKANDFEEL_CID);

Could probably move this into GetWindowsThemeName

>+static nsresult
>+GetWindowsThemeName(nsPresContext* aPresContext, const nsMediaFeature* aFeature,
>+                nsCSSValue& aResult)

nit: indentation

Also might want to consider making this more generic like GetSystemMetric. In fact, I wonder if these could be merged. I think dbaron is the best person to comment on this though. There's already a metric for OSX's Graphite theme so there's already some potential gain.

>+        if (NS_FAILED(rv)) {
>+                return rv; 
>+        }
>+        aResult.SetStringValue(themeName, eCSSUnit_String);
>+        return NS_OK;   
>+}

How about:

if (NS_SUCCEEDED(rv)) {
  aResult.SetStringValue(themeName, eCSSUnit_String);
}
return rv;



>     // Null-mName terminator:
>     {
>         nsnull,
>         nsMediaFeature::eMinMaxAllowed,
>         nsMediaFeature::eInteger,
>         { nsnull },
>         nsnull
>-    },
>+    }
> };

Unnecessary change?

>     enum ValueType {
>         // All value types allow eCSSUnit_Null to indicate that no value
>         // was given (in addition to the types listed below).
>         eLength,     // values are such that nsCSSValue::IsLengthUnit() is true
>         eInteger,    // values are eCSSUnit_Integer
>         eBoolInteger,// values are eCSSUnit_Integer (0, -0, or 1 only)
>         eIntRatio,   // values are eCSSUnit_Array of two eCSSUnit_Integer
>         eResolution, // values are in eCSSUnit_Inch (for dpi) or
>                      //   eCSSUnit_Centimeter (for dpcm)
>-        eEnumerated  // values are eCSSUnit_Enumerated (uses keyword table)
>+        eEnumerated,  // values are eCSSUnit_Enumerated (uses keyword table)
>+		eString     // anaaktge currently meant for ms styles & color themes 

tab x2 and comment is too specific - match the style above.

>-OS_LIBS += $(call EXPAND_LIBNAME,shell32 ole32 uuid version winspool comdlg32 imm32 winmm wsock32 msimg32 shlwapi psapi)
>+OS_LIBS += $(call EXPAND_LIBNAME,shell32 ole32 uuid version winspool comdlg32 imm32 winmm wsock32 msimg32 shlwapi psapi uxtheme)

We don't need to link uxtheme as far as I know. This will cause Firefox not to load on Win2k and it's unnecessary since we dynamically load all the other functions too.

>+  typedef enum {
>+    eMetricString_WindowsColorTheme
>+  } nsMetricStringID;
>+

Not sure I like the naming here. Can we make it a generic theme name and only implement it for Windows? That or drop "Color" as that's implicit from "Theme". eMetricString_ThemeName ?

>+#include "Uxtheme.h"

This won't work on WINCE (not that we care greatly anymore) but it'd be good to stick it in the #ifndef WINCE below. Also, what do you need from here?

>+NS_IMETHODIMP nsLookAndFeel::GetMetric(const nsMetricStringID aID, nsString & aMetric)
>+{
>+	// anaaktge all teh dirty plumbing, todo later reuse from jim's code?
>+	// anaaktge missing high contrast theme names, awaiting testing on vista box 
>+	nsresult res = NS_ERROR_FAILURE;
>+	HRESULT colorResult;
>+    WCHAR colorSchemeName[MAX_PATH+1] = {L'\0'};
>+	WCHAR themeName[MAX_PATH+1] = {L'\0'};
>+
>+	// anaaktge and i did mean dirty!
>+	WCHAR lunaPath[] = L"C:\\WINDOWS\\resources\\Themes\\Luna\\luna.msstyles";
>+	WCHAR royalePath[] = L"C:\\WINDOWS\\resources\\Themes\\Royale\\Royale.msstyles";
>+	WCHAR aeroPath[] = L"C:\\WINDOWS\\resources\\Themes\\Aero\\aero.msstyles";
>+	WCHAR zunePath[] = L"C:\\WINDOWS\\resources\\Themes\\Zune\\Zune.msstyles";

Yeah...this is too hacky. Jim's stuff is better but the underlying API is really to blame for the mess.

>+
>+	WCHAR lunaSilver[] = L"Metallic";
>+	WCHAR lunaOlive[] = L"HomeStead";
>+	WCHAR lunaBlue[] = L"NormalColor";
>+
>+	switch (aID)
>+	{
>+	case eMetricString_WindowsColorTheme :
>+		colorResult = nsUXThemeData::GetCurrentThemeName(
>+					(LPWSTR)&themeName, MAX_PATH, 
>+					(LPWSTR)&colorSchemeName, MAX_PATH, 
>+					NULL, 0);
>+		// classic aka 'no theme' 
>+		if( (colorResult != S_OK) && !nsUXThemeData::IsAppThemed() )
>+		{
>+			// anaaktge todo colors for classic go in here
>+			aMetric = L"windows-classic";
>+			res = NS_OK;
>+		}
>+		else if (!wcscmp(themeName,lunaPath))
>+		{
>+			if(!wcscmp(colorSchemeName,lunaBlue))
>+			{
>+				aMetric = L"windows-luna-blue";
>+				res = NS_OK;
>+			}
>+			else if(!wcscmp(colorSchemeName, lunaSilver))
>+			{
>+				aMetric = L"windows-luna-silver";
>+				res = NS_OK;
>+			}
>+			else if(!wcscmp(colorSchemeName, lunaOlive))
>+			{
>+				aMetric = L"windows-luna-olive";
>+				res = NS_OK;
>+			}
>+			else {}
>+		}
>+		else if(!wcscmp(themeName, aeroPath))
>+		{
>+			aMetric = L"windows-aero";
>+			res = NS_OK;
>+		}
>+		else if(!wcscmp(themeName, royalePath))
>+		{
>+			aMetric = L"windows-royale";
>+			res = NS_OK;
>+		}
>+		else if(!wcscmp(themeName, zunePath))
>+		{
>+			aMetric = L"windows-zune";
>+			res = NS_OK;
>+		}
>+		else 
>+		{}
>+		break;
>+	default:
>+		break;
>+	}
>+	return res;
>+}

This code is tabtastic. I'd suggest pulling in Jim's code and creating a lookup table that maps his enum to the string constants here. Should be much cleaner.

>+  static inline HRESULT GetCurrentThemeName(LPWSTR pszThemeFileName, int dwMaxNameChars,
>+		LPWSTR pszColorBuff, int cchMaxColorChars, LPWSTR pszSizeBuff,
>+		int cchMaxSizeChars) {
>+	if(!getCurrentThemeName)
>+		return E_FAIL;
>+    return getCurrentThemeName(pszThemeFileName, dwMaxNameChars,
>+		pszColorBuff, cchMaxColorChars, pszSizeBuff, cchMaxSizeChars);

nit: indentation

Also, you need to provide a stub implementation of your new nsILookAndFeel::GetMetric overload so that other platforms can compile. It can just be the constant function NS_ERROR_FAILURE and you can stash that in nsILookAndFeel.h so you won't have to touch all the backends.

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #48)
> Also, you need to provide a stub implementation of your new
> nsILookAndFeel::GetMetric overload so that other platforms can compile. It can
> just be the constant function NS_ERROR_FAILURE and you can stash that in
> nsILookAndFeel.h so you won't have to touch all the backends.

Wouldn't it be better to add it to nsXPLookAndFeel.h instead?

Comment 50

[Shawn Wilsher :sdwilsh]

Comment on attachment 460441 [details] [diff] [review]
round, hopefully with legible formatting

Will provide feedback after robarnold's comments have been addressed.

Comment 51

[Alex Faaborg [:faaborg] (Firefox UX)]

Ideally we could get this for beta5 so that we could begin using it with our theme work.

Comment 52

[Alex Faaborg [:faaborg] (Firefox UX)]

Doesn't block the initial work, but related to the visual styling being done in bug 583386 (Firefox menu)

Comment 53

[Alex Faaborg [:faaborg] (Firefox UX)]

Roc: I just want to clarify that it is ok for this to only block final, and doesn't have to make beta 6 in order to land?

Comment 54

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

Yes

Comment 55

[Alex Faaborg [:faaborg] (Firefox UX)]

While this bug doesn't block feature freeze, we really need the capabilities this bug provides in order to do polish work on our XP theme prior to RC.  There isn't really a flag to describe what we need, but ideally we would get access to this functionality the day after feature freeze.

Comment 56

[Allison Naaktgeboren :ally]

Unfortunately it is unlikely that I will have time or be able to keep up with the recent changes to write a new patch. Someone else should do this.

Comment 57

[Jim Mathies [:jimm]]

taking..

Comment 58

[Alex Faaborg [:faaborg] (Firefox UX)]

>taking..

that's great, as soon as this bug is resolved we can begin work on landing the Windows XP theme changes.

Comment 59

[Jim Mathies [:jimm]]

(In reply to comment #11)
> We need to differentiate between the following 9 cases:
> 
> classic
> aero (on 7)
> aero (on vista)
> royale
> luna blue
> luna silver
> luna olive
> high contrast black (all three types)
> high contrast white
> 
> current we can only differentiate on 2 cases:
> 
> classic
> not classic

Note, bug 425598 seems to cover detecting high contrast themes.

Comment 60

[Csaba Kozák [:WonderCsabo]]

What about aero basic?

Comment 61

[Jim Mathies [:jimm]]

So, I've been digging into the history of all this to try and understand how we got here. We originally implemented 'windows-default-theme' in bug 426660. (dbaron had a nice critique of that work in bug 426660 comment 25, I think he made some good points on the shortcomings which have since been shown to be true.) As a follow up to address a shortcoming of 'windows-default-theme' we added 'windows-classic' in bug 431666.  We now rely pretty heavily on 'windows-default-theme', but it would seem we use that more as a switch for themed vs. classic than anything else. We spun off bug 425598 from there on detecting high contrast themes. That bug is still open and there's a suggestion that it be resolved won't fix. (Although I'm not sure why, that actually seems useful as some of the comments in the bug suggest.) We also added 'moz-windows-compositor' so we could detect glass.

We discussed here the differences between being able to detect the actual theme by name, and exposing theme colors we currently do not have access to. I do see a few small advantages in exposing the name. The Zune theme for example apparently doesn't expose the color of its progress meter directly through VSCLASS_PROGRESS theme colors. However that's a small, corner case. The zip I posted here has common theme colors in it, and I think access to a few of those may provide 99% of what we need for theming with the right colors. (VSCLASS_WINDOW may hold all the colors we need, the UX folks can confirm.) If we implement a media query for the theme name only, then I assume we'll be hard coding all sorts of color information in css. That seems wrong, those should probably be pulled from the system so there's less maintenance. So I'm wondering if going with theme colors is the better, long term approach. 

We can however do the name thing if we really feel it gives us some advantage. In the zip I posted I've printed the both the theme name ('luna') and a scheme name (luna: 'normalcolor', 'metallic', 'homestead' | royale: 'normalcolor' | zune: 'normalcolor'). We have enough information there to provide this. 

What I need to know is: based on the theme work we still want to do, will the colors in the zip offer what we need? If the answer to that is yes, then we should go that route. If however there is something we simply can't do without knowing what theme/scheme is in use, then we can go with the theme name.

(Also, note, we have an open, non-blocking bug for glass colorization color (bug 578780), so I won't be covering that here.)

Comment 62

[Jim Mathies [:jimm]]

(In reply to comment #60)
> What about aero basic?

moz-windows-compositor accurately detects glass. The underlying theme can be supported using color extraction or possibly the theme name approach.

Comment 63

[Alex Faaborg [:faaborg] (Firefox UX)]

>What I need to know is: based on the theme work we still want to do, will the
>colors in the zip offer what we need? If the answer to that is yes, then we
>should go that route. If however there is something we simply can't do without
>knowing what theme/scheme is in use, then we can go with the theme name.

We would like to create the windows XP themes using a lot of hard coded colors and images.  Limiting ourselves to only using extracted colors makes it impossible for us to do things like implementing the new title bar / tab strip background, which is in the same general style of each XP theme, but uses slightly different colors, lighting and textures.

All of the visual design work has already been completed by Stephen, but without the ability to select on theme name, we won't be able to use any of it.

Comment 64

[Alex Faaborg [:faaborg] (Firefox UX)]

Example of what I mean, this blue title bar and tab strip background matches the general feel of XP Lune Blue, but is currently a set of hard coded colors, and not based on extracted values: https://wiki.mozilla.org/images/2/2e/Firefox-4-Mockup-i06-%28XP%29-%28LunaBlue%29-%28TabsTop%29-%28MenuBar%29-%28BookmarksBar%29.png

Comment 65

[Jim Mathies [:jimm]]

Alright, name/scheme it is then.

Comment 66

[Jim Mathies [:jimm]]

alright so this is what I'm proposing as recognized themes, please let me know if this will meet our needs:

detected themes:

@media all and (moz-win-theme: xyz)

luna-default
luna-metallic
luna-homestead
royale-default
zune-default
aero-default

other queries we already have:

moz-windows-classic - themed vs. classic mode
moz-windows-compositor - glass enabled

Comment 67

[Alex Faaborg [:faaborg] (Firefox UX)]

Looks good, is luna-homestead the internal api name for what is surfaced in the UI as "Olive"?

Comment 68

[Jim Mathies [:jimm]]

(In reply to comment #67)
> Looks good, is luna-homestead the internal api name for what is surfaced in the
> UI as "Olive"?

http://en.wikipedia.org/wiki/Luna_(theme)

Yes. I can name them with the more common names to if need be.

Comment 69

[Alex Faaborg [:faaborg] (Firefox UX)]

>I can name them with the more common names to if need be.

Yeah, we should have the css selectors named based on the names windows shows in the appearance UI, so that any developers using them won't have to check documentation.

Comment 70

[Jim Mathies [:jimm]]

Attachment: layout patch v.1

Initial workup. dbaron, curious what you think. I really wanted to use the cached metrics values in nsCSSRulesProcessor. This seemed to be the best way to accomplish that.

Comment 71

[Jim Mathies [:jimm]]

Attachment: widget patch v.1

Comment 72

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

Comment on attachment 481070 [details] [diff] [review]
layout patch v.1

>+// windows theme selector metrics
>+GK_ATOM(windows_theme_aero, "-windows-theme-aero")
>+GK_ATOM(windows_theme_luna_blue, "-windows-theme-luna-blue")
>+GK_ATOM(windows_theme_luna_olive, "-windows-theme-luna-olive")
>+GK_ATOM(windows_theme_luna_silver, "-windows-theme-luna-silver")
>+GK_ATOM(windows_theme_royale, "-windows-theme-royale")
>+GK_ATOM(windows_theme_zune, "-windows-theme-zune")
>+GK_ATOM(windows_theme_generic, "-windows-theme-generic")

These shouldn't have a dash at the beginning.

>diff --git a/layout/style/nsCSSRuleProcessor.cpp b/layout/style/nsCSSRuleProcessor.cpp
>--- a/layout/style/nsCSSRuleProcessor.cpp
>+++ b/layout/style/nsCSSRuleProcessor.cpp
>@@ -1030,16 +1030,40 @@ InitSystemMetrics()
>     sSystemMetrics->AppendElement(nsGkAtoms::touch_enabled);
>   }
>  
>   rv = lookAndFeel->GetMetric(nsILookAndFeel::eMetric_MaemoClassic, metricResult);
>   if (NS_SUCCEEDED(rv) && metricResult) {
>     sSystemMetrics->AppendElement(nsGkAtoms::maemo_classic);
>   }
> 
>+#ifdef XP_WIN
>+  if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowsThemeAero, metricResult))
>+      && metricResult)
>+    sSystemMetrics->AppendElement(nsGkAtoms::windows_theme_aero);
>+  else if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowsThemeLunaBlue, metricResult))
>+           && metricResult)
>+    sSystemMetrics->AppendElement(nsGkAtoms::windows_theme_luna_blue);
>+  else if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowsThemeLunaOlive, metricResult))
>+           && metricResult)
>+    sSystemMetrics->AppendElement(nsGkAtoms::windows_theme_luna_olive);
>+  else if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowsThemeLunaSilver, metricResult))
>+           && metricResult)
>+    sSystemMetrics->AppendElement(nsGkAtoms::windows_theme_luna_silver);
>+  else if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowsThemeRoyale, metricResult))
>+           && metricResult)
>+    sSystemMetrics->AppendElement(nsGkAtoms::windows_theme_royale);
>+  else if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowsThemeZune, metricResult))
>+           && metricResult)
>+    sSystemMetrics->AppendElement(nsGkAtoms::windows_theme_zune);
>+  else if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowsThemeGeneric, metricResult))
>+           && metricResult)
>+    sSystemMetrics->AppendElement(nsGkAtoms::windows_theme_generic);
>+#endif

Wouldn't this work better if you had a single eMetric_ constant with seven enumerated values?

>+        cmp = !(actual == required); // string comparison
>+		  }
>+		break;

weird indent here (maybe tabs?)

>+static nsresult
>+GetIdentifier(nsPresContext* aPresContext, const nsMediaFeature* aFeature,
>+              nsCSSValue& aResult)
>+{
>+    if (aFeature->mName == &nsGkAtoms::_moz_windows_theme) {
>+      if (nsCSSRuleProcessor::HasSystemMetric(nsGkAtoms::windows_theme_aero))
>+        aResult.SetStringValue(NS_LITERAL_STRING("aero"), eCSSUnit_Ident);
>+      else if (nsCSSRuleProcessor::HasSystemMetric(nsGkAtoms::windows_theme_luna_blue))
>+        aResult.SetStringValue(NS_LITERAL_STRING("luna-blue"), eCSSUnit_Ident);
>+      else if (nsCSSRuleProcessor::HasSystemMetric(nsGkAtoms::windows_theme_luna_olive))
>+        aResult.SetStringValue(NS_LITERAL_STRING("luna-olive"), eCSSUnit_Ident);
>+      else if (nsCSSRuleProcessor::HasSystemMetric(nsGkAtoms::windows_theme_luna_silver))
>+        aResult.SetStringValue(NS_LITERAL_STRING("luna-silver"), eCSSUnit_Ident);
>+      else if (nsCSSRuleProcessor::HasSystemMetric(nsGkAtoms::windows_theme_royale))
>+        aResult.SetStringValue(NS_LITERAL_STRING("royale"), eCSSUnit_Ident);
>+      else if (nsCSSRuleProcessor::HasSystemMetric(nsGkAtoms::windows_theme_zune))
>+        aResult.SetStringValue(NS_LITERAL_STRING("zune"), eCSSUnit_Ident);
>+      else if (nsCSSRuleProcessor::HasSystemMetric(nsGkAtoms::windows_theme_generic))
>+        aResult.SetStringValue(NS_LITERAL_STRING("generic"), eCSSUnit_Ident);
>+    }
>+    return NS_OK; // aResult is already set to eCSSUnit_Null
>+}

Maybe it would make more sense to cache the string separately in addition to in sSystemMetrics (which would still be used for :-moz-system-metric())?  It seems like that would add up to less code.  If not, at least use an array here and iterate through it.

>+        eIdent       // values are a string identifier (eCSSUnit_Ident)

s/string identifier/identifier/ (since in CSS identifiers and strings are different concepts)


Otherwise looks fine, though.

Comment 73

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

(In reply to comment #72)
> Maybe it would make more sense to cache the string separately in addition to in
> sSystemMetrics (which would still be used for :-moz-system-metric())?  It seems
> like that would add up to less code.  If not, at least use an array here and
> iterate through it.

Actually, it's probably easier to cache an enum than a string (if you follow my earlier suggestion about a single metric).

Comment 74

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

Also, probably better to:
 * make GetIdentifier explicitly call aResult.Reset() in the cases where there is no value
 * rename GetIdentifier to GetWindowsTheme and don't check aFeature->mName
 * follow 4-space indent that the file uses

Comment 75

[Jim Mathies [:jimm]]

(In reply to comment #73)
> (In reply to comment #72)
> > Maybe it would make more sense to cache the string separately in addition to in
> > sSystemMetrics (which would still be used for :-moz-system-metric())?  It seems
> > like that would add up to less code.  If not, at least use an array here and
> > iterate through it.
> 
> Actually, it's probably easier to cache an enum than a string (if you follow my
> earlier suggestion about a single metric).

We still need the individual theme atoms in sSystemMetrics for :-moz-system-metric() correct?

Comment 76

[Jim Mathies [:jimm]]

(In reply to comment #75)
> (In reply to comment #73)
> > (In reply to comment #72)
> > > Maybe it would make more sense to cache the string separately in addition to in
> > > sSystemMetrics (which would still be used for :-moz-system-metric())?  It seems
> > > like that would add up to less code.  If not, at least use an array here and
> > > iterate through it.
> > 
> > Actually, it's probably easier to cache an enum than a string (if you follow my
> > earlier suggestion about a single metric).
> 
> We still need the individual theme atoms in sSystemMetrics for
> :-moz-system-metric() correct?

(I think that's what you meant by "which would still be used for", but wanted to confirm.)

As for caching the enum, an #ifdef'd static value in nsCSSRuleProcessor maybe? Or something more generic that could be reused, like another array for cached PRInt32's? I'm not sure how serious I should be about avoiding platform feature specific code here.

Comment 77

[Jim Mathies [:jimm]]

Attachment: widget patch v.2

use a single enumerated metric value.

Comment 78

[Jim Mathies [:jimm]]

Attachment: widget patch v.3

added eWindowsTheme_Classic, which can replace uses of eMetric_WindowsClassic.

Comment 79

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

(In reply to comment #75)
> We still need the individual theme atoms in sSystemMetrics for
> :-moz-system-metric() correct?

Yes.

(In reply to comment #76)
> As for caching the enum, an #ifdef'd static value in nsCSSRuleProcessor maybe?

That's fine.

Comment 80

[Jim Mathies [:jimm]]

Attachment: layout patch v.2

Comment 81

[Jim Mathies [:jimm]]

Attachment: layout patch v.2

minor nit fixed

Comment 82

[Jim Mathies [:jimm]]

Attachment: tests v.1

some tests too. unfortunately there's a lot of this I can't test since it's tied to the desktop theme. I'm going to do some heavy local testing to make sure everything is working.

Comment 83

[Jim Mathies [:jimm]]

Comment on attachment 481232 [details] [diff] [review]
widget patch v.3

roc, do you have the time for reviewing these widget changes?

Comment 84

[[e-mail]]

Comment on attachment 481232 [details] [diff] [review]
widget patch v.3

>-// {3fd2930f-1040-4d08-b638-0b3f134e6b6f}
>+// {BE476931-6D4F-4eee-B6FB-3770D94A8AE2}
> #define NS_ILOOKANDFEEL_IID \
>-{ 0xc23ca876, 0x6ecf, 0x49c6, \
>-    { 0xb2, 0xb4, 0x5b, 0xe5, 0x16, 0xb5, 0x0e, 0x28 } }
>+{ 0xbe476931, 0x6d4f, 0x4eee, { 0xb6, 0xfb, 0x37, 0x70, 0xd9, 0x4a, 0x8a, 0xe2 } }
I didn't think that IID changes were necessary for constant additions.

>+  enum WindowsThemeIdentifier {
>+    eWindowsTheme_Aero = 1,
>+    eWindowsTheme_LunaBlue,
>+    eWindowsTheme_LunaOlive,
>+    eWindowsTheme_LunaSilver,
>+    eWindowsTheme_Royale,
>+    eWindowsTheme_Zune,
>+    eWindowsTheme_Classic,
>+    eWindowsTheme_Generic // unrecognized theme
>+  };
Not sure why you used a different order here to nsUXThemeData.h

>+        switch(nsUXThemeData::GetNativeThemeId()) {
>+            case WINTHEME_CLASSIC:
>+              aMetric = eWindowsTheme_Classic;
>+              break;
>+            case WINTHEME_AERO:
>+              aMetric = eWindowsTheme_Aero;
>+              break;
>+            case WINTHEME_LUNA:
>+              switch(nsUXThemeData::GetNativeThemeColor()) {
Why not cache the metric directly, instead of recomputing it each time?
Nit: switch isn't a function, add a space before the opening (

>+struct THEMELIST {
>+  const LPCWSTR name;
>+  int type;
>+};
I have a vague memory of some compilers complaining about initialising const members of types with a POD-style initialiser. Seeing as the actual variables are const anyway, I don't see how an extra const here helps.

>+  WCHAR themeFileName[MAX_PATH + 1] = {L'\0'};
>+  WCHAR themeColor[MAX_PATH + 1] = {L'\0'};
I don't think you need to initialise these.

>+  PRBool bFound = PR_FALSE;
>+  for (int i = 0; i < NS_ARRAY_LENGTH(knownColors); ++i) {
>+    if (!lstrcmpiW(themeColor, knownColors[i].name)) {
>+      sThemeColor = (WindowsThemeColor)knownThemes[i].type;
>+      bFound = PR_TRUE;
>+      break;
>+    }
>+  }
>+
>+  // If we didn't detect the color scheme, reset to generic.
>+  if (!bFound) {
>+    sThemeId = WINTHEME_UNRECOGNIZED;
>+    sThemeColor = WINTHEMECOLOR_NORMAL;
>+  }
I'm not sure what this is trying to achieve. If your theme is one of the non-Luna known themes, then it will still accept a homestead or metallic colour, which will then get ignored.

Comment 85

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

Comment on attachment 481232 [details] [diff] [review]
widget patch v.3

but see Neil's comments

Comment 86

[Jim Mathies [:jimm]]

(In reply to comment #84)
> Comment on attachment 481232 [details] [diff] [review]
> widget patch v.3
> 
> >+  PRBool bFound = PR_FALSE;
> >+  for (int i = 0; i < NS_ARRAY_LENGTH(knownColors); ++i) {
> >+    if (!lstrcmpiW(themeColor, knownColors[i].name)) {
> >+      sThemeColor = (WindowsThemeColor)knownThemes[i].type;
> >+      bFound = PR_TRUE;
> >+      break;
> >+    }
> >+  }
> >+
> >+  // If we didn't detect the color scheme, reset to generic.
> >+  if (!bFound) {
> >+    sThemeId = WINTHEME_UNRECOGNIZED;
> >+    sThemeColor = WINTHEMECOLOR_NORMAL;
> >+  }
> I'm not sure what this is trying to achieve. If your theme is one of the
> non-Luna known themes, then it will still accept a homestead or metallic
> colour, which will then get ignored.

That's ok since metallic and homestead are specific to luna. I suppose if one of those themes had one of these colors then things could get confused. But they don't.

I'll update the rest and re-post.

Comment 87

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

Comment on attachment 481332 [details] [diff] [review]
layout patch v.2

>diff --git a/layout/style/nsMediaFeatures.cpp b/layout/style/nsMediaFeatures.cpp
>+#ifdef XP_WIN
>+struct WINTHEMELIST {

Could you call this WindowsThemeName instead of WINTHEMELIST?

>+    nsILookAndFeel::WindowsThemeIdentifier id;
>+    const wchar_t* name;
>+};
>+
>+// Windows theme identities used in the -moz-windows-theme media query.
>+const WINTHEMELIST themeStrings[] = {
>+    { nsILookAndFeel::eWindowsTheme_Aero,       L"aero" },
>+    { nsILookAndFeel::eWindowsTheme_LunaBlue,   L"luna-blue" },
>+    { nsILookAndFeel::eWindowsTheme_LunaOlive,  L"luna-olive" },
>+    { nsILookAndFeel::eWindowsTheme_LunaSilver, L"luna-silver" },
>+    { nsILookAndFeel::eWindowsTheme_Royale,     L"royale" },
>+    { nsILookAndFeel::eWindowsTheme_Zune,       L"zune" },
>+    { nsILookAndFeel::eWindowsTheme_Generic,    L"generic" }
>+};
>+#endif


>+#ifdef XP_WIN
>+static nsresult
>+GetWindowsTheme(nsPresContext* aPresContext, const nsMediaFeature* aFeature,
>+                nsCSSValue& aResult)
>+{
>+    aResult.Reset();
>+
>+    // Classic mode should fail to match.
>+    if (nsCSSRuleProcessor::GetWindowsThemeIdentifier() ==
>+        nsILookAndFeel::eWindowsTheme_Classic)
>+        return NS_OK;
>+
>+    for (int idx = 0; idx < NS_ARRAY_LENGTH(themeStrings); ++idx) {
>+        if (nsCSSRuleProcessor::GetWindowsThemeIdentifier() ==
>+            themeStrings[idx].id) {
>+            aResult.SetStringValue(nsDependentString(themeStrings[idx].name),
>+                                   eCSSUnit_Ident);
>+            break;
>+        }
>+    }
>+    return NS_OK;
>+}
>+#endif

Instead of making the entire function ifdef XP_WIN, you should put the ifdefs inside the function, so that when not XP_WIN it just looks like:
    aResult.Reset();
    return NS_OK;


Also, please cache the result of nsCSSRuleProcessor::GetWindowsThemeIdentifier() rather than calling it each time.

Please rename idx to i (for local style) and make it a size_t.

>+#ifdef XP_WIN
>+    {
>+        &nsGkAtoms::_moz_windows_theme,
>+        nsMediaFeature::eMinMaxNotAllowed,
>+        nsMediaFeature::eIdent,
>+        { nsnull },
>+        GetWindowsTheme
>+    },
>+#endif

And make this not-ifdefed, so we always support the feature, but it's false.

>diff --git a/layout/style/nsMediaFeatures.h b/layout/style/nsMediaFeatures.h
>+        eIdent       // values are in eCSSUnit_Ident

remove "in "

r=dbaron with that

Comment 88

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

Comment on attachment 481334 [details] [diff] [review]
tests v.1

>+  expression_should_be_parseable("-moz-windows-theme: aero");
>+  expression_should_be_parseable("-moz-windows-theme: luna-blue");
>+  expression_should_be_parseable("-moz-windows-theme: luna-olive");
>+  expression_should_be_parseable("-moz-windows-theme: luna-silver");
>+  expression_should_be_parseable("-moz-windows-theme: royale");
>+  expression_should_be_parseable("-moz-windows-theme: generic");
>+  expression_should_be_parseable("-moz-windows-theme: zune");
>+  expression_should_not_be_parseable("-moz-windows-theme: ");

You might also want to test that -moz-windows-theme: '' is not parseable, and that -moz-windows-theme: garbage is parseable.


Please don't add a separate file, though.  Instead, add to the existing file (which basically means reducing the diff to just the addition of the lines quoted above).  With the changes I suggested to the previous patch, there shouldn't even be any need to run the tests conditionally.

r=dbaron with that

Comment 89

[Jim Mathies [:jimm]]

Attachment: rollup patch

rollup patch with updates.

Comment 90

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/28565b318863
http://hg.mozilla.org/mozilla-central/rev/dac24a7544ba
http://hg.mozilla.org/mozilla-central/rev/904a556a15f2

Comment 91

[Eric Shepherd [:sheppy]]

Documented here:

https://developer.mozilla.org/En/CSS/Media_queries#-moz-windows-theme

Linked to from:

https://developer.mozilla.org/en/Theme_changes_in_Firefox_4