Bug 53085 – [cairo] Fix LayoutTests/fast/canvas/patternfill-repeat.html

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Committed r76593: <http://trac.webkit.org/changeset/76593>

[Carlos Garcia Campos]

Reopening, webkit-patch fixed it when I committed r76593

[Zan Dobersek]

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

[Dominik Röttsches (drott)]

Created attachment 147780 [details]
Clipping repeats.

[Carlos Garcia Campos]

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.

[Dominik Röttsches (drott)]

Created attachment 147794 [details]
Clipping repeats, v2, OwnPtr for path.

[Carlos Garcia Campos]

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.

[Martin Robinson]

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?

[Dominik Röttsches (drott)]

Created attachment 148081 [details]
Clipping repeats, v3

[Dominik Röttsches (drott)]

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.

[WebKit Review Bot]

Comment on attachment 148081 [details]
Clipping repeats, v3

Clearing flags on attachment: 148081

Committed r120598: <http://trac.webkit.org/changeset/120598>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89359

[Zan Dobersek]

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.

[Dominik Röttsches (drott)]

Created attachment 148302 [details]
Clipping repeats, v4, OwnPtrCairo free fix.

[Dominik Röttsches (drott)]

(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.

[Martin Robinson]

Comment on attachment 148302 [details]
Clipping repeats, v4, OwnPtrCairo free fix.

Sorry, I should have caught that in my previous review.

[Dominik Röttsches (drott)]

(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.

[WebKit Review Bot]

Comment on attachment 148302 [details]
Clipping repeats, v4, OwnPtrCairo free fix.

Clearing flags on attachment: 148302

Committed r120755: <http://trac.webkit.org/changeset/120755>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.