Add support for addPath method for platforms using cairo.
Created attachment 227415 [details] Patch
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.
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) };
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.
Created attachment 227610 [details] Patch
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.
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.
Created attachment 227637 [details] Patch
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.
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?
(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.
Created attachment 227708 [details] Patch
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.
Created attachment 227717 [details] Patch
(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!
*** Bug 130464 has been marked as a duplicate of this bug. ***
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.
(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.
Comment on attachment 227717 [details] Patch r=me
(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?
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.
(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.
Created attachment 251219 [details] Patch
Martin, Dirk, could you review this patch?
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?
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)
Comment on attachment 251219 [details] Patch Clearing flags on attachment: 251219 Committed r183088: <http://trac.webkit.org/changeset/183088>
All reviewed patches have been landed. Closing bug.