Bug 32048 - [chromium] theme scrollbars to match gtk theme
Summary: [chromium] theme scrollbars to match gtk theme
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-01 18:30 PST by Evan Martin
Modified: 2009-12-07 18:55 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.34 KB, patch)
2009-12-01 18:30 PST, Evan Martin
no flags Details | Formatted Diff | Diff
patch (11.88 KB, patch)
2009-12-01 18:36 PST, Evan Martin
no flags Details | Formatted Diff | Diff
patch with fixed tabs and caps (11.99 KB, patch)
2009-12-01 18:39 PST, Evan Martin
no flags Details | Formatted Diff | Diff
patch with fixed tabs and caps (11.98 KB, patch)
2009-12-01 18:41 PST, Evan Martin
no flags Details | Formatted Diff | Diff
patch with fixed tabs and caps (11.98 KB, patch)
2009-12-01 18:46 PST, Evan Martin
no flags Details | Formatted Diff | Diff
patch with fixed tabs and caps (11.75 KB, patch)
2009-12-02 12:30 PST, Evan Martin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Martin 2009-12-01 18:30:43 PST
Created attachment 44121 [details]
Patch

When using the system colors, we should make the scrollbars match.
Comment 1 Evan Martin 2009-12-01 18:36:26 PST
Created attachment 44122 [details]
patch
Comment 2 WebKit Review Bot 2009-12-01 18:39:26 PST
Attachment 44122 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 5120 characters of output:
itespace/braces] [4]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:108:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:112:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:114:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:115:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:116:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:121:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:122:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:123:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:124:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:125:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:127:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:130:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:130:  Missing spaces around /  [whitespace/operators] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:131:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:136:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:139:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 33
Comment 3 Evan Martin 2009-12-01 18:39:48 PST
Created attachment 44123 [details]
patch with fixed tabs and caps
Comment 4 Evan Martin 2009-12-01 18:41:11 PST
Created attachment 44124 [details]
patch with fixed tabs and caps
Comment 5 WebKit Review Bot 2009-12-01 18:44:50 PST
Attachment 44124 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:100:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:134:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 2
Comment 6 Evan Martin 2009-12-01 18:46:42 PST
Created attachment 44125 [details]
patch with fixed tabs and caps
Comment 7 Adam Barth 2009-12-01 18:47:45 PST
Thanks for fixing the style issues.
Comment 8 WebKit Review Bot 2009-12-01 18:50:10 PST
style-queue ran check-webkit-style on attachment 44125 [details] without any errors.
Comment 9 Eric Seidel (no email) 2009-12-01 20:29:24 PST
Comment on attachment 44125 [details]
patch with fixed tabs and caps

Would be nice to document the pixel format.  "unsigned" doesn't tell the reader much.  I'm kinda surprised we don't have a WebColor with all the other Web* we've got. :)

I'm surprised we don't already ahve something like saturateAndBrighten in Color.h

I would have written:
 86     if (color[1] > 1.0)
 87         color[1] = 1.0;
 88     else if (color[1] < 0.0)
 89         color[1] = 0.0;

as:
color[1] = std::min(std::max(hsv[1] + saturateAmount, 0.0), 1.0)

Ha.  If you don't like my min/max above, at least write a function do do it, cause you repeat the same 4 lines everywehre:
if (min_diff < 0.2)
 131         min_diff = 0.2;
 132     else if (min_diff > 0.5)
 133         min_diff = 0.5;

Style viloations:
inactive_color

I'm surprised check-webkit-style doesn't get those.  I guess I'll file a bug.

Otherwise looks good!
Comment 10 Eric Seidel (no email) 2009-12-01 20:31:38 PST
Filed bug 32051 about check-webkit-style complaining about under_bar style variable names.
Comment 11 Evan Martin 2009-12-02 12:30:19 PST
Created attachment 44173 [details]
patch with fixed tabs and caps
Comment 12 Eric Seidel (no email) 2009-12-02 13:00:14 PST
Comment on attachment 44173 [details]
patch with fixed tabs and caps

WebKit doesn't get excited about wrapping:
 154     SkColorToHSV(RenderThemeChromiumLinux::thumbInactiveColor(),
 155                  thumb_hsv);

This looks fine though.

I'm slightly surprised you need both:
+#include "RenderTheme.h"
+#include "RenderThemeChromiumLinux.h"
Comment 13 Adam Langley 2009-12-07 18:36:15 PST
Comment on attachment 44173 [details]
patch with fixed tabs and caps

cq+ at Markus's request.
Comment 14 WebKit Commit Bot 2009-12-07 18:55:01 PST
Comment on attachment 44173 [details]
patch with fixed tabs and caps

Clearing flags on attachment: 44173

Committed r51827: <http://trac.webkit.org/changeset/51827>
Comment 15 WebKit Commit Bot 2009-12-07 18:55:07 PST
All reviewed patches have been landed.  Closing bug.