[webkit-reviews] review denied: [Bug 5966] <image> does not preserveAspectRatio : [Attachment 6779] cleaned up patch

[bugzilla-request-daemon at opendarwin.org]

Maciej Stachowiak <mjs at apple.com> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 5966: <image> does not preserveAspectRatio
http://bugzilla.opendarwin.org/show_bug.cgi?id=5966

Attachment 6779: cleaned up patch
http://bugzilla.opendarwin.org/attachment.cgi?id=6779&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
+	 if (origDestWidth > (origDestHeight / widthToHeightMultiplier)) {
+	     destRect.setWidth(origDestHeight / widthToHeightMultiplier);
+	     switch(aspectRatio->align()) {
+		 case SVG_PRESERVEASPECTRATIO_XMINYMID:
+		 case SVG_PRESERVEASPECTRATIO_XMIDYMID:
+		 case SVG_PRESERVEASPECTRATIO_XMAXYMID:
+		     destRect.setX(origDestWidth / 2 - destRect.width() / 2);
+		     break;
+		 case SVG_PRESERVEASPECTRATIO_XMINYMAX:
+		 case SVG_PRESERVEASPECTRATIO_XMIDYMAX:
+		 case SVG_PRESERVEASPECTRATIO_XMAXYMAX:
+		     destRect.setX(origDestWidth - destRect.width());
+		     break;
+	     }
+	 }

I think this part is wrong, I think it should be grouping the switch blocks by
XMID and XMAX, not YMID and YMAX. Shouldn't it? The other setX branch does
that. Other than this apparent bug, looks fine.

r- for the bug.

I think you could consider refactoring it like Eric said but I don't believe
this is required.