Bug 20578 – [CAIRO] pattern of a canvas-element changes after modifications on canvas-element

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Schulze]

If you create a pattern of a canvas-element

var pattern = ctx.createPattern(canvas, 'no-repeat');

and modify the canvas after that, the pattern is changed too.

[Shinichiro Hamaji]

Created attachment 31714 [details]
Patch v1

 LayoutTests/ChangeLog                              |   18 +++++++
 .../fast/canvas/canvas-pattern-modify-expected.txt |   13 +++++
 LayoutTests/fast/canvas/canvas-pattern-modify.html |    6 ++
 LayoutTests/fast/canvas/canvas-pattern-modify.js   |   52 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   15 ++++++
 .../platform/graphics/cairo/ImageBufferCairo.cpp   |   19 +++++++-
 6 files changed, 122 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 31714 [details]
Patch v1

Isn't there an easier/cleaner way to copy an image in Cairo? 

Is memcpy the right allocator to use?

[Shinichiro Hamaji]

Created attachment 31764 [details]
Patch v2

 LayoutTests/ChangeLog                              |   18 +++++++
 .../fast/canvas/canvas-pattern-modify-expected.txt |   13 +++++
 LayoutTests/fast/canvas/canvas-pattern-modify.html |    6 ++
 LayoutTests/fast/canvas/canvas-pattern-modify.js   |   52 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   15 ++++++
 .../platform/graphics/cairo/ImageBufferCairo.cpp   |   22 ++++++++-
 6 files changed, 125 insertions(+), 1 deletions(-)

[Shinichiro Hamaji]

(In reply to comment #2)
> (From update of attachment 31714 [details] [review])
> Isn't there an easier/cleaner way to copy an image in Cairo? 

I searched cairo API and web, but couldn't find an convenient API for this. In the following discussion, a person is saying that "cairo isn't providing a "copy surface" primitive".

http://lists.cairographics.org/archives/cairo/2007-June/010877.html

> Is memcpy the right allocator to use?

Ah, I think using cairo_paint would be better as it would work even if the source surface has different internal representation. I've uploaded another patch which changes this. Thanks.

[Eric Seidel (no email)]

Comment on attachment 31764 [details]
Patch v2

I think this should be broken into a "copySurface" static inline.  That function should have a comment pointing to the discussion about how cairo is lacking this call.

Otherwise this looks great.

I think it would be nice to fix the one nit about breaking this out into its own function with a comment about how cairo doesn't provide this natively.  Eventually cairo is likely to provide this, and when it does, we can replace your copySurface function with a call to the native cairo call.

[Shinichiro Hamaji]

Created attachment 31926 [details]
Patch v3

[Shinichiro Hamaji]

Comment on attachment 31926 [details]
Patch v3

Thanks for the review! I've created a new static inline function with some comments.

[Shinichiro Hamaji]

Created attachment 32550 [details]
Patch v4

Fixing a style error for copySurface. Though Eric kindly said that he'll fix this, I fixed it to save Eric's time a bit.

[Oliver Hunt]

Comment on attachment 32550 [details]
Patch v4

r=me

[Adam Barth]

Will land.

[Adam Barth]

Committed r45989