Bug 130580 – [Cairo] Implement Path::addPath

• Component: Canvas
• Status: RESOLVED FIXED

[Jae Hyun Park]

Add support for addPath method for platforms using cairo.

[Jae Hyun Park]

Created attachment 227415 [details]
Patch

[WebKit Commit Bot]

Attachment 227415 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cairo/PathCairo.cpp:302:  c_matrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 227415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227415&action=review

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:310
> +        OwnPtr<cairo_path_t> pathCopy = adoptPtr(cairo_copy_path(cr));

Please don’t use OwnPtr in new code. We are trying to get rid of OwnPtr. You should used std::unique_ptr in new code. I’m also surprised that it’s safe to call delete on a path returned from cairo_copy_path, but I guess that’s what the surrounding code already does. I think the way to write this with unique_ptr is something like this:

    std::unique_ptr<cairo_path_t> pathCopy { cairo_copy_path(cr) };

[Martin Robinson]

Comment on attachment 227415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227415&action=review

>> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:310
>> +        OwnPtr<cairo_path_t> pathCopy = adoptPtr(cairo_copy_path(cr));
> 
> Please don’t use OwnPtr in new code. We are trying to get rid of OwnPtr. You should used std::unique_ptr in new code. I’m also surprised that it’s safe to call delete on a path returned from cairo_copy_path, but I guess that’s what the surrounding code already does. I think the way to write this with unique_ptr is something like this:
> 
>     std::unique_ptr<cairo_path_t> pathCopy { cairo_copy_path(cr) };

This works because we have a template specialization for cairo_path_t (template <> void deleteOwnedPtr<cairo_path_t>(cairo_path_t*);) in OwnPtrCairo.h. I think we could use GUniquePtr here, but we'd need to make a deleter using WTF_DEFINE_GPTR_DELETER.

[Jae Hyun Park]

Created attachment 227610 [details]
Patch

[WebKit Commit Bot]

Attachment 227610 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cairo/PathCairo.cpp:302:  c_matrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 227610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227610&action=review

> Source/WTF/wtf/gobject/GUniquePtr.h:41
> +    macro(cairo_path_t, cairo_path_destroy) \

This isn't really the right place for this. We want to add this in a file alongside OwnPtrCairo.h, I think.

[Jae Hyun Park]

Created attachment 227637 [details]
Patch

[WebKit Commit Bot]

Attachment 227637 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cairo/PathCairo.cpp:302:  c_matrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jae Hyun Park]

It seems like other cairo_matrix_t in PathCairo.cpp is named c_matrix, which is not in accordance with webkit style.

Would it be better if I rename cairo_matrix_t c_matrix to matrix to pass the style check? Or is it a convention for cairo_matrix_t?

[Martin Robinson]

(In reply to comment #10)
> It seems like other cairo_matrix_t in PathCairo.cpp is named c_matrix, which is not in accordance with webkit style.
> 
> Would it be better if I rename cairo_matrix_t c_matrix to matrix to pass the style check? Or is it a convention for cairo_matrix_t?

I think it would be better to fix the name.

[Jae Hyun Park]

Created attachment 227708 [details]
Patch

[Martin Robinson]

Comment on attachment 227708 [details]
Patch

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.

[Jae Hyun Park]

Created attachment 227717 [details]
Patch

[Jae Hyun Park]

(In reply to comment #13)
> (From update of attachment 227708 [details])
> 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?
I used one constructor instead of constructor + assignment in the latest patch. 
> 
> > 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.

Fixed.

Thanks for the review!

[Dirk Schulze]

*** Bug 130464 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

Comment on attachment 227717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227717&action=review

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:309
> +    GUniquePtr<cairo_path_t> pathCopy(cairo_copy_path(cr));

Are you sure that you need a copy of the path? We had addPath implemented in the past. I don't think we copied it.

[Jae Hyun Park]

(In reply to comment #17)
> (From update of attachment 227717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227717&action=review
> Are you sure that you need a copy of the path? We had addPath implemented in the past. I don't think we copied it.

If you mean addPath that was implemented in GraphicsContextCairo, it used appendWebCorePathToCairoContext(), which calls appendPathToCairoContext().
In appendPathToCairoContext, it calls cairo_copy_path() and cairo_append_path.

[Dirk Schulze]

Comment on attachment 227717 [details]
Patch

r=me

[Jae Hyun Park]

(In reply to comment #19)
> (From update of attachment 227717 [details])
> r=me
Thanks for the review.

However, this patch cannot be committed right now because we cannot use GUniquePtr for cairo_path_t as WinCairo port does not glib.

Would it be ok if i used OwnPtr<cairo_path_t> for now, and change the use of OwnPtrCairo altogether in 131531 bug?

[Jinwoo Song]

Jae Hyun Park, is there any progress? If you don't mind, I'd like to propose to use std::unique_ptr with custom deleter.

[Jae Hyun Park]

(In reply to comment #21)
> Jae Hyun Park, is there any progress? If you don't mind, I'd like to propose
> to use std::unique_ptr with custom deleter.

Please go ahead. I'm in no condition to work on this for a while.

[Jinwoo Song]

Created attachment 251219 [details]
Patch

[Jinwoo Song]

Martin, Dirk, could you review this patch?

[Dirk Schulze]

Comment on attachment 251219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251219&action=review

r=me with comment.

> LayoutTests/platform/efl/TestExpectations:-221
> -webkit.org/b/130464 fast/canvas/canvas-path-addPath.html

What about the Gtk port? Doesn't it have such a line as well?

[Jinwoo Song]

Comment on attachment 251219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251219&action=review

>> LayoutTests/platform/efl/TestExpectations:-221
>> -webkit.org/b/130464 fast/canvas/canvas-path-addPath.html
> 
> What about the Gtk port? Doesn't it have such a line as well?

Yes, but GTK port disabled this feature as default. GTK guys may update TestExpectations after enable CANVAS_PATH in another patch.

cmake/OptionsGTK.cmake:159:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CANVAS_PATH OFF)

[WebKit Commit Bot]

Comment on attachment 251219 [details]
Patch

Clearing flags on attachment: 251219

Committed r183088: <http://trac.webkit.org/changeset/183088>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.