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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Nov 24 20:15:49 PST 2007

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

Attachment 17440: Proposed patch 2

------- 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
+    if (!color) {

Please use c++ style static_casts here and below.  The 256's should also be
+	 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.
+	     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. 
+	      color = 0xFF000000;

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

More information about the webkit-reviews mailing list