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

[bugzilla-daemon at webkit.org]

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

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

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