Bug 20578

Summary: [CAIRO] pattern of a canvas-element changes after modifications on canvas-element
Product: WebKit Reporter: Dirk Schulze <krit>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, hamaji, jmalonzo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
URL: http://philip.html5.org/tests/canvas/suite/tests/2d.pattern.modify.canvas1.html
Attachments:
Description Flags
Patch v1
none
Patch v2
eric: review-
Patch v3
none
Patch v4 oliver: review+

Description Dirk Schulze 2008-08-31 11:30:01 PDT
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.
Comment 1 Shinichiro Hamaji 2009-06-23 02:10:41 PDT
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(-)
Comment 2 Eric Seidel (no email) 2009-06-23 17:42:52 PDT
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?
Comment 3 Shinichiro Hamaji 2009-06-23 18:33:41 PDT
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(-)
Comment 4 Shinichiro Hamaji 2009-06-23 18:41:00 PDT
(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.
Comment 5 Eric Seidel (no email) 2009-06-26 03:25:46 PDT
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.
Comment 6 Shinichiro Hamaji 2009-06-26 04:29:29 PDT
Created attachment 31926 [details]
Patch v3
Comment 7 Shinichiro Hamaji 2009-06-26 04:30:21 PDT
Comment on attachment 31926 [details]
Patch v3

Thanks for the review! I've created a new static inline function with some comments.
Comment 8 Shinichiro Hamaji 2009-07-09 22:45:13 PDT
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.
Comment 9 Oliver Hunt 2009-07-16 00:02:55 PDT
Comment on attachment 32550 [details]
Patch v4

r=me
Comment 10 Adam Barth 2009-07-16 15:07:21 PDT
Will land.
Comment 11 Adam Barth 2009-07-16 15:28:02 PDT
Committed r45989