Summary: | [CAIRO] pattern of a canvas-element changes after modifications on canvas-element | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||
Component: | New Bugs | Assignee: | 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
Dirk Schulze
2008-08-31 11:30:01 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 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?
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(-)
(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 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.
Created attachment 31926 [details]
Patch v3
Comment on attachment 31926 [details]
Patch v3
Thanks for the review! I've created a new static inline function with some comments.
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 on attachment 32550 [details]
Patch v4
r=me
Will land. Committed r45989 |