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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 16 10:47:51 PST 2007

Darin Adler <darin at apple.com> has granted Andrew Wellington
<proton at wiretapped.net>'s request for review:
Bug 6129: Incomplete implementation of CSS 2.1 system colors

Attachment 17908: Proposed patch

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. I'm saying review+, but there's one issue that needs to be fixed.
This will break 64-bit unless we change one use of unsigned to NSUInteger.

Yes, you were right and I was wrong about +[NSGraphicsContext

I don't see any reason for systemColor to take a Color& instead of returning a
Color object. These are small objects and need not be returned using out
reference parameters.

RenderTheme::systemColor uses a switch statement instead of an array the way
CSSStyleSelector.cpp did. The array seems easier to read than the switch. The
switch statement is big and it's hard to see which color has which value.

Color objects are simply RGBA values (integers) plus a valid flag. Because of
that, I think m_systemColorCache could store RGBA values instead of color
objects. We don't really need a way to store an invalid object in the cache.

We normally use a different position of the * for ObjC classes than C++
classes; this patch has things like NSColor* where we use NSColor * -- I'm not
a big fan of the rule but I do like to stay consistent.

I think this rect could just be constructed on the line of code where we paint.
We don't really need a 0,0 1x1 rect for the bitmap setup.

 203	     NSRect offscreenRect = NSMakeRect(0, 0, 1, 1);

We normally use "unsigned" rather than "unsigned int" in code like this. Also,
this should be declared further down in the file, right where it's used. Also,
this should be NSUInteger, not "unsigned", so it compiles under 64-bit.

 205	     unsigned int pixel[4];

There's no reason to first initialize this to nil and then give it a real value
two lines later:

 204	     NSBitmapImageRep* offscreenRep = nil;

I think these lines could just say "1" -- there's no real value in getting the
size out of the rect and I don't think it makes things a lot clearer.


I don't think it's helpful to use alpha in the NSBitmapImageRep that we use to
figure out what the color is. We don't read the alpha value out, so we
shouldn't write it in.


There's no need to include a typecast here. 0 would work fine without it.


This can just be value 4 rather than doing math, and I think it might be

bytesPerRow:(4 * offscreenRect.size.width)

More information about the webkit-reviews mailing list