Simplify RenderImage::imageDimensionsChanged()

Source/core/rendering/RenderImage.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2003, 2004, 2005, 2006, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Google Inc. All rights reserved.
  * Copyright (C) Research In Motion Limited 2011-2012. All rights reserved.
  *
@@ skipping 166 matching lines @@
 
     bool imageSizeChanged = false;
 
     // Set image dimensions, taking into account the size of the alt text.
     if (m_imageResource->errorOccurred() || !newImage)
         imageSizeChanged = setImageSizeForAltText(m_imageResource->cachedImage());
 
     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()
 {
     // Propagate container size to the image resource.
     LayoutRect containerRect = replacedContentRect();
     IntSize containerSize(containerRect.width(), containerRect.height());
     if (!containerSize.isEmpty())
         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.
     // layout() will be called after we are inserted in the tree which will take care of
     // what we are doing here.
     if (!containingBlock())
         return;
 
-    bool shouldRepaint = true;
-    if (intrinsicSizeChanged) {
-        if (!preferredLogicalWidthsDirty())
-            setPreferredLogicalWidthsDirty();
+    bool intrinsicSizeChanged = oldIntrinsicSize != newIntrinsicSize;
+    if (intrinsicSizeChanged || imageSizeChangedToAccomodateAltText)
+        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;
-        }
-
-        // 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()) {
         // The inner content rectangle is calculated during layout, but may need an update now
         // (unless the box has already been scheduled for layout). In order to calculate it, we
         // may need values from the containing block, though, so make sure that we're not too
         // early. It may be that layout hasn't even taken place once yet.
         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)
 {
     if (!m_imageResource)
         return;
 
     if (documentBeingDestroyed())
         return;
 
@@ skipping 344 matching lines @@
         return 0;
 
     ImageResource* cachedImage = m_imageResource->cachedImage();
     if (cachedImage && cachedImage->image() && cachedImage->image()->isSVGImage())
         return toSVGImage(cachedImage->image())->embeddedContentBox();
 
     return 0;
 }
 
 } // namespace WebCore