Bug 22902 – RenderPath clean-up for strokeBoundingBox

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Remove the platform dependent code from RenderPath to the graphics of each plaform.

[Dirk Schulze]

Created attachment 26097 [details]
Move strokeBBox code to Path

move the code to Path.

[Nikolas Zimmermann]

Comment on attachment 26097 [details]
Move strokeBBox code to Path

Hi Dirk, nice patch! I've got some comments, that lead to r-:

> +
> +        Move the the boundingBox code for strokes from
> +        RenderPath to Path.

Should be a bit more explicit, something like "Move strokeBoundingRect functionality in Path".

> Index: WebCore/platform/graphics/Path.h
> +    typedef void (*StyleDecoratorCallback)(GraphicsContext*, void*);

There's already a typedef for the "PathApplierFunction" below, you might want to move the new typedef there as well, to group the callbacks.
Though I suggested 'StyleDecoratorCallback' the name is flawed. How about calling it 'PathStyleDecoratorCallback"?

> +        FloatRect strokeBoundingRect(StyleDecoratorCallback callBack, void* data);

You can omit the parameter name "callBack", and maybe we should change the argument order, as the apply() function has the user data "void* data" as first argument. Better to keep it consistent.

> Index: WebCore/platform/graphics/cairo/PathCairo.cpp
>  
> +FloatRect Path::strokeBoundingRect(StyleDecoratorCallback callBack, void* data)
> +{
> +    GraphicsContext gc(platformPath()->m_cr);
> +    if (callBack) {
> +        (*callBack)(&gc, data);
> +        double x0, x1, y0, y1;
> +        cairo_stroke_extents(platformPath()->m_cr, &x0, &y0, &x1, &y1);
> +        return FloatRect(x0, y0, x1 - x0, y1 - y0);
> +    }
> +    return boundingRect();
> +}
> +

Okay, this is a bit flawed - we should try to calculate the stroke extents, even if there is no callback set for decorating the style. Something along the lines:
GraphicsContext gc(..);
if (callBack)
     (*callBack)(&gc, data);

double x0, x1....
cairo_stroke_extents(...)
return FloatRect(..);

This way we also don't need the boundingRect() fallback.
You might as well want to store platformPath()->m_cr in a local variable, to avoid calling the function twice. Not required though.


> Index: WebCore/platform/graphics/cg/PathCG.cpp
  > +CGContextRef inline scratchContext()
> +{
> +    static CGContextRef scratch = 0;
> +    if (!scratch) {
> +        CFMutableDataRef empty = CFDataCreateMutable(NULL, 0);
> +        CGDataConsumerRef consumer = CGDataConsumerCreateWithCFData(empty);
> +        scratch = CGPDFContextCreate(consumer, NULL, NULL);
> +        CGDataConsumerRelease(consumer);
> +        CFRelease(empty);
> +
> +        CGFloat black[4] = {0, 0, 0, 1};
> +        CGContextSetFillColor(scratch, black);
> +        CGContextSetStrokeColor(scratch, black);
> +     }
> +    return scratch;
> +}

I'd name it 'static inline CGContextRef' - as scratchContext() is only used in this translation unit.
Please indicate the static in the variable name "static CGContextRef s_contextRef = 0;".

> +FloatRect Path::strokeBoundingRect(StyleDecoratorCallback callBack, void* data)
> +{
> +    CGContextRef context = scratchContext();
> +
> +    CGContextBeginPath(context);
> +    CGContextAddPath(context, platformPath());
> +
> +    GraphicsContext gc(context);
> +    if (callBack) {
> +        (*callBack)(&gc, data);
> +        CGContextReplacePathWithStrokedPath(context);
> +        if (CGContextIsPathEmpty(context))
> +            return FloatRect();
> +        return FloatRect(CGContextGetPathBoundingBox(context));
> +    }
> +    return boundingRect();
> +}


We need to call CGContextSaveGState before calling CGContextBeginPath and CGContextRestoreGState after calling CGContextGetPathBoundingBox, as the scratchContext() is shared. Consecutive calls would fail at the moment.
Same comment as for Cairo, the callBack can be null, and we still should do the CGContextReplacePath... logic.

> Index: WebCore/platform/graphics/qt/GraphicsContextQt.cpp
> ===================================================================
> --- WebCore/platform/graphics/qt/GraphicsContextQt.cpp	(revision 39303)
> +++ WebCore/platform/graphics/qt/GraphicsContextQt.cpp	(working copy)
> @@ -525,6 +525,12 @@ void GraphicsContext::drawConvexPolygon(
>      p->restore();
>  }
>  
> +QPen GraphicsContext::pen()
> +{
> +    QPainter *p = m_data->p();
> +    return p->pen();
> +}
> +

