Simplify RenderImage::imageDimensionsChanged()

Source/core/rendering/RenderImage.cpp

Index: Source/core/rendering/RenderImage.cpp
diff --git a/Source/core/rendering/RenderImage.cpp b/Source/core/rendering/RenderImage.cpp
index 0a68e451818c029be59afd71ee3d7f09a7b16a73..4715049f1a32d23a083f3ceb17ff33491633a1f6 100644
--- a/Source/core/rendering/RenderImage.cpp
+++ b/Source/core/rendering/RenderImage.cpp
@@ -184,14 +184,11 @@ void RenderImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)
     imageDimensionsChanged(imageSizeChanged, rect);
 }
 
-bool RenderImage::updateIntrinsicSizeIfNeeded(const LayoutSize& newSize, bool imageSizeChanged)
+void RenderImage::updateIntrinsicSizeIfNeeded(const LayoutSize& newSize)
 {
-    if (newSize == intrinsicSize() && !imageSizeChanged)
-        return false;
     if (m_imageResource->errorOccurred() || !m_imageResource->hasImage())
-        return imageSizeChanged;
+        return;
     setIntrinsicSize(newSize);
-    return true;
 }
 
 void RenderImage::updateInnerContentRect()
@@ -203,9 +200,11 @@ void RenderImage::updateInnerContentRect()
         m_imageResource->setContainerSizeForRenderer(containerSize);
 }
 
-void RenderImage::imageDimensionsChanged(bool imageSizeChanged, const IntRect* rect)
+void RenderImage::imageDimensionsChanged(bool imageSizeChangedToAccomodateAltText, const IntRect* rect)
 {
-    bool intrinsicSizeChanged = updateIntrinsicSizeIfNeeded(m_imageResource->intrinsicSize(style()->effectiveZoom()), imageSizeChanged);
+    LayoutSize oldIntrinsicSize = intrinsicSize();
+    LayoutSize newIntrinsicSize = m_imageResource->intrinsicSize(style()->effectiveZoom());
+    updateIntrinsicSizeIfNeeded(newIntrinsicSize);
 
     // In the case of generated image content using :before/:after/content, we might not be
     // in the render tree yet. In that case, we just need to update our intrinsic size.
@@ -214,35 +213,16 @@ void RenderImage::imageDimensionsChanged(bool imageSizeChanged, const IntRect* r
     if (!containingBlock())
         return;
 
-    bool shouldRepaint = true;
-    if (intrinsicSizeChanged) {
-        if (!preferredLogicalWidthsDirty())
-            setPreferredLogicalWidthsDirty();
-
-        bool hasOverrideSize = hasOverrideHeight() || hasOverrideWidth();
-        if (!hasOverrideSize && !imageSizeChanged) {
-            LogicalExtentComputedValues computedValues;
-            computeLogicalWidthInRegion(computedValues);
-            LayoutUnit newWidth = computedValues.m_extent;
-            computeLogicalHeight(height(), 0, computedValues);
-            LayoutUnit newHeight = computedValues.m_extent;
-
-            imageSizeChanged = width() != newWidth || height() != newHeight;
-        }
+    bool intrinsicSizeChanged = oldIntrinsicSize != newIntrinsicSize;
+    if (intrinsicSizeChanged || imageSizeChangedToAccomodateAltText)
+        setPreferredLogicalWidthsDirty();
 
-        // FIXME: We only need to recompute the containing block's preferred size
-        // if the containing block's size depends on the image's size (i.e., the container uses shrink-to-fit sizing).
-        // There's no easy way to detect that shrink-to-fit is needed, always force a layout.
-        bool containingBlockNeedsToRecomputePreferredSize =
-            style()->logicalWidth().isPercent()
-            || style()->logicalMaxWidth().isPercent()
-            || style()->logicalMinWidth().isPercent();
-
-        if (imageSizeChanged || hasOverrideSize || containingBlockNeedsToRecomputePreferredSize) {
-            shouldRepaint = false;
-            if (!selfNeedsLayout())
-                setNeedsLayout();
-        }
+    // If the actual area occupied by the image has changed then a layout is required, otherwise a repaint will suffice.
+    bool hasSpecifiedSize = style()->logicalWidth().isSpecified() && style()->logicalHeight().isSpecified();
+    bool needsLayout = !selfNeedsLayout() && !hasSpecifiedSize && (intrinsicSizeChanged || imageSizeChangedToAccomodateAltText);

[ojan, 2014/04/10 22:36:36]

Why do we need the !selfNeedsLayout check? setNeedsLayout will early return if
image already needs layout. Would be nice to avoid the repaint code below if it
already needs layout.

[rhogan, 2014/04/16 20:55:42]

No good reason - just didn't drop the existing check for !selfNeedsLayout() when
refactoring. I'm dropping it in the updated patch.

+    if (needsLayout) {
+        setNeedsLayout();
+        return;

[ojan, 2014/04/10 22:36:36]

I'm a bit worried about this early return. Skipping the repaint code is create,
but I think it might be wrong to skip the contentChanged call. Not 100% sure.

[rhogan, 2014/04/16 20:55:42]

It's currently skipped if we decide you need a layout, so this isn't a change in
behaviour.

[ojan, 2014/04/18 01:26:52]

Ah, yes. I misread the old code.

     }
 
     if (everHadLayout() && !selfNeedsLayout()) {
@@ -253,22 +233,21 @@ void RenderImage::imageDimensionsChanged(bool imageSizeChanged, const IntRect* r
         updateInnerContentRect();
     }
 
-    if (shouldRepaint) {
-        LayoutRect repaintRect;
-        if (rect) {
-            // The image changed rect is in source image coordinates (pre-zooming),
-            // so map from the bounds of the image to the contentsBox.
-            repaintRect = enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), m_imageResource->imageSize(1.0f)), contentBoxRect()));
-            // Guard against too-large changed rects.
-            repaintRect.intersect(contentBoxRect());
-        } else
-            repaintRect = contentBoxRect();
+    LayoutRect repaintRect;
+    if (rect) {
+        // The image changed rect is in source image coordinates (pre-zooming),
+        // so map from the bounds of the image to the contentsBox.
+        repaintRect = enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), m_imageResource->imageSize(1.0f)), contentBoxRect()));
+        // Guard against too-large changed rects.
+        repaintRect.intersect(contentBoxRect());
+    } else {
+        repaintRect = contentBoxRect();
+    }
 
-        repaintRectangle(repaintRect);
+    repaintRectangle(repaintRect);
 
-        // Tell any potential compositing layers that the image needs updating.
-        contentChanged(ImageChanged);
-    }
+    // Tell any potential compositing layers that the image needs updating.
+    contentChanged(ImageChanged);
 }
 
 void RenderImage::notifyFinished(Resource* newImage)