Bug 34683 – [Haiku] Implement Path, Gradient and flesh out GraphicsContextHaiku

• Component: Platform
• Status: RESOLVED WONTFIX

[Stephan Aßmus]

The patch unfortunately cannot be broken into smaller pieces.

PathHaiku was irritatingly implemented as BRegion, while BRegion has nothing to
do with paths. BShape is the way to go. I've implemented the subset of
functionality that can be implemented using BShape.

PathGradient was previously not implemented at all. I've taken the Qt
implementation as a reference and websites using gradients seem to render fine.

GraphicsContextHaiku was not implementing a great deal of functionality. The
patch implements

 * partial support for paths and gradients
 * more clipping methods
 * support for round rects
 * support for transparency layers
 * support for translating the context, as needed for scrolling
 * changes the strokeArc() method to produce beautiful arcs as needed for box
elements
   without the need to implement clipping paths.
 * implements filling rects
 * implements rounding to device pixels
 * a few missing new methods that broke the build on Haiku
 * fixes a few warnings

--

[Stephan Aßmus]

Created attachment 48295 [details]
Implements Path and Gradient support for Haiku, fleshes out GraphicsContextHaiku

Patch against r54275.

[WebKit Review Bot]

Attachment 48295 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/240826

[Stephan Aßmus]

The Qt build exited with an internal compiler error. This patch doesn't touch Qt code, unless the diffs to GraphicsContext.h, Path.h and Gradient.h didn't apply at the right places (inside the #if PLATFORM(HAIKU)).

[Eric Seidel (no email)]

Comment on attachment 48295 [details]
Implements Path and Gradient support for Haiku, fleshes out GraphicsContextHaiku

This patch is kinda large and would be better done in pieces.

[Ryan Leavengood]

Hi Eric,

Any suggestions on how to split this up? Most of the changes are in one file, GraphicsContextHaiku, which is a rather large file due to the design of the WebCore platform code. The other changes need to be part of this patch since they relate to the changes in GraphicsContextHaiku. If you could reconsider your position about the size of this patch I would really appreciate it :)

Stephan is doing a great job updating this port which I've let get out of date. Getting these changes into the main WebKit repo will make it much easier for us to work together without having to email patches or create external repos with copies of the WebKit code, which I know you guys tend to frown on.

[Stephan Aßmus]

Created attachment 48990 [details]
Implments creating and filling platform gradients

Smaller patch for just GradientHaiku.cpp and Gradient.h. If the Haiku build system would be integrated, this patch would break the Haiku build, since GraphicsContextHaiku.cpp would have to be adjusted as well, but it doesn't matter. It's more important to reduce the size of the patch. I've also fixed some coding style violations compared to the previous patch (using C++ casts in C++ code). Patch is against r54932.

[Stephan Aßmus]

Created attachment 48991 [details]
 Implement native Path backend.

Smaller patch for just implementing the Path support. This one can really not be broken down any further. Again, it would actually need changes in GraphicsContextHaiku.cpp, but since the Haiku build system is not included in WebKit, it doesn't matter.

[Eric Seidel (no email)]

Comment on attachment 48990 [details]
Implments creating and filling platform gradients

Thanks for splitting it up.

It's sad that we don't have an easy way for ports to use OwnPtr here, as that would be nicer than having to do platformDestroy().

[Eric Seidel (no email)]

Comment on attachment 48991 [details]
 Implement native Path backend.

I sometimes wrap locking blocks in { } to indicate that I'm holding a lock, I'm not sure what WebKit's official style is.  We generally use stack objects to do locking/unlocking automatically for us:
84     bitmap.Lock();
 85     bitmap.AddChild(&view);
 86     // Current pen location is used as origin for the shape.
 87     view.MovePenTo(-point.x(), -point.y());
 88     // TODO: Handle WindRule... (needs support in BView, the backend already
 89     // support it.)
 90     view.FillShape(m_path);
 91     view.Sync();
 92 
 93     uint8* bits = reinterpret_cast<uint8*>(bitmap.Bits());
 94     bool result = bits[0] < 128;
 95 
 96     view.RemoveSelf();
 97     bitmap.Unlock();

Yeah, given that you do it twice, i tmight make sense to write a simple auto-locking class for just use within this file.

[WebKit Commit Bot]

Comment on attachment 48990 [details]
Implments creating and filling platform gradients

Clearing flags on attachment: 48990

Committed r55115: <http://trac.webkit.org/changeset/55115>

[Stephan Aßmus]

Created attachment 49270 [details]
Implement native Path backend.

This new patch implements the HitTestBitmap helper class. It also fixes a bug in Path::translate() where the MoveTo() op didn't adjust y.

[Stephan Aßmus]

I just realized I should perhaps explain the "locking" thing. Usually, a BView is attached to a BWindow. In fact, it can only be attached to a BWindow. A BWindow usually runs it's own thread for dispatching events. To help coders not make the mistake to invoke BView methods without having the BWindow looper thread properly locked, the BView methods all check the locking status of their parent BWindow and drop the program into the debugger if there is anything wrong. When invoking methods from within the BWindow thread, no locking needs to be done, since it always happens from within the event dispatching code which already has the BWindow locked. It's just important when invoking from other threads, like BApplication.

Then Be thought that it would be nice to be able to attach BViews also to BBitmaps. For this, they created a special private constructor for BWindow which only BBitmap has access to. A BBitmap which is created with the flag to allow attaching BViews creates such a dummy BWindow for holding the child BViews. It also makes sure that there is an app_server thread running which performs the actual drawing commands for the BView. But this BWindow is special in that the event looper thread never runs. Still, the BView methods check for the locked status of the parent BWindow. So the locking here happens more to prevent the invokation of the debugger, than to actually synchronize critical sections. If WebCore was to run documents, layouting and painting in multiple threads, then it could actually become useful to use the BBitmap lock for synchronization. For now it would just add unnecessary overhead.

I hope this makes it clear why Lock() and Unlock() in HitTestBitmap don't implement the stack object based auto-locking that you were thinking of. Such technique is otherwise well known in Haiku coding, but it doesn't apply here.

[Stephan Aßmus]

Created attachment 49527 [details]
Implement native Path backend.

The new patchs adds reference counting to global HitTestBitmap object, so that the internal BBitmap can be destroyed before the invokation of the global destructors. This is necessary, since destroying BBitmaps require a running BApplication, which will be gone at that point.

[Eric Seidel (no email)]

Comment on attachment 49527 [details]
Implement native Path backend.

Why not use ref() and deref() as the names so that we could use RefPtr to hold this?

[Stephan Aßmus]

Changing the method names to ref() and deref() in order to use RefPtr, I would have to add another member to the Path class just for the Haiku platform. This adds clutter to the common Path.h header and Path objects would be ca couple bytes bigger on Haiku for no real benefit, IMHO. The only thing I gain is that I don't have to deref in the Path destructor. To be honest, I don't think this change is justified. I don't even know if adding a RefPtr<HitTestBitmap> to the Path class for Haiku is even possible without making the whole HitTestBitmap class known in the Path.h header.

If you think this change absolutely has to be done in order for the patch to get the green light, I will of course do it, please just leave a note, I would not want the patch to remain sitting here. I just thought I'd try to explain why I think it's better as it is. :-)

[Stephan Aßmus]

I hope this is not too annoying, but just wanted to give a reminder that I am still waiting on feedback to my last comment. Thanks for your time!

[David Levin]

Comment on attachment 49527 [details]
Implement native Path backend.

Just a few minor nits. Sorry (I would r+ if you could land it).

> Index: WebCore/ChangeLog
> +            Fixed memory corruption, curtesy of Michael Lotz. We can't just assign pointers!

courtesy 

> +        (WebCore::Path::addQuadCurveTo):
> +            Implemented (curtesy of Michael Lotz).

courtesy 


> Index: WebCore/platform/graphics/haiku/PathHaiku.cpp
> +// A one pixel sized BBitmap for drawing into. Default high-color of BViews

Single space after . in comments.


> +        virtual status_t IterateLineTo(int32 lineCount, BPoint* linePts)

Abbreviations are discouraged in WebKit. "Pts".

> +    // BShapeIterator allows us to modify the path data "in place"

Please add a "."

[Ahmad Saleem]

Haiku port doesn't exist and if there is any unofficial - these patch might not be as useful as they were in the past and might require total effort from scratch.

Marking this bug as 'RESOLVED WONTFIX' from usability perspective and if someone can salvage for their 'Haiku' port, they are happy to do so.