Qt doesn't follow the style convention? If it does, please move the star.
'p' can never be null? Otherwhise you might want to catch that.

> Index: WebCore/platform/graphics/qt/PathQt.cpp
> +FloatRect Path::strokeBoundingRect(StyleDecoratorCallback callBack, void* data)
> +{
> +    std::auto_ptr<ImageBuffer> scratchImage = ImageBuffer::create(IntSize(1, 1), false);
> +    GraphicsContext* gc = scratchImage->context();
> +    if (callBack) {
> +        (*callBack)(gc, data);
> +        QPen pen = gc->pen();
> +        QPainterPathStroker stroke;
> +        stroke.setWidth(pen.widthF());
> +        stroke.setCapStyle(pen.capStyle());
> +        stroke.setJoinStyle(pen.joinStyle());
> +        stroke.setMiterLimit(pen.miterLimit());
> +        stroke.setDashPattern(pen.dashPattern());
> +        stroke.setDashOffset(pen.dashOffset());
> +        return (stroke.createStroke(*platformPath())).boundingRect();
> +    }
> +    return boundingRect();
> +}

Ok, same comment as for Cairo/Cg, should calculate the stroke width even without a callBack passed.
Hm, aren't we supposed to create a 'shared' scratchContext() - maybe the same abstraction as in CG is needed?
A new scratchContext() function storing the scratch context as static?
Doesn't scratchImage even leak at the moment?

> Index: WebCore/rendering/RenderPath.h
> ===================================================================
> --- WebCore/rendering/RenderPath.h	(revision 39303)
> +++ WebCore/rendering/RenderPath.h	(working copy)
> @@ -31,6 +31,7 @@
>  #include "FloatRect.h"
>  
>  #include "RenderObject.h"
> +#include "RenderStyle.h"

A RenderStyle class forward should be enough, no?

Thanks for starting this great cleanup work, looking forward to the next patch.

[Dirk Schulze]

Created attachment 26098 [details]
Move strokeBBox code to Path

Made most of the suggested changes. Haven't changed the coding style on GCQt to match the rest of the code, and removed RenderSytle instead of RenderObject in RenderPath.h, paintinfo needs the object.
The shared context causes a segfault on qt. Leafing std::auto_ptr<ImageBuffer> for now.

[Dirk Schulze]

Created attachment 26100 [details]
Move strokeBBox code to Path

fixes some issues and style problems. Passes all Layout and pixeltests.

[Darin Adler]

Comment on attachment 26100 [details]
Move strokeBBox code to Path

> +    typedef void (*PathStyleDecoratorCallback) (void*, GraphicsContext*);
>      typedef void (*PathApplierFunction) (void* info, const PathElement*);

It's a little weak to use these void* pointers to pass data. A more C++ style solution would be to have these decorators and appliers be objects derived from an abstract base class.

Also, we normally don't have that space between the close parenthesis and open parentheses in other source files that define function pointer types.

It seems unnecessarily inconsistent to call the data "data" when calling the function, "info" in one function type definition, and give no name in the other function type definition. I'd like to see naming that made it clear the data goes with the callback. Or even better, the object design instead of the callback design.

> +static inline CGContextRef scratchContext()
> +{
> +    static CGContextRef s_contextRef = 0;
> +    if (!s_contextRef) {
> +        CFMutableDataRef empty = CFDataCreateMutable(NULL, 0);
> +        CGDataConsumerRef consumer = CGDataConsumerCreateWithCFData(empty);
> +        s_contextRef = CGPDFContextCreate(consumer, NULL, NULL);
> +        CGDataConsumerRelease(consumer);
> +        CFRelease(empty);
> +
> +        CGFloat black[4] = {0, 0, 0, 1};
> +        CGContextSetFillColor(s_contextRef, black);
> +        CGContextSetStrokeColor(s_contextRef, black);
> +    }
> +    return s_contextRef;
> +}

This seems like a long function to inline, and has an extra branch on it. Here's how I'd write it:

    static CGContextRef createScratchContext()
    {
        // lots of code
        return context;
    }

    static inline CGContextRef scratchContext()
    {
        static CGContextRef context = createScratchContext();
        return context;
    }

> +FloatRect Path::strokeBoundingRect(void* data, PathStyleDecoratorCallback callBack)

If "callback" is one word, then it should always be one word. The "B" should not be capitalized.

> +    return FloatRect(box);

Do you need to explicitly cast to FloatRect? Won't this be converted automatically?

> +    //FIXME: We should try to use a 'shared Context' instead of creating a new ImageBuffer
> +    //       on each call.

Strange formatting here.

> +struct StrokeBoundingBoxData {
> +    const RenderObject* object;
> +    RenderStyle* style;
> +};

This struct is an implementation detail that shouldn't be in a header file.

r=me as is, but please consider some of my comments

[Dirk Schulze]

Created attachment 26187 [details]
Move strokeBBox code to Path 3

third attempt

[Nikolas Zimmermann]

Comment on attachment 26187 [details]
Move strokeBBox code to Path 3

Nice patch, overall, r- due some subtle issues:

> Index: WebCore/platform/graphics/StrokeStyleApplier.h
> +    class StrokeStyleApplier {
> +    public:
> +        virtual ~StrokeStyleApplier() {}
> +        virtual void strokeStyle(GraphicsContext*) = 0;
> +    };
> +}

I'd rather move the destructor in a protected section, and also add a constructor - protected as well.
So only the classes deriving from StrokeStyleApplier can create/destruct it.

> Index: WebCore/platform/graphics/cg/PathCG.cpp
> +static CGContextRef createScratchContext()
> +{
> +    static CGContextRef s_contextRef = 0;
> +    if (!s_contextRef) {
> +        CFMutableDataRef empty = CFDataCreateMutable(NULL, 0);
> +        CGDataConsumerRef consumer = CGDataConsumerCreateWithCFData(empty);
> +        s_contextRef = CGPDFContextCreate(consumer, NULL, NULL);
> +        CGDataConsumerRelease(consumer);
> +        CFRelease(empty);
> +
> +        CGFloat black[4] = {0, 0, 0, 1};
> +        CGContextSetFillColor(s_contextRef, black);
> +        CGContextSetStrokeColor(s_contextRef, black);
> +    }
> +    return s_contextRef;
> +}
I think, what Darin intented is to have a createScratchContext() class that only creates the CGContextRef, it is not supposed to store it.
So just rename "static CGContextRef s_contextRef" -> "CGContextRef contextRef", and return that.

> Index: WebCore/rendering/RenderPath.cpp
> +class StrokeBoundingRectStyleApplier : public StrokeStyleApplier {

Sorry that I suggested this bad name :-) I think "StrokeStyle" should remain as one word, so "BoundingRectStrokeStyleApplier" may sounds a bit more approriate.
Can you clean the review flag of the patch Darin already r+ed?

Keen to review the final version :-)

[Dirk Schulze]

Created attachment 26188 [details]
Move strokeBBox code to Path 4

last version?

[Nikolas Zimmermann]

Comment on attachment 26188 [details]
Move strokeBBox code to Path 4

This looks perfectly fine, r=me. Please fix two issues, before landing:

> Index: WebCore/platform/graphics/StrokeStyleApplier.h
> +
> +    protected:
> +        virtual ~StrokeStyleApplier() {}
Also add a protected constructor: "StrokeStyleApplier() {}" here.

> Index: WebCore/platform/graphics/cg/PathCG.cpp
> +static CGContextRef createScratchContext()
> +{
> +    CGContextRef contextRef = 0;
> +    if (!contextRef) {
> +        CFMutableDataRef empty = CFDataCreateMutable(NULL, 0);
> +        CGDataConsumerRef consumer = CGDataConsumerCreateWithCFData(empty);
> +        contextRef = CGPDFContextCreate(consumer, NULL, NULL);
> +        CGDataConsumerRelease(consumer);
> +        CFRelease(empty);
> +
> +        CGFloat black[4] = {0, 0, 0, 1};
> +        CGContextSetFillColor(contextRef, black);
> +        CGContextSetStrokeColor(contextRef, black);
> +    }
> +    return contextRef;
> +}

The if (!contextRef) case should be removed, it's always true.

[Dirk Schulze]

landed in r39427.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> landed in r39427.

http://trac.webkit.org/changeset/39427

This broke the WX build.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> This broke the WX build.

http://build.webkit.org/waterfall

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> (In reply to comment #10)
> > landed in r39427.
> 
> http://trac.webkit.org/changeset/39427
> 
> This broke the WX build.

Added stub implementation:

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/wx/PathWx.cpp
Committed r39560

http://trac.webkit.org/changeset/39560