Bug 32048 – [chromium] theme scrollbars to match gtk theme

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Evan Martin]

Created attachment 44121 [details]
Patch

When using the system colors, we should make the scrollbars match.

[Evan Martin]

Created attachment 44122 [details]
patch

[WebKit Review Bot]

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

[Evan Martin]

Created attachment 44123 [details]
patch with fixed tabs and caps

[Evan Martin]

Created attachment 44124 [details]
patch with fixed tabs and caps

[WebKit Review Bot]

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

[Evan Martin]

Created attachment 44125 [details]
patch with fixed tabs and caps

[Adam Barth]

Thanks for fixing the style issues.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44125 [details] without any errors.

[Eric Seidel (no email)]

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!

[Eric Seidel (no email)]

Filed bug 32051 about check-webkit-style complaining about under_bar style variable names.

[Evan Martin]

Created attachment 44173 [details]
patch with fixed tabs and caps

[Eric Seidel (no email)]

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"

[Adam Langley]

Comment on attachment 44173 [details]
patch with fixed tabs and caps

cq+ at Markus's request.

[WebKit Commit Bot]

Comment on attachment 44173 [details]
patch with fixed tabs and caps

Clearing flags on attachment: 44173

Committed r51827: <http://trac.webkit.org/changeset/51827>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.