Bug 20246 - Implement RenderTheme::systemColor() for Windows
Summary: Implement RenderTheme::systemColor() for Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Eric Roman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-31 15:41 PDT by Eric Roman
Modified: 2008-10-12 16:59 PDT (History)
2 users (show)

See Also:


Attachments
Call through to GetSysColor() for RenderThemeSafari::systemColor() (3.74 KB, patch)
2008-07-31 15:44 PDT, Eric Roman
hyatt: review-
Details | Formatted Diff | Diff
I see. Moved the change from RenderThemeSafari to RenderThemeWin. (3.68 KB, patch)
2008-08-01 14:24 PDT, Eric Roman
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Roman 2008-07-31 15:41:17 PDT
CSS system colors (http://msdn.microsoft.com/en-us/library/aa358804.aspx) are hard coded constants on Windows Safari. Instead these should query windows.
Comment 1 Eric Roman 2008-07-31 15:44:43 PDT
Created attachment 22590 [details]
Call through to GetSysColor() for RenderThemeSafari::systemColor()

-I think I need to do a parallel change to RenderThemeWin.cpp, but not sure.
-Not sure what sort of test to write for this, since the expected value will depend on the theme.
Comment 2 Eric Seidel (no email) 2008-08-01 12:23:48 PDT
Comment on attachment 22590 [details]
Call through to GetSysColor() for RenderThemeSafari::systemColor()

Looks OK.  The switch style is probably wrong by WebKit standards.  I'd almost consider using a HashMap here, except that'd be a bunch of memory on the heap for no real need.

I wonder if we need any kind of caching here?  Color does some amount of caching, but I don't think that applies here.

Adam Roben should comment.
Comment 3 Dave Hyatt 2008-08-01 12:25:59 PDT
Comment on attachment 22590 [details]
Call through to GetSysColor() for RenderThemeSafari::systemColor()

The point of RenderThemeSafari is to match the look of OS X and not the look of Windows.
Comment 4 Dave Hyatt 2008-08-01 12:26:53 PDT
RenderThemeWin (if someone used it) would be more suitable for this patch, since that theme is about matching the native Windows look.

Comment 5 Eric Roman 2008-08-01 14:24:57 PDT
Created attachment 22611 [details]
I see. Moved the change from RenderThemeSafari to RenderThemeWin.
Comment 6 Dave Hyatt 2008-08-01 15:01:33 PDT
Comment on attachment 22611 [details]
I see. Moved the change from RenderThemeSafari to RenderThemeWin.

r=me
Comment 7 Darin Adler 2008-10-12 16:59:26 PDT
http://trac.webkit.org/changeset/37537