[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 #17908|review?                     |review+
               Flag|                            |




------- Comment #22 from darin at apple.com  2007-12-16 10:47 PDT -------
(From update of attachment 17908)
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)


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