[webkit-reviews] review denied: [Bug 22902] RenderPath clean-up for strokeBoundingBox : [Attachment 26097] Move strokeBBox code to Path

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 17 08:44:35 PST 2008


Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 22902: RenderPath clean-up for strokeBoundingBox
https://bugs.webkit.org/show_bug.cgi?id=22902

Attachment 26097: Move strokeBBox code to Path
https://bugs.webkit.org/attachment.cgi?id=26097&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.


More information about the webkit-reviews mailing list