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

Source/core/rendering/svg/RenderSVGImage.cpp

 /*
  * Copyright (C) 2006 Alexander Kellett <lypanov@kde.org>
  * Copyright (C) 2006 Apple Computer, Inc.
  * Copyright (C) 2007 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2007, 2008, 2009 Rob Buis <buis@kde.org>
  * Copyright (C) 2009 Google, Inc.
  * Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
  * Copyright (C) 2010 Patrick Gansterer <paroga@paroga.com>
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 19 matching lines @@
 #include "core/rendering/GraphicsContextAnnotator.h"
 #include "core/rendering/ImageQualityController.h"
 #include "core/rendering/PointerEventsHitRules.h"
 #include "core/rendering/RenderImageResource.h"
 #include "core/rendering/svg/RenderSVGResource.h"
 #include "core/rendering/svg/SVGRenderSupport.h"
 #include "core/rendering/svg/SVGRenderingContext.h"
 #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 {
 
 RenderSVGImage::RenderSVGImage(SVGImageElement* impl)
     : RenderSVGModelObject(impl)
     , m_needsBoundariesUpdate(true)
     , m_needsTransformUpdate(true)
     , m_imageResource(RenderImageResource::create())
 {
     m_imageResource->initialize(this);
 }
 
 RenderSVGImage::~RenderSVGImage()
 {
 }
 
 void RenderSVGImage::destroy()
 {
     ImageQualityController::remove(this);
     m_imageResource->shutdown();
     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());
     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 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);
             updatedViewport = true;
         }
     } else if (boundsChanged) {
         newViewportSize = enclosingIntRect(m_objectBoundingBox).size();
         updatedViewport = true;
     }
     if (updatedViewport)
@@ skipping 139 matching lines @@
 
 void RenderSVGImage::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&, const RenderLayerModelObject*) const
 {
     // this is called from paint() after the localTransform has already been applied
     IntRect contentRect = enclosingIntRect(paintInvalidationRectInLocalCoordinates());
     if (!contentRect.isEmpty())
         rects.append(contentRect);
 }
 
 } // namespace blink