Use the viewBox when scaling <svg:image>s non-uniformly w/ pAR=none

Source/core/rendering/svg/RenderSVGImage.cpp

Index: Source/core/rendering/svg/RenderSVGImage.cpp
diff --git a/Source/core/rendering/svg/RenderSVGImage.cpp b/Source/core/rendering/svg/RenderSVGImage.cpp
index 0dd6aa8942b9af18c799e3ea5363c1d2ed64cc8c..b0b1f521740b921213cdba5c4fb55431d3221d4b 100644
--- a/Source/core/rendering/svg/RenderSVGImage.cpp
+++ b/Source/core/rendering/svg/RenderSVGImage.cpp
@@ -61,48 +61,44 @@ void RenderSVGImage::destroy()
     RenderSVGModelObject::destroy();
 }
 
-bool RenderSVGImage::forceNonUniformScaling(SVGImageElement* image) const
+LayoutSize RenderSVGImage::computeNonUniformScalingViewportSize() const
 {
-    // Images with preserveAspectRatio=none should force non-uniform
-    // scaling. This can be achieved by setting the image's container size to
-    // its intrinsic size. If the image does not have an intrinsic size - or
-    // the intrinsic size is degenerate - set the container size to the bounds
-    // as in pAR!=none cases.
-    // See: http://www.w3.org/TR/SVG/single-page.html, 7.8 The ‘preserveAspectRatio’ attribute.
-    if (image->preserveAspectRatio()->currentValue()->align() != SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE)
-        return false;
-    ImageResource* cachedImage = m_imageResource->cachedImage();
-    if (!cachedImage)
-        return false;
-    Length intrinsicWidth;
-    Length intrinsicHeight;
-    FloatSize intrinsicRatio;
-    cachedImage->computeIntrinsicDimensions(intrinsicWidth, intrinsicHeight, intrinsicRatio);
-    if (!intrinsicWidth.isFixed() || !intrinsicHeight.isFixed())
-        return false;
-    // If the viewport defined by the referenced image is zero in either
-    // dimension, then SVGImage will have computed an intrinsic size of 300x150.
-    if (!floatValueForLength(intrinsicWidth, 0) || !floatValueForLength(intrinsicHeight, 0))
-        return false;
-    return true;
+    // Note: This assumes that the 'intrinsic ratio' as returned by

[pdr., 2014/11/12 00:48:25]

Could we document this on the Image interface instead of here?

[fs, 2014/11/12 10:13:44]

Done.

+    // ImageResource::computeIntrinsicDimensions (and indirectly the same
+    // method on Image) is the viewport of the image. This is not documented to
+    // be the case in the Image interface, but is what the current
+    // implementations return.
+    if (ImageResource* cachedImage = m_imageResource->cachedImage()) {
+        Length intrinsicWidth;
+        Length intrinsicHeight;
+        FloatSize intrinsicRatio;
+        cachedImage->computeIntrinsicDimensions(intrinsicWidth, intrinsicHeight, intrinsicRatio);
+        if (!intrinsicRatio.isEmpty())
+            return roundedLayoutSize(intrinsicRatio);
+    }
+    return m_imageResource->intrinsicSize(style()->effectiveZoom());

[pdr., 2014/11/12 00:48:25]

It looks like this will end up just returning an empty rect in
RenderImageResource::getImageSize so we could simplify this as:
if (ImageResource* cachedImage = ...) {
  ...
  return roundedLayoutSize(intrinsicRatio);
}
return empty rect;

[fs, 2014/11/12 10:13:45]

Yes, true. Done.

 }
 
 bool RenderSVGImage::updateImageViewport()
 {
     SVGImageElement* image = toSVGImageElement(element());
     FloatRect oldBoundaries = m_objectBoundingBox;
-    bool updatedViewport = false;
 
     SVGLengthContext lengthContext(image);
     m_objectBoundingBox = FloatRect(image->x()->currentValue()->value(lengthContext), image->y()->currentValue()->value(lengthContext), image->width()->currentValue()->value(lengthContext), image->height()->currentValue()->value(lengthContext));
 
     bool boundsChanged = oldBoundaries != m_objectBoundingBox;
 
+    // Images with preserveAspectRatio=none should force non-uniform
+    // scaling. This can be achieved by setting the image's container size to
+    // its viewport size (i.e. if a viewBox is available - use that - else use intrinsic size.
+    // See: http://www.w3.org/TR/SVG/single-page.html, 7.8 The ‘preserveAspectRatio’ attribute.
     IntSize newViewportSize;
-    if (forceNonUniformScaling(image)) {
-        LayoutSize intrinsicSize = m_imageResource->intrinsicSize(style()->effectiveZoom());
-        if (intrinsicSize != m_imageResource->imageSize(style()->effectiveZoom())) {
-            newViewportSize = roundedIntSize(intrinsicSize);
+    bool updatedViewport = false;
+    if (image->preserveAspectRatio()->currentValue()->align() == SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE) {

[pdr., 2014/11/12 00:48:25]

Could we move the PAR_NONE check and the viewport size comparison into
computeNonUniformScalingViewportSize?

Something like:
if (computeNonUnifornScalingViewportSize(newViewportSize)) {
    updatedViewport = true;
} else {
    ....

[fs, 2014/11/12 10:13:44]

While it's possible to do that, you then get essentially three values to store
in the bool - pAR!=none, viewport size unchanged, update viewport.
To retain the "correct" flow (and set the right viewport in all cases) you'd
need to sacrifice one of them (the middle one). While that's possible, it also
mean we'll be resetting the container size every time. Probably not a big deal
(at least not ATM), but updating the container size too much has bit us before
in these cases (i.e. the initial bug that started this farce...), so I'd rather
tread lightly here (if possible)...

Tangentially, it might make sense to structure the code like:

LayoutSize viewportSize = computeViewportSize(...);
if (viewportSize != m_imageResource->imageSize(...))
  m_imageResource->setContainerSize(roundedIntSize(viewportSize));
m_needs... |= boundsChanged;
return boundsChanged;

i.e. implicitly signal the changes in the 'viewport' / bounds.

+        LayoutSize computedViewportSize = computeNonUniformScalingViewportSize();
+        if (computedViewportSize != m_imageResource->imageSize(style()->effectiveZoom())) {
+            newViewportSize = roundedIntSize(computedViewportSize);
             updatedViewport = true;
         }
     } else if (boundsChanged) {
@@ -112,7 +108,7 @@ bool RenderSVGImage::updateImageViewport()
     if (updatedViewport)
         m_imageResource->setContainerSizeForRenderer(newViewportSize);
     m_needsBoundariesUpdate |= boundsChanged;
-    return updatedViewport;
+    return updatedViewport || boundsChanged;

[pdr., 2014/11/12 00:48:25]

Does it make sense for the bounds to change but the viewport not update?

[fs, 2014/11/12 10:13:45]

Yes, this would even be expected in the pAR=none case (where the viewport
depends on the content rather then the container). As I alluded to above though,
this could probably just be 'return boundsChanged', because 'updatedViewport'
implies 'boundsChanged' - at least in all context where the return-value is
checked (and likely in the other two as well.)

 }
 
 void RenderSVGImage::layout()