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

[bugzilla-daemon at webkit.org]

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
http://bugs.webkit.org/show_bug.cgi?id=6129

Attachment 17908: Proposed patch
http://bugs.webkit.org/attachment.cgi?id=17908&action=edit

------- 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
saveGraphicsState].

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.

 207								   
pixelsWide:offscreenRect.size.width
 208								   
pixelsHigh:offscreenRect.size.height

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.

 210							      
samplesPerPixel:4
 211								     
hasAlpha:YES

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

 214								 
bitmapFormat:(NSBitmapFormat)0

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

 215								  
bytesPerRow:(4 * offscreenRect.size.width)