[Webkit-unassigned] [Bug 22902] RenderPath clean-up for strokeBoundingBox

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 21 12:40:56 PST 2008


https://bugs.webkit.org/show_bug.cgi?id=22902


zimmermann at kde.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26187|                            |review-
               Flag|                            |




------- Comment #7 from zimmermann at kde.org  2008-12-21 12:40 PDT -------
(From update of attachment 26187)
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 :-)


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list