Summary: | [cairo] Fix LayoutTests/fast/canvas/patternfill-repeat.html | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||
Component: | WebKitGTK | Assignee: | Dominik Röttsches (drott) <d-r> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alex, d-r, gustavo, kenneth, leandro, mrobinson, rakuco, tonikitoo, webkit.review.bot, zan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | 89359 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2011-01-25 07:12:00 PST
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> All reviewed patches have been landed. Closing bug. |