[webkit-reviews] review granted: [Bug 23440] RenderThemeWin cleanup (selection colors and focus rings) : [Attachment 26872] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 23440: RenderThemeWin cleanup (selection colors and focus rings)
https://bugs.webkit.org/show_bug.cgi?id=23440

Attachment 26872: Patch
https://bugs.webkit.org/attachment.cgi?id=26872&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +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