Bug 28122 – [Haiku] Adding some simple graphic files (IntPoint, IntRect…)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Maxime Simon]

Here is a patch to add 8 simple files:
WebCore/platform/graphics/haiku/ColorHaiku.cpp
WebCore/platform/graphics/haiku/FloatPointHaiku.cpp
WebCore/platform/graphics/haiku/FloatRectHaiku.cpp
WebCore/platform/graphics/haiku/GradientHaiku.cpp
WebCore/platform/graphics/haiku/IntPointHaiku.cpp
WebCore/platform/graphics/haiku/IntRectHaiku.cpp
WebCore/platform/graphics/haiku/IntSizeHaiku.cpp
WebCore/platform/graphics/haiku/PathHaiku.cpp

Regards,
Maxime

[Maxime Simon]

Created attachment 34407 [details]
Patch to add eight graphic files.

[Eric Seidel (no email)]

Comment on attachment 34407 [details]
Patch to add eight graphic files.

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.

[Eric Seidel (no email)]

The parts that I didn't comment on could be split out into a separate patch if you wanted quicker approval there.  This patch is definitely reviewable at this size however (it's mostly boilerplate).

[Maxime Simon]

(In reply to comment #2)
> (From update of attachment 34407 [details])
> 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?

Indeed rgb_color is a struct of { red, green, blue, alpha } like WebCore::RGBA32, So I should change this.

> 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()));

I assume that the +1/-1 are from the previous port and are what was done in the Syllable port. But I'm pretty sure they aren't necessary.

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

In CGPoint they used a static_cast as I did. :D

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

I know that both BRect and BSize have IntegerWidth() and IntegerHeight() methods, but BPoint needs a cast to int.
That's what I used, but I will check again, and at least remove the +1/-1.

[Maxime Simon]

(In reply to comment #2)
> (From update of attachment 34407 [details])
> 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.

ColorSkia? I do not want to sound boring, but there is no such file.
In fact I was wrong, I can't cast an unsigned (RGBA32) to a struct of unsigned (rgb_color). So it would still use the make_color function.

[Maxime Simon]

Created attachment 34426 [details]
Patch to add eight graphic files for WebCore/platform/graphics/haiku.

[Ryan Leavengood]

(In reply to comment #5)
> 
> ColorSkia? I do not want to sound boring, but there is no such file.

I don't see that either.

> In fact I was wrong, I can't cast an unsigned (RGBA32) to a struct of unsigned
> (rgb_color). So it would still use the make_color function.

Are you sure? Doing a cast instead of calling a function would probably be
faster and I'm sure this conversion would need to be done a lot.

Eric, our rgb_color struct is defined here:

http://svn.berlios.de/viewcvs/haiku/haiku/trunk/headers/os/interface/GraphicsDefs.h?revision=25797&view=markup

Since this is C++ code the rgb_color struct is essentially a public class, so
maybe it can't be just cast even if the members are in the same order (then of
course there is endianness, which may also be an issue, right?)

I am not particularly skilled at the moment on these low-level types of issues
(too much Ruby, HTML and JavaScript these last few years), so any insight is
appreciated.

Also in regards to all the BRect and IntRect stuff, that probably does need an
audit.

[Eric Seidel (no email)]

Note that the CG constructors are explicit:
    explicit IntPoint(const CGPoint&); // don't do this implicitly since it's lossy

[Eric Seidel (no email)]

An old ColorSkia:
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/port/platform/graphics/ColorSkia.cpp?revision=3958&view=markup&sortby=date&pathrev=3958

[Eric Seidel (no email)]

Comment on attachment 34426 [details]
Patch to add eight graphic files for WebCore/platform/graphics/haiku.

OK.

[Eric Seidel (no email)]

Comment on attachment 34426 [details]
Patch to add eight graphic files for WebCore/platform/graphics/haiku.

Clearing flags on attachment: 34426

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/platform/graphics/haiku/ColorHaiku.cpp
	A	WebCore/platform/graphics/haiku/FloatPointHaiku.cpp
	A	WebCore/platform/graphics/haiku/FloatRectHaiku.cpp
	A	WebCore/platform/graphics/haiku/GradientHaiku.cpp
	A	WebCore/platform/graphics/haiku/IntPointHaiku.cpp
	A	WebCore/platform/graphics/haiku/IntRectHaiku.cpp
	A	WebCore/platform/graphics/haiku/IntSizeHaiku.cpp
	A	WebCore/platform/graphics/haiku/PathHaiku.cpp
Committed r47142
	M	WebCore/ChangeLog
	A	WebCore/platform/graphics/haiku/PathHaiku.cpp
	A	WebCore/platform/graphics/haiku/GradientHaiku.cpp
	A	WebCore/platform/graphics/haiku/ColorHaiku.cpp
	A	WebCore/platform/graphics/haiku/FloatPointHaiku.cpp
	A	WebCore/platform/graphics/haiku/FloatRectHaiku.cpp
	A	WebCore/platform/graphics/haiku/IntPointHaiku.cpp
	A	WebCore/platform/graphics/haiku/IntSizeHaiku.cpp
	A	WebCore/platform/graphics/haiku/IntRectHaiku.cpp
r47142 = e4c305eb83f3dda05416b0db9336be7cd0e99ba8 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47142

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.