[webkit-reviews] review granted: [Bug 6129] Incomplete implementation of CSS 2.1 system colors : [Attachment 17440] Proposed patch 2

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Andrew Wellington
<proton at wiretapped.net>'s request for review:
Bug 6129: Incomplete implementation of CSS 2.1 system colors
http://bugs.webkit.org/show_bug.cgi?id=6129

Attachment 17440: Proposed patch 2
http://bugs.webkit.org/attachment.cgi?id=17440&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
This seems really good.  A few nits though.

I think a comment explain *why* this can fail (and will fail) would be really
informative and helpful. 
+    NSColor* color = [theColor
colorUsingColorSpaceName:NSCalibratedRGBColorSpace];
+    if (!color) {

Please use c++ style static_casts here and below.  The 256's should also be
256.0f.
+	 color = [NSColor colorWithCalibratedRed:((float)pixel[0]) / 256

These should also be 256.0f I think.  I am also surprised that we don't have a
constructor for Color that takes an NSColor.
+    return Color(static_cast<int>(256.0 * [color redComponent]),
static_cast<int>(256.0 * [color greenComponent]), static_cast<int>(256.0 *
[color blueComponent]));

This could use a comment.
+	 case CSS_VAL_INFOBACKGROUND:
+	     color = 0xFFFBFCC5;


Is returning here ok?  Does this case ever get hit?
+	 default:
+	     return;

Do you need to check for validity again?  Does RenderTheme::systemColor ever
return an invalid color?
+    if (color.isValid())
+	 m_systemColorCache.set(propId, color);


I don't think this comment adds anything.
+    // System color
+    virtual void systemColor(int propId, Color&) const;


You need a space between the switch and the (.

+void RenderTheme::systemColor(int propId, Color& color) const
+{
+    switch(propId) {

There is an extra space before color. 
+	 case CSS_VAL_WINDOWTEXT:
+	      color = 0xFF000000;


I am not thrilled with this line, maybe it should go in
colorForCSSValue(ident).
+	 if (!col.isValid())
+	     theme()->systemColor(ident, col);