[webkit-reviews] review denied: [Bug 6129] Incomplete implementation of CSS 2.1 system colors : [Attachment 17571] Final (hopefully) patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 2 10:31:26 PST 2007


Darin Adler <darin 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 17571: Final (hopefully) patch
http://bugs.webkit.org/attachment.cgi?id=17571&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
     (static_cast<float>(pixel[0])) / 256

Despite the reason you offered, it's not correct to divide by 256. That turns a
0xFF into 255.0/256.0 rather than 1.0. That's the same as the reason you see
use of 255.0f in the nsColor function in ColorMac.mm. Same for the
multiplication at the end of the function. This should just use the Color
functions that already exist as much as possible and should not involve new
code. See Mitz's comment for some hints about how you could fix this.

I also think it would be better to convert the pixels directly into a Color
rather than making the pixel, then turning the pixel into an NSColor, then
extracting the channel values again to make a Color.

+	 [NSGraphicsContext saveGraphicsState];

I believe the call to saveGraphicsState is neither necessary nor sufficient. We
need to save ad restore the current context. The saveGraphicsState method saves
the state *of* the current context. Since we're making a new context, what we
need to save is what the current context is, not state within it.

+	     // take system default

I don't think you mean "system" here. You mean take the theme-independent
default?

+	     // Using this value instead of controlColor to avoid changing
existing button colors

The verb tense here is confusing. Please write a comment that will make sense
in the future, rather than one that describes the current change.

+	     // There is no NSColor for this so we use the previous hard coded
value

The word previous here won't make sense in the future. Please find another way
to word this.


More information about the webkit-reviews mailing list