[webkit-reviews] review denied: [Bug 26030] [Chromium] Chromium Linux ignores background color on <select>. : [Attachment 30920] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Langley <agl at chromium.org>'s
request for review:
Bug 26030: [Chromium] Chromium Linux ignores background color on <select>.
https://bugs.webkit.org/show_bug.cgi?id=26030

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
I would have given this color a name:
 129	     return 0xffdddddd;

static Color someNiceNameForTheColor(0xffdddddd);
return someNiceNameForTheColor;

Braces get their own line:
 264 static float brightenColorCap(float in) {


Single line ifs don't get braces:
292	if (max == min) {
 293	     hue = 0.f;
 294	 } else if (max == r) {

Color functions belong on Color.h or somewhere in Skia...
 282 static SkColor brightenColor(SkColor base, float brightenAmount)

256341 static void paintButtonLike(RenderTheme* theme, RenderObject* o, const
RenderObject::PaintInfo& i, const IntRect& rect) {
 need { on its own line too.

WK of course uses CamelCase:
 334	 int final_r = 255.f * brightenColorDecode(p, q, tr);

Looks nice though! :)

r- for color stuff needing to move somewhere else.