[webkit-reviews] review denied: [Bug 26949] WebCore part of the Haiku WebKit port : [Attachment 32228] Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 26949: WebCore part of the Haiku WebKit port
https://bugs.webkit.org/show_bug.cgi?id=26949

Attachment 32228: Patch to add Haiku-specific drawing files for
WebCore/platform/graphics/.
https://bugs.webkit.org/attachment.cgi?id=32228&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Who owns the BView in the GraphicsContextPlatformPrivate?  I'm surprised that
GCPlatformPrivate doesnt' need to retain it?

style:
npoints should be numberOfPoints or pointsLength or similar.

Style:
8     if (rects.size() > 1)
 229	 {

spacing:
 235	     m_data->view->StrokeRect(region.Frame(),B_MIXED_COLORS);

Naming:
 257 FloatRect GraphicsContext::roundToDevicePixels(const FloatRect& frect)

Naming:
 287 void GraphicsContext::strokeRect(const FloatRect& r, float width)
 298 void GraphicsContext::setLineCap(LineCap lc)

No need to wrap lines like this:
void GraphicsContext::addInnerRoundedRectClip(const IntRect& rect,
 451						   int thickness)
 452 

I'm surprised your compiler can't convert from "return 0":
    return PassRefPtr<Icon>(0);

Why are you adding the empy ImageBufferData class?

I think you mean something like "if the image hasn't fully loaded":
 130	 if (!image) // If it's too early we won't have an image yet.

But I'm still confused as to why we'd be calling pattern drawing routines with
an image w/o a first frame.

I think there are functions to do this for you:
 140	 ctxt->clip(IntRect(dstRect.x(), dstRect.y(), dstRect.width(),
dstRect.height()));

IntRect(dstRect) probably does what you want.  Although you might want
enclosingIntRect() there.

c++ style casts please:
 57	const unsigned char* uContents = (const unsigned char*) data.data();


I'm surprised:
 50 ImageDecoder* createDecoder(const Vector<char>& data)

isn't shared between teh ports?

Spacing:
 45	return BRect(BPoint(x(), y()), BSize(width(),height()));

It's better to break patches down by size instead of directory.

r- for all the nits above.