Summary: | [chromium] theme scrollbars to match gtk theme | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Martin <evan> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, fishd, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Other | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Attachments: |
|
Created attachment 44122 [details]
patch
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
Created attachment 44123 [details]
patch with fixed tabs and caps
Created attachment 44124 [details]
patch with fixed tabs and caps
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
Created attachment 44125 [details]
patch with fixed tabs and caps
Thanks for fixing the style issues. style-queue ran check-webkit-style on attachment 44125 [details] without any errors.
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!
Filed bug 32051 about check-webkit-style complaining about under_bar style variable names. Created attachment 44173 [details]
patch with fixed tabs and caps
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 on attachment 44173 [details]
patch with fixed tabs and caps
cq+ at Markus's request.
Comment on attachment 44173 [details] patch with fixed tabs and caps Clearing flags on attachment: 44173 Committed r51827: <http://trac.webkit.org/changeset/51827> All reviewed patches have been landed. Closing bug. |
Created attachment 44121 [details] Patch When using the system colors, we should make the scrollbars match.