Bug 23440

Summary: RenderThemeWin cleanup (selection colors and focus rings)
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: PlatformAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch aroben: review+

Description Dave Hyatt 2009-01-20 14:55:47 PST
Clean up RenderThemeWin.
Comment 1 Dave Hyatt 2009-01-20 14:56:25 PST
Created attachment 26872 [details]
Patch
Comment 2 Adam Roben (:aroben) 2009-01-20 15:05:59 PST
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
Comment 3 David Levin 2009-03-24 12:44:28 PDT
Committed as r40074.

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