[Webkit-unassigned] [Bug 6129] Incomplete implementation of CSS 2.1 system colors

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=6129


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #17571|review?                     |review-
               Flag|                            |




------- Comment #20 from darin at apple.com  2007-12-02 10:31 PDT -------
(From update of attachment 17571)
     (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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.