Created attachment 169886 [details] Patch

(In reply to comment #0) > It uses destRect location in calculating positioning within the src rect. > > These two files in the test suite are rendered wrong as a result: > svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html > svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html You want a review for something that makes the rendering wrong? :P Can you be more explicit please? How is it different? How does it differ in comparison to other UAs? Can you share pictures please?

Created attachment 169896 [details] Current rendering

Created attachment 169897 [details] Expected rendering as done by Firefox (ignore the color differences)

Created attachment 169898 [details] Post-patch rendering

Before the patch, the girl's head was being drawn lower than it should be because the location of the wrong rect was used in the code. Moreover, the crop changes with the positioning of the target rectangle (try adding a 'y=100' attribute to the filtered rect: the position of the image with respect to the rectangle will remain the same in Firefox and Opera (correct), but will change in WebKit (wrong)).

I'm still waiting for a review.

Comment on attachment 169886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169886&action=review Sorry, was traveling. The fix sounds good then. But have some comments. > Source/WebCore/ChangeLog:8 > + Corrects SVG aspect ratio calculations in 'slice' mode. Can you add more detail please? What is the problem? What have you done in order to fix the problem? What causes the problem and so on. > Source/WebCore/ChangeLog:13 > + * svg/SVGPreserveAspectRatio.cpp: > + (WebCore::SVGPreserveAspectRatio::transformRect): Inline comments behind functions can be very helpful as well. > LayoutTests/ChangeLog:8 > + [SVG] SVGPreserveAspectRatio::transformRect is done incorrectly in 'slice' mode > + https://bugs.webkit.org/show_bug.cgi?id=99984 > + > + Reviewed by NOBODY (OOPS!). > + > + Corrects SVG aspect ratio calculations in 'slice' mode. I would like to have more tests. It is not good if it is just triggered by SVGFEImage. Can you add a test for <svg> and preserve aspect ratio (hopefully a ref test)? A test with <image> would be preferred as well.

Created attachment 172593 [details] Patch

Comment on attachment 172593 [details] Patch I am unsure if you didn't read my last comment. I would like to have some new tests for <svg> and <image>, since they use SVGPreserveAspectRatio as well.

View in context: https://bugs.webkit.org/attachment.cgi?id=169886&action=review >> Source/WebCore/ChangeLog:8 >> + Corrects SVG aspect ratio calculations in 'slice' mode. > > Can you add more detail please? What is the problem? What have you done in order to fix the problem? What causes the problem and so on. Done. >> Source/WebCore/ChangeLog:13 >> + (WebCore::SVGPreserveAspectRatio::transformRect): > > Inline comments behind functions can be very helpful as well. Done. >> LayoutTests/ChangeLog:8 >> + Corrects SVG aspect ratio calculations in 'slice' mode. > > I would like to have more tests. It is not good if it is just triggered by SVGFEImage. Can you add a test for <svg> and preserve aspect ratio (hopefully a ref test)? A test with <image> would be preferred as well. The offending code is currently only used by SVGFEImage (see http://code.google.com/p/chromium/source/search?q=transformrect). There already exist multiple aspect ratio tests of <img>, css backgroundImage etc., but they do not currently hit this particular method. There's a battery of tests of the functionality as part of the patch for bug 98403, which applies transformRect to a wider range of situations.

(In reply to comment #11) > The offending code is currently only used by SVGFEImage (see http://code.google.com/p/chromium/source/search?q=transformrect). There already exist multiple aspect ratio tests of <img>, css backgroundImage etc., but they do not currently hit this particular method. Obviously none of the tests failed, since you didn't need to rebaseline. Therefore, these tests don't test your functional change. > > There's a battery of tests of the functionality as part of the patch for bug 98403, which applies transformRect to a wider range of situations. If there are tests in bug 98403 that run into this issue, please add them into this patch. Again, I would like to see tests where you run into this issue for the elements <svg> and <image> (not <img>).

Created attachment 172818 [details] Testcase

Created attachment 172820 [details] Patch

<img> and <svg> use SVGPreserveAspectRatio::getCTM and are not affected by this bug. I added an exhaustive test for <feImage>, which tests all aspect ratio choices that can hit SVGPreserveAspectRatio::transformRect.

Comment on attachment 172820 [details] Patch This is the same test as LayoutTests/svg/W3C-SVG-1.1-SE/filters-image-05-f.svg You don't need to upload it again. I wonder why we don't have the same problem in other cases like LayoutTests/svg/W3C-SVG-1.1/coords-viewattr-02-b.svg Which does exactly the same on <image> and does not fail. I would like to know why it doesn't fail before approving this patch. Maybe it is a bug in FEImage instead, maybe SVGImageElement has hacks that need to be removed. Maybe the test is slightly different, in which case we need tests for <image> as well.

Sorry, <img> should have been <image> above.

You do not have this problem in W3C-SVG-1.1-SE/filters-image-05-f.svg because it uses translation to bring the source and destination locations to 0, 0. In that case the old code does not fail. The new tests specifies non-zero destination location and fails.

(In reply to comment #15) > <img> and <svg> use SVGPreserveAspectRatio::getCTM and are not affected by this bug. > > I added an exhaustive test for <feImage>, which tests all aspect ratio choices that can hit SVGPreserveAspectRatio::transformRect. RenderSVGImage uses: SVGImageElement* imageElement = static_cast<SVGImageElement*>(node()); imageElement->preserveAspectRatio().transformRect(destRect, srcRect); So it might be affected as well, no?

(In reply to comment #18) > You do not have this problem in W3C-SVG-1.1-SE/filters-image-05-f.svg because it uses translation to bring the source and destination locations to 0, 0. In that case the old code does not fail. > > The new tests specifies non-zero destination location and fails. This makes sense to me.

Comment on attachment 172820 [details] Patch The new file has no expectation. Can you add one please? Then I can r+ it.

Created attachment 172827 [details] Failing <image> testcase You were right, <image> can fail as well. I could swear chromium code search couldn't find the RenderSVGImage reference a couple of days ago. My mistake. Adding a new test case for <image> and the expected results.

Created attachment 177468 [details] Patch

Comment on attachment 177468 [details] Patch LayoutTests/svg/image-preserveAspectRatio-all.svg should appear in svg/as-image Otherwise this is right. But I'm not an approved reviewer, so someone else will have to do the final R+.

Created attachment 177747 [details] Patch

Comment on attachment 177747 [details] Patch Attachment 177747 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15148543 New failing tests: svg/as-image/image-preserveAspectRatio-all.svg svg/filters/feImage-preserveAspectRatio-all.svg

Comment on attachment 177747 [details] Patch I agree with Dirk that the ChangeLog could be more descriptive, you're telling us what was done but not why. Ideally you'd reference the SVG spec formulas here, or some other source proving our implementation is wrong. That would have sped-up the review process :-) Anyhow, please be careful with not breaking mac EWS, it's red. I'll r+ this, but Dirk or others can feel free to r- it again, if the ChangeLog should be updated for instance.

Created attachment 188976 [details] Rebased patch Yikes! We let a good patch go stale. Branimir is no longer working on this stuff. I've rebased his patch and updated the ChangeLog as was requested by the reviewers.

Attachment 188976 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/svg/as-image/image-preserveAspectRatio-all-expected.png', u'LayoutTests/platform/chromium/svg/as-image/image-preserveAspectRatio-all-expected.txt', u'LayoutTests/platform/chromium/svg/filters/feImage-preserveAspectRatio-all-expected.png', u'LayoutTests/platform/chromium/svg/filters/feImage-preserveAspectRatio-all-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/svg/as-image/image-preserveAspectRatio-all.svg', u'LayoutTests/svg/filters/feImage-preserveAspectRatio-all.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/svg/SVGPreserveAspectRatio.cpp']" exit_code: 1 LayoutTests/platform/chromium/svg/filters/feImage-preserveAspectRatio-all-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 188976 [details] Rebased patch Attachment 188976 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16629206 New failing tests: svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html

Created attachment 188983 [details] Rebased patch (2) SVN1.7 vs SVN1.6 compatibility issues :/ Lets try this again.

Attachment 188983 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/svg/as-image/image-preserveAspectRatio-all-expected.png', u'LayoutTests/svg/as-image/image-preserveAspectRatio-all-expected.txt', u'LayoutTests/svg/as-image/image-preserveAspectRatio-all.svg', u'LayoutTests/svg/filters/feImage-preserveAspectRatio-all-expected.png', u'LayoutTests/svg/filters/feImage-preserveAspectRatio-all-expected.txt', u'LayoutTests/svg/filters/feImage-preserveAspectRatio-all.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/svg/SVGPreserveAspectRatio.cpp']" exit_code: 1 LayoutTests/svg/as-image/image-preserveAspectRatio-all-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 188983 [details] Rebased patch (2) Attachment 188983 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16613513 New failing tests: svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html

Created attachment 189134 [details] Rebased patch (3)

Attachment 189134 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/svg/as-image/image-preserveAspectRatio-all-expected.png', u'LayoutTests/svg/as-image/image-preserveAspectRatio-all-expected.txt', u'LayoutTests/svg/as-image/image-preserveAspectRatio-all.svg', u'LayoutTests/svg/filters/feImage-preserveAspectRatio-all-expected.png', u'LayoutTests/svg/filters/feImage-preserveAspectRatio-all-expected.txt', u'LayoutTests/svg/filters/feImage-preserveAspectRatio-all.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/svg/SVGPreserveAspectRatio.cpp']" exit_code: 1 LayoutTests/svg/as-image/image-preserveAspectRatio-all-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 189134 [details] Rebased patch (3) Marking for review. I think the style checker is wrong to flag this patch (the mimetype property is properly set). I'll file a separate bug for that.

(In reply to comment #36) > (From update of attachment 189134 [details]) > Marking for review. I think the style checker is wrong to flag this patch (the mimetype property is properly set). I'll file a separate bug for that. As promised, bug for the style checker warning: https://bugs.webkit.org/show_bug.cgi?id=110249

Comment on attachment 189134 [details] Rebased patch (3) r=me