This test is using the different background repeat modes: repeat, repeat-x, repeat-y and no-repeat. The problem is that cairo doesn't have a extend mode for repeat-x/y and we are always using EXTEND_REPEAT. We could use EXTEND_NONE for repeat-x/y and paint the pattern multiple times ourselves, or we could try to clip the row/column we want to draw.
Committed r76593: <http://trac.webkit.org/changeset/76593>
Reopening, webkit-patch fixed it when I committed r76593
The following tests were found to be passing consistently after moving from using the Skipped list to using test_expectations.txt: fast/canvas/patternfill-repeat.html Their expectations were removed in r116122[1] (covered by bug #85591). Tests that are covered by this bug and are still failing are the following: canvas/philip/tests/2d.pattern.paint.repeatx.coord1.html canvas/philip/tests/2d.pattern.paint.repeatx.outside.html canvas/philip/tests/2d.pattern.paint.repeaty.coord1.html canvas/philip/tests/2d.pattern.paint.repeaty.outside.html 1: http://trac.webkit.org/changeset/116122
Created attachment 147780 [details] Clipping repeats.
Comment on attachment 147780 [details] Clipping repeats. View in context: https://bugs.webkit.org/attachment.cgi?id=147780&action=review > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:305 > + cairo_append_path(m_cr.get(), currentPath); > +} I think you are leaking the path here, you should call cairo_path_destroy(), or even better you could use OwnPtr.
Created attachment 147794 [details] Clipping repeats, v2, OwnPtr for path.
Comment on attachment 147794 [details] Clipping repeats, v2, OwnPtr for path. Thanks for the patch, it looks good to me in general, but I prefer that martin or alex review it since they know this code much better than me.
Comment on attachment 147794 [details] Clipping repeats, v2, OwnPtr for path. View in context: https://bugs.webkit.org/attachment.cgi?id=147794&action=review Great work, just a few comments. > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:277 > + int patternWidth = patternImage->width(); > + int patternHeight = patternImage->height(); There's no need to cache these values, you only use them once. > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:280 > + bool repeatX = state.fillPattern->repeatX(); > + bool repeatY = state.fillPattern->repeatY(); I would recommend moving these and patternImage down to right before you use them. > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:283 > + OwnPtr<cairo_path_t> currentPath = adoptPtr(cairo_copy_path(m_cr.get())); > + cairo_new_path(m_cr.get()); Probably should drop a comment here that says something like "We need to preserve any pre-existing paths." > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:285 > + FloatRect clipRect = targetRect; Why not just start with the cairo_clip_extents?
Created attachment 148081 [details] Clipping repeats, v3
Thanks for the review and positive feedback, Martin. Here's an updated patch. (In reply to comment #8) > (From update of attachment 147794 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147794&action=review > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:277 > > + int patternWidth = patternImage->width(); > > + int patternHeight = patternImage->height(); > > There's no need to cache these values, you only use them once. Moved down into where they are used. > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:280 > > + bool repeatX = state.fillPattern->repeatX(); > > + bool repeatY = state.fillPattern->repeatY(); > > I would recommend moving these and patternImage down to right before you use them. Done. > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:283 > > + OwnPtr<cairo_path_t> currentPath = adoptPtr(cairo_copy_path(m_cr.get())); > > + cairo_new_path(m_cr.get()); > > Probably should drop a comment here that says something like "We need to preserve any pre-existing paths." Comment on saving and restoring added. > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:285 > > + FloatRect clipRect = targetRect; > > Why not just start with the cairo_clip_extents? Thanks - Yes, much better, very useful suggestion. I hope the double to float conversion there is alright for all practical purposes.
Comment on attachment 148081 [details] Clipping repeats, v3 Clearing flags on attachment: 148081 Committed r120598: <http://trac.webkit.org/changeset/120598>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 89359
Reopening, commit is causing crashes on GTK and EFL release builders: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/25424 http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release/builds/1502 Rolling out in bug #89359.
Created attachment 148302 [details] Clipping repeats, v4, OwnPtrCairo free fix.
(In reply to comment #15) > Created an attachment (id=148302) [details] > Clipping repeats, v4, OwnPtrCairo free fix. #include "OwnPtrCairo.h" was missing from PlatformContextCairo.cpp - leading to the wrong free function being called. Zan or Martin - could I ask you to re-land? Thanks.
Comment on attachment 148302 [details] Clipping repeats, v4, OwnPtrCairo free fix. Sorry, I should have caught that in my previous review.
(In reply to comment #17) > (From update of attachment 148302 [details]) > Sorry, I should have caught that in my previous review. No problem, thanks for the r+ - I tested this locally on EFL and GTK Debug builds - and I guess this was one of the few cases where due to WTF::fastFree the badness happens only in Release.
Comment on attachment 148302 [details] Clipping repeats, v4, OwnPtrCairo free fix. Clearing flags on attachment: 148302 Committed r120755: <http://trac.webkit.org/changeset/120755>