[webkit-reviews] review denied: [Bug 6129] Incomplete
implementation of CSS 2.1 system colors : [Attachment 16501]
Possible patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Oct 27 23:10:18 PDT 2007
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.
More information about the webkit-reviews
mailing list