[webkit-reviews] review denied: [Bug 28122] [Haiku] Adding some simple graphic files (IntPoint, =?UTF-8?Q?=20IntRect=E2=80=A6?=) : [Attachment 34407] Patch to add eight graphic files.

[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 28122: [Haiku] Adding some simple graphic files (IntPoint, IntRect…)
https://bugs.webkit.org/show_bug.cgi?id=28122

Attachment 34407: Patch to add eight graphic files.
https://bugs.webkit.org/attachment.cgi?id=34407&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
What does the color struct look like?  Could you just use a COMPILE_ASSERT to
assert identical layout with the WebCore:RGBA32 type and not need to call
make_color, etc?

See how I did things in ColorSkia (where we just cast directly).  Your current
solution is *totally fine*.  I just wanted to bring the opportunity of casting
directly to your attention.

Why are the +1 needed for one way conversion:
 38	, m_size(rect.Width() + 1, rect.Height() + 1)

and -1 is not needed the other way:
 44	return BRect(BPoint(x(), y()), BSize(width(), height()));

That at least needs a comment.

It seems if BPoint is a float point you don't want to have it implicitly
constructed.
 IntPoint::IntPoint(const BPoint& point)
 37	: m_x(static_cast<int>(point.x))
 38	, m_y(static_cast<int>(point.y))
 39 {
 40 }

See how CGPoint is handled.

Yeah, it seems that BRect, BPoint, BSize should only map to the Float* variants
(similar to how CG* works).  Why a FIXME here, but not before?
42 IntRect::operator BRect() const
 43 {
 44	// FIXME: subtract 1 from height and width?
 45	return BRect(BPoint(x(), y()), BSize(width(), height()));
 46 }

Parts of this would be easy to approve.  I think you need to double-check how
you do the B* to Float*/Int* conversions.  Since it seems that B* are all
floating-point values, you can follow exactly what the CG* types do, since
those are all floating point values.