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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 26100: Move strokeBBox code to Path
https://bugs.webkit.org/attachment.cgi?id=26100&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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