Bug 23440 – RenderThemeWin cleanup (selection colors and focus rings)

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dave Hyatt]

Clean up RenderThemeWin.

[Dave Hyatt]

Created attachment 26872 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 26872 [details]
Patch

> +Color RenderTheme::activeSelectionForegroundColor() const
> +{
> +    if (supportsForegroundSelectionColors() && !m_activeSelectionForegroundColor.isValid())

It would be slightly more efficient to reverse the two sides of this && (ditto elsewhere).

> -Color RenderTheme::platformInactiveSelectionBackgroundColor() const
> +Color RenderTheme::platformActiveSelectionForegroundColor() const
>  {
> -    // Use a grey color by default if the platform theme doesn't define anything.
> -    return Color(128, 128, 128);
> +    // Use a white color by default if the platform theme doesn't define anything.
> +    return Color(255, 255, 255);
>  }
>  
> -Color RenderTheme::platformActiveSelectionForegroundColor() const
> +Color RenderTheme::platformInactiveSelectionBackgroundColor() const
>  {
> -    return Color();
> +    // Use a grey color by default if the platform theme doesn't define anything.
> +    return Color(176, 176, 176);

Why the change from 128 to 176?

>  Color RenderTheme::platformInactiveSelectionForegroundColor() const
>  {
> -    return Color();
> +	// Use a black color by default.
> +    return Color(0, 0, 0);
>  }

I think it would be better to use Color::white, Color::black, and Color::gray instead of hand-coding the values in this file.

>  Color RenderThemeWin::platformInactiveSelectionBackgroundColor() const
>  {
> -    COLORREF color = GetSysColor(COLOR_GRAYTEXT);
> -    return Color(GetRValue(color), GetGValue(color), GetBValue(color), 255);
> +    return Color(176, 176, 176, 255);
>  }

Again, I think Color::gray would be better. I don't think the fourth parameter is required.

r=me

[David Levin]

Committed as r40074.

Resolving since it was committed (to move this out of the commit queue).