Cleanups to viewport-handling and image invalidation in LayoutSVGImage

Source/core/layout/svg/LayoutSVGImage.cpp

Index: Source/core/layout/svg/LayoutSVGImage.cpp
diff --git a/Source/core/layout/svg/LayoutSVGImage.cpp b/Source/core/layout/svg/LayoutSVGImage.cpp
index 90cdeee1df37ea5dca5368f51d0000ce08474924..9f22f8941089a792bb0b373a2068e9ff657b71e2 100644
--- a/Source/core/layout/svg/LayoutSVGImage.cpp
+++ b/Source/core/layout/svg/LayoutSVGImage.cpp
@@ -88,32 +88,29 @@ static bool containerSizeIsSetForLayoutObject(ImageResource& cachedImage, const
     return !image->isSVGImage() || image != cachedImage.imageForLayoutObject(layoutObject);
 }
 
-bool LayoutSVGImage::updateImageViewport()
+void LayoutSVGImage::updateImageContainerSize()
+{
+    ImageResource* cachedImage = m_imageResource->cachedImage();
+    if (!cachedImage || !cachedImage->usesImageContainerSize())
+        return;
+    FloatSize imageViewportSize = computeImageViewportSize(*cachedImage);
+    if (LayoutSize(imageViewportSize) != m_imageResource->imageSize(styleRef().effectiveZoom())
+        || !containerSizeIsSetForLayoutObject(*cachedImage, this)) {
+        m_imageResource->setContainerSizeForLayoutObject(roundedIntSize(imageViewportSize));
+    }
+}
+
+void LayoutSVGImage::updateBoundingBox()
 {
-    SVGImageElement* image = toSVGImageElement(element());
     FloatRect oldBoundaries = m_objectBoundingBox;
 
-    SVGLengthContext lengthContext(image);
+    SVGLengthContext lengthContext(element());
     m_objectBoundingBox = FloatRect(
         lengthContext.valueForLength(styleRef().svgStyle().x(), styleRef(), SVGLengthMode::Width),
         lengthContext.valueForLength(styleRef().svgStyle().y(), styleRef(), SVGLengthMode::Height),
         lengthContext.valueForLength(styleRef().width(), styleRef(), SVGLengthMode::Width),
         lengthContext.valueForLength(styleRef().height(), styleRef(), SVGLengthMode::Height));
-    bool boundsChanged = oldBoundaries != m_objectBoundingBox;
-
-    bool updatedViewport = false;
-    ImageResource* cachedImage = m_imageResource->cachedImage();
-    if (cachedImage && cachedImage->usesImageContainerSize()) {
-        FloatSize imageViewportSize = computeImageViewportSize(*cachedImage);
-        if (LayoutSize(imageViewportSize) != m_imageResource->imageSize(styleRef().effectiveZoom())
-            || !containerSizeIsSetForLayoutObject(*cachedImage, this)) {
-            m_imageResource->setContainerSizeForLayoutObject(roundedIntSize(imageViewportSize));
-            updatedViewport = true;
-        }
-    }
-
-    m_needsBoundariesUpdate |= boundsChanged;
-    return updatedViewport || boundsChanged;
+    m_needsBoundariesUpdate |= oldBoundaries != m_objectBoundingBox;
 }
 
 void LayoutSVGImage::layout()
@@ -121,7 +118,8 @@ void LayoutSVGImage::layout()
     ASSERT(needsLayout());
     LayoutAnalyzer::Scope analyzer(*this);
 
-    updateImageViewport();
+    updateBoundingBox();
+    updateImageContainerSize();
 
     bool transformOrBoundariesUpdate = m_needsTransformUpdate || m_needsBoundariesUpdate;
     if (m_needsTransformUpdate) {
@@ -180,18 +178,14 @@ bool LayoutSVGImage::nodeAtFloatPoint(HitTestResult& result, const FloatPoint& p
 
 void LayoutSVGImage::imageChanged(WrappedImagePtr, const IntRect*)
 {
-    // The image resource defaults to nullImage until the resource arrives.
-    // This empty image may be cached by SVG resources which must be invalidated.
-    if (SVGResources* resources = SVGResourcesCache::cachedResourcesForLayoutObject(this))
-        resources->removeClientFromCache(this);
-
-    // Eventually notify parent resources, that we've changed.
+    // Notify parent resources that we've changed. This also invalidates
+    // references from resources (filters) that may have a cached
+    // representation of this image/layout object.
     LayoutSVGResourceContainer::markForLayoutAndParentResourceInvalidation(this, false);
 
     // Update the SVGImageCache sizeAndScales entry in case image loading finished after layout.
     // (https://bugs.webkit.org/show_bug.cgi?id=99489)
-    m_objectBoundingBox = FloatRect();
-    updateImageViewport();
+    updateImageContainerSize();

[f(malita), 2015/07/01 16:39:34]

If we're no longer updating the bbox here, is it possible to leave the
LayoutSVGImage in an inconsistent state (what happens at paint time)?

(based on that comment, it sounds like we're already in dangerous territory here
- wondering if this makes it worse)

[fs, 2015/07/01 16:58:13]

The image (dimensions) does not affect the viewport (== the bbox) in which we
paint it. When the above comment was added, the artificial bounds change was
required to trigger the additional logic to update the container size - this is
no longer the case.
If the bounds change we'll get a layout - which will update the bbox and the
container size - so if the bounds are dirty at the point when this is called,
the container size will be updated again during layout. If the bounds are not
dirty this is a no-op except for the case mentioned in the comment (loading
finished after layout) - which we do have a test for.

 
     m_bufferedForeground.clear();