Cleanups to viewport-handling and image invalidation in LayoutSVGImage

Source/core/layout/svg/LayoutSVGImage.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 70 matching lines @@
 
 static bool containerSizeIsSetForLayoutObject(ImageResource& cachedImage, const LayoutObject* layoutObject)
 {
     const Image* image = cachedImage.image();
     // If a container size has been specified for this layoutObject, then
     // imageForLayoutObject() will return the SVGImageForContainer while image()
     // will return the underlying SVGImage.
     return !image->isSVGImage() || image != cachedImage.imageForLayoutObject(layoutObject);
 }
 
-bool LayoutSVGImage::updateImageViewport()
+void LayoutSVGImage::updateImageContainerSize()
 {
-    SVGImageElement* image = toSVGImageElement(element());
+    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()
+{
     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()
 {
     ASSERT(needsLayout());
     LayoutAnalyzer::Scope analyzer(*this);
 
-    updateImageViewport();
+    updateBoundingBox();
+    updateImageContainerSize();
 
     bool transformOrBoundariesUpdate = m_needsTransformUpdate || m_needsBoundariesUpdate;
     if (m_needsTransformUpdate) {
         m_localTransform = toSVGImageElement(element())->calculateAnimatedLocalTransform();
         m_needsTransformUpdate = false;
     }
 
     if (m_needsBoundariesUpdate) {
         m_bufferedForeground.clear();
 
@@ skipping 38 matching lines @@
                 return true;
             }
         }
     }
 
     return false;
 }
 
 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();
 
     setShouldDoFullPaintInvalidation();
 }
 
 void LayoutSVGImage::addFocusRingRects(Vector<LayoutRect>& rects, const LayoutPoint&) const
 {
     // this is called from paint() after the localTransform has already been applied
     LayoutRect contentRect = LayoutRect(paintInvalidationRectInLocalCoordinates());
     if (!contentRect.isEmpty())
         rects.append(contentRect);
 }
 
 } // namespace blink