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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 17 15:05:21 PST 2008


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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26100|review?                     |review+
               Flag|                            |




------- Comment #5 from darin at apple.com  2008-12-17 15:05 PDT -------
(From update of attachment 26100)
> +    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


-- 
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