|Summary:||Fix a bug in pattern transformation in PatternSkia|
|Product:||WebKit||Reporter:||Hin-Chung Lam <hclam>|
|Component:||Platform||Assignee:||Stephen White <senorblanco>|
|Severity:||Normal||CC:||brettw, commit-queue, dglazkov, krit, senorblanco, webkit.review.bot|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Hin-Chung Lam 2009-03-11 18:58:52 PDT
PatternSkia should respect the local transformation in createPlatformPattern and apply the local transformation to shader. This patch fixes the following layout tests in chromium: LayoutTests/svg/batik/paints/patternRegionA.svg LayoutTests/svg/W3C-SVG-1.1/pservers-grad-06-b.svg LayoutTests/svg/W3C-SVG-1.1/pservers-pattern-01-b.svg LayoutTests/svg/W3C-SVG-1.1/coords-units-01-b.svg Note that Skia is bad about having positive translation in tile mode. The code takes special care of that. See the code for more details. Review in chromium is here: http://codereview.chromium.org/42100/show
Comment 1 Hin-Chung Lam 2009-03-11 18:59:41 PDT
Created attachment 28509 [details] Patch to patternSkia.cpp
Comment 2 Hin-Chung Lam 2009-03-11 19:16:42 PDT
Created attachment 28511 [details] patch + change log
Comment 3 Dimitri Glazkov (Google) 2009-03-18 11:14:30 PDT
Brett, can you look at this change?
Comment 4 Brett Wilson (Google) 2009-03-18 11:20:19 PDT
I would like to let the Skia folks know about this before we work around something. If it is a bug, it should be fixed there, or we should know a fix is pending before checking in. I'll forward it.
Comment 5 Hin-Chung Lam 2009-03-24 13:08:13 PDT
(In reply to comment #4) > I would like to let the Skia folks know about this before we work around > something. If it is a bug, it should be fixed there, or we should know a fix is > pending before checking in. I'll forward it. > Any updates from skia people?
Comment 6 Dirk Schulze 2009-03-24 13:51:50 PDT
There is maybe another way to support transformations for patterns. On CG and Qt we transform the context right before we stroke/fill anything with a pattern, the path should not be affected. Just look at fillPath/strokePath in GraphicsContextCg.cpp or GraphicsContextQt.cpp to know what I mean. Could that avoid this hack at the end of the pattern-patch? To round and cut off some values seems to be incorrect for me.
Comment 7 Eric Seidel (no email) 2009-05-19 20:37:44 PDT
Comment on attachment 28511 [details] patch + change log r- based on Dirk's comments. Mostly I can't quite tell if this is right, Dirk is questioning the correctness, and we're trying to clear out the review queue one. Feel free to mark this r=? with answers to Dirk's questions.
Comment 8 Dirk Schulze 2009-11-14 00:31:10 PST
No progress on this bug? :-(
Comment 9 Stephen White 2010-01-28 12:52:45 PST
Created attachment 47643 [details] New fix The good news is, the Skia bug should be fixed, so the hack part of Alpha's fix (above) should no longer be necessary. The bad news is, I changed the allocation of SkShaders a few months back to be done once on first draw, so it was necessary to rework Patterns a bit so that the m_patternSpaceTransform could be forwarded to the SkShader.
Comment 10 Stephen White 2010-01-28 13:17:16 PST
Created attachment 47644 [details] ChangeLog and non-Skia fixes
Comment 11 WebKit Review Bot 2010-01-28 13:56:08 PST
Attachment 47644 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/217145
Comment 12 Stephen White 2010-01-28 14:58:26 PST
Created attachment 47652 [details] Same as above, plus QT fix
Comment 13 Eric Seidel (no email) 2010-02-01 15:14:05 PST
Comment on attachment 47652 [details] Same as above, plus QT fix I would have probably put the "else" into its own function, but OK.
Comment 14 WebKit Commit Bot 2010-02-01 21:36:48 PST
Comment on attachment 47652 [details] Same as above, plus QT fix Clearing flags on attachment: 47652 Committed r54203: <http://trac.webkit.org/changeset/54203>
Comment 15 WebKit Commit Bot 2010-02-01 21:36:58 PST
All reviewed patches have been landed. Closing bug.