Use correct viewport for intrinsic size-less SVG images in <svg:image>

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 842319d72e3da3cdb116fbfda47fb4e1c7062ba4..a9cf83413a48fd51fd58e01637956ed73d9a38a0 100644
--- a/Source/core/rendering/svg/RenderSVGImage.cpp
+++ b/Source/core/rendering/svg/RenderSVGImage.cpp
@@ -37,6 +37,7 @@
 #include "core/rendering/svg/SVGResources.h"
 #include "core/rendering/svg/SVGResourcesCache.h"
 #include "core/svg/SVGImageElement.h"
+#include "platform/LengthFunctions.h"
 #include "platform/graphics/GraphicsContextStateSaver.h"
 
 namespace blink {
@@ -61,6 +62,21 @@ void RenderSVGImage::destroy()
     RenderSVGModelObject::destroy();
 }
 
+static bool hasIntrinsicSize(ImageResource* imageResource)
+{
+    if (!imageResource)
+        return false;
+    Length intrinsicWidth;
+    Length intrinsicHeight;
+    FloatSize intrinsicRatio;
+    imageResource->computeIntrinsicDimensions(intrinsicWidth, intrinsicHeight, intrinsicRatio);
+    if (!intrinsicWidth.isFixed() || !intrinsicHeight.isFixed())
+        return false;
+    if (!floatValueForLength(intrinsicWidth, 0) || !floatValueForLength(intrinsicHeight, 0))

[pdr., 2014/09/10 03:21:48]

Are these checks correct when the user does want a width of 0? Do we need them
in SVGSVGElement::hasIntrinsicWidth too?

[fs, 2014/09/10 09:31:00]

This could be considered a "counter-weight" to how SVGImage computes its size -
in this case you'll get the fallback intrinsic size of 300x150. I added an
additional test for a case like that.

In a slightly wider scope of things, the intrinsic size of the referenced image
doesn't really matter, because width/height/etc. on the <image> ends up
overriding whatever's on the the image root [1]. I.e. embedder really decides in
these cases. But since what we're really doing here is matching the impedance of
(SVG)Image and RenderSVGImage, the former ("avoid 300x150") is what we are
interested in here [2].

[1] "When an ‘image’ element references an SVG image, the ‘clip’ and ‘overflow’
properties on the root element in the referenced SVG image are ignored (in the
same manner as the ‘x’, ‘y’, ‘width’ and ‘height’ attributes are ignored).",
http://dev.w3.org/SVG/profiles/1.1F2/publish/struct.html#ImageElement fourth
paragraph
[2] Yes, I think this is hairy.

+        return false;
+    return true;
+}
+
 bool RenderSVGImage::updateImageViewport()
 {
     SVGImageElement* image = toSVGImageElement(element());
@@ -72,11 +88,14 @@ bool RenderSVGImage::updateImageViewport()
 
     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 intrinsic size.
+    // 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, 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.
     IntSize newViewportSize;
-    if (image->preserveAspectRatio()->currentValue()->align() == SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE) {
+    if (image->preserveAspectRatio()->currentValue()->align() == SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE

[pdr., 2014/09/10 03:21:48]

The code change here looks good but I think we can simplify the logic a bit.

What about something like:
if (forceNonUniformScaling(image)) {
    // etc
} else if (boundsChanged) {
...

forceNonUniformScaling could then encapsulate this somewhat tricky logic and the
comment above this.

[fs, 2014/09/10 09:31:00]

Certainly looks better. Done.

+        && hasIntrinsicSize(m_imageResource->cachedImage())) {
         LayoutSize intrinsicSize = m_imageResource->intrinsicSize(style()->effectiveZoom());
         if (intrinsicSize != m_imageResource->imageSize(style()->effectiveZoom())) {
             newViewportSize = roundedIntSize(intrinsicSize);