[webkit-reviews] review denied: [Bug 6129] Incomplete implementation of CSS 2.1 system colors : [Attachment 16501] Possible patch

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has denied 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 16501: Possible patch
http://bugs.webkit.org/attachment.cgi?id=16501&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
+	 else if (ident == CSS_VAL_GREY)
+	     col = 0xFF808080;

Why not keep grey in the colorValues array?

+	 color = [NSColor colorWithCalibratedRed:((float)pixel[0]) / 256.0
+					   green:((float)pixel[1]) / 256.0
+					    blue:((float)pixel[2]) / 256.0
+					   alpha:((float)pixel[3]) / 256.0];
+	 [offscreenRep release];
+    } 
+    return Color(static_cast<int>(256.0 * [color redComponent]),
static_cast<int>(256.0 * [color greenComponent]), static_cast<int>(256.0 *
[color blueComponent]));

I believe you should replace every occurrence of 256.0 with 255.0f.

+void RenderThemeMac::systemColor(int propId, Color& color) const
+{
+    if (m_systemColorCache.contains(propId))
+	 color = m_systemColorCache.get(propId);

It would be good to return right here so the long switch statement doesn't have
to be in an else block.

r- so the above can be fixed. The design looks good overall. We're also going
to have to consider whether this will break existing web sites.