[webkit-reviews] review denied: [Bug 99984] [SVG] SVGPreserveAspectRatio::transformRect is done incorrectly in 'slice' mode : [Attachment 169886] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Branimir Lambov
<blambov at google.com>'s request for review:
Bug 99984: [SVG] SVGPreserveAspectRatio::transformRect is done incorrectly in
'slice' mode
https://bugs.webkit.org/show_bug.cgi?id=99984

Attachment 169886: Patch
https://bugs.webkit.org/attachment.cgi?id=169886&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.