[webkit-reviews] review denied: [Bug 130580] [Cairo] Implement Path::addPath : [Attachment 227708] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Jae Hyun Park
<jaepark at webkit.org>'s request for review:
Bug 130580: [Cairo] Implement Path::addPath
https://bugs.webkit.org/show_bug.cgi?id=130580

Attachment 227708: Patch
https://bugs.webkit.org/attachment.cgi?id=227708&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=227708&action=review


> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:302
> +    cairo_matrix_t matrix = cairo_matrix_t(transform);

Do you need to use the explicit constructor here or is assignment enough?

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:317
> +    if (path.platformPath() == ensurePlatformPath()) {
> +	   Path clonePath(path);
> +	   cairo_t* cr = clonePath.platformPath()->context();
> +	   cairo_transform(cr, &matrix);
> +	   GUniquePtr<cairo_path_t> pathCopy(cairo_copy_path(cr));
> +	   cairo_append_path(ensurePlatformPath()->context(), pathCopy.get());
> +    } else {
> +	   cairo_t* cr = path.platformPath()->context();
> +	   cairo_transform(cr, &matrix);
> +	   GUniquePtr<cairo_path_t> pathCopy(cairo_copy_path(cr));
> +	   cairo_append_path(ensurePlatformPath()->context(), pathCopy.get());
> +    }

I don't know if you need a special case for appending the path to itself. You
always make a copy of the cairo_path_t anyway. There's a bug here, which is
that you transform the Path and then don't restore the previous transformation.