| Summary: | [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | blambov, dglazkov, enne, jamesr, krit, morrita, rolandsteiner, schenney, webkit.review.bot, zimmermann | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 44514 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ryosuke Niwa
2011-01-29 12:33:27 PST
Similar issues in svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg svg/clip-path/deep-nested-clip-in-mask-panning.svg svg/clip-path/deep-nested-clip-in-mask.svg svg/clip-path/nested-clip-in-mask-image-based-clipping.svg svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg svg/clip-path/nested-clip-in-mask-path-based-clipping.svg The reason is most likely the implementation of GraphicsContext::clipToImageBuffer for Skia. Created attachment 118212 [details]
Patch
Comment on attachment 118212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118212&action=review > Source/WebCore/ChangeLog:9 > + [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux > + https://bugs.webkit.org/show_bug.cgi?id=53378 > + > + Reviewed by NOBODY (OOPS!). These two should appear before the description. > LayoutTests/ChangeLog:9 > + [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux > + https://bugs.webkit.org/show_bug.cgi?id=53378 > + > + Reviewed by NOBODY (OOPS!). Ditto. Comment on attachment 118212 [details] Patch Attachment 118212 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10752238 New failing tests: svg/clip-path/nested-clip-in-mask-path-based-clipping.svg svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg svg/clip-path/deep-nested-clip-in-mask-panning.svg svg/clip-path/nested-clip-in-mask-image-based-clipping.svg svg/clip-path/clip-in-mask.svg svg/batik/text/textProperties.svg svg/W3C-SVG-1.1/filters-example-01-b.svg svg/clip-path/deep-nested-clip-in-mask.svg svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg Created attachment 118402 [details]
Patch
(In reply to comment #4) > (From update of attachment 118212 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118212&action=review > > > Source/WebCore/ChangeLog:9 > > + [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux > > + https://bugs.webkit.org/show_bug.cgi?id=53378 > > + > > + Reviewed by NOBODY (OOPS!). > > These two should appear before the description. > Done. > > LayoutTests/ChangeLog:9 > > + [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux > > + https://bugs.webkit.org/show_bug.cgi?id=53378 > > + > > + Reviewed by NOBODY (OOPS!). > > Ditto. Done. (In reply to comment #5) > (From update of attachment 118212 [details]) > Attachment 118212 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10752238 > > New failing tests: > svg/clip-path/nested-clip-in-mask-path-based-clipping.svg > svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg > svg/clip-path/deep-nested-clip-in-mask-panning.svg > svg/clip-path/nested-clip-in-mask-image-based-clipping.svg > svg/clip-path/clip-in-mask.svg > svg/batik/text/textProperties.svg > svg/W3C-SVG-1.1/filters-example-01-b.svg > svg/clip-path/deep-nested-clip-in-mask.svg > svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg I intend to run webkit-patch rebaseline on these tests after the patch is landed. (In reply to comment #8) > (In reply to comment #5) > > (From update of attachment 118212 [details] [details]) > > Attachment 118212 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/10752238 > > > > New failing tests: > > svg/clip-path/nested-clip-in-mask-path-based-clipping.svg > > svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg > > svg/clip-path/deep-nested-clip-in-mask-panning.svg > > svg/clip-path/nested-clip-in-mask-image-based-clipping.svg > > svg/clip-path/clip-in-mask.svg > > svg/batik/text/textProperties.svg > > svg/W3C-SVG-1.1/filters-example-01-b.svg > > svg/clip-path/deep-nested-clip-in-mask.svg > > svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg > > I intend to run webkit-patch rebaseline on these tests after the patch is landed. You need to mark these as expected failures in test_expectations.txt in that case. (In reply to comment #9) > You need to mark these as expected failures in test_expectations.txt in that case. I think it's okay not to include it in the patch since doing so will likely cause a patch conflict. I disagree, if you put the new lines somewhere other than the very end of test_expectations.txt the odds of a patch conflict are low. Landing without the test_expectations.txt is just giving the gardener an avoidable red tree and creating a lot of extra work. (In reply to comment #11) > I disagree, if you put the new lines somewhere other than the very end of test_expectations.txt the odds of a patch conflict are low. Landing without the test_expectations.txt is just giving the gardener an avoidable red tree and creating a lot of extra work. The author can land & add expectations in one patch when he/she lands a patch. In my experience, adding per-file expectation for test_expectations.txt in a patch posted on Bugzilla causes nothing but trouble because there is a big delay between the time you post the patch on Bugzilla and it gets landed. It's much better to add lines before landing the patch after the patch has been reviewed. Comment on attachment 118402 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118402&action=review > LayoutTests/platform/chromium/test_expectations.txt:-842 > -// New in WK r65880, some probably just need rebaselining, others have real diffs > -BUGWK53378 LINUX WIN : svg/clip-path/clip-in-mask.svg = IMAGE > -BUGWK53378 LINUX WIN : svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg = IMAGE+TEXT > -BUGWK53378 LINUX WIN : svg/clip-path/deep-nested-clip-in-mask-panning.svg = IMAGE+TEXT > -BUGWK53378 LINUX WIN : svg/clip-path/deep-nested-clip-in-mask.svg = IMAGE+TEXT > -BUGWK53378 LINUX WIN : svg/clip-path/nested-clip-in-mask-image-based-clipping.svg = IMAGE > -BUGWK53378 LINUX WIN : svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg = IMAGE > -BUGWK53378 LINUX WIN : svg/clip-path/nested-clip-in-mask-path-based-clipping.svg = IMAGE > -// Rebaselined in WK r65874, Linux has missing lines > - Having said that, it seems bad to remove these lines pre-emptively. You should land this patch and then remove these lines as you confirm your patch fixed the bug. Created attachment 118554 [details]
Patch
Comment on attachment 118554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118554&action=review > Source/WebCore/ChangeLog:9 > + Fixes a problem in applying clip layers on Skia that was causing > + all complex clipping (including text and/or masks) to fail. Can you explain what the bug was and how this fixes it? I don't know this code very well and it's hard for me to tell if the new code is correct. Comment on attachment 118554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118554&action=review >> Source/WebCore/ChangeLog:9 >> + all complex clipping (including text and/or masks) to fail. > > Can you explain what the bug was and how this fixes it? I don't know this code very well and it's hard for me to tell if the new code is correct. The bug was in Skia's clipping code's handling of coordinate transformations. The method beginLayerClippedToImage was taking rectangle coordinates in one local coordinate space, but it was applying them in a different one because of the delay between the time it is called and the actual application occurs in applyClipFromImage. The fix translates the coordinates passed to beginLayerClippedToImage to absolute ones, so that they are not affected by any change in the transform matrix, and makes sure that applyClipFromImage clears the matrix before applying the clip layer to correctly apply the absolute coordinates. Comment on attachment 118554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118554&action=review >>> Source/WebCore/ChangeLog:9 >>> + all complex clipping (including text and/or masks) to fail. >> >> Can you explain what the bug was and how this fixes it? I don't know this code very well and it's hard for me to tell if the new code is correct. > > The bug was in Skia's clipping code's handling of coordinate transformations. The method beginLayerClippedToImage was taking rectangle coordinates in one local coordinate space, but it was applying them in a different one because of the delay between the time it is called and the actual application occurs in applyClipFromImage. The fix translates the coordinates passed to beginLayerClippedToImage to absolute ones, so that they are not affected by any change in the transform matrix, and makes sure that applyClipFromImage clears the matrix before applying the clip layer to correctly apply the absolute coordinates. Please include this in the ChangeLog, otherwise, this patch seems fine. Created attachment 118641 [details]
Patch
Comment on attachment 118641 [details] Patch Clearing flags on attachment: 118641 Committed r103091: <http://trac.webkit.org/changeset/103091> All reviewed patches have been landed. Closing bug. Committed r103099: <http://trac.webkit.org/changeset/103099> Committed r103111: <http://trac.webkit.org/changeset/103111> Committed r114407: <http://trac.webkit.org/changeset/114407> |