Simplify RenderImage::imageDimensionsChanged()

Simplify RenderImage::imageDimensionsChanged()

This function decides if a change to the image requires relayout or if
repainting will suffice. It only needs to worry about layout-related updates to the
element that won't be detected elsewhere. These are: a change to the
image's intrinsic size (e.g. when the resource changes) or a change in
size to accomodate alt text for an image that failed to load - otherwise a
repaint will do just as well.

We don't need to recalculate height and width to figure this out - and the basis
of this patch is that doing so is a false economy. If the intrinsic
size has changed the only conditions that would avoid the need for triggering a
layout are:

  - if the first layout hasn't happened yet;
  - if the CSS dimensions of the image prevent the intrinsic size ever affecting
    the used size of the image, i.e. if it has fixed, percent, or calculated dimensions.

We don't need to worry about:

  - Absolutely positioned images with top/left/right/bottom specified. If the intrinsic
    dimensions of such an image changes, the used dimensions do too.

So currently when intrinsic size has changed we will calculate width/height twice - once
in order to figure out that a layout is required and a second time during layout itself.
Remove this unnecessary extra work by laying out if the intrinsic size has changed and
it affects the size of the image, otherwise just repaint.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171915

[ojan, 2014-01-22 01:45:56]

I'm not sure this is an improvement. It's less code, which is nice. But, it's
also less general.

I listed a couple cases where I think that the new code is forcing a layout more
often than it needs to. And, as we add other features to the platform that might
affect image sizes, this code would need to be updated.

https://codereview.chromium.org/114323004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/114323004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderImage.cpp:231: bool hasFixedSize =
style()->logicalWidth().isFixed() && style()->logicalHeight().isFixed();
I think you can use isSpecified here instead of isFixed to be a bit more
general.

You'll likely need to add back in the width percent thing on line 250.

Actually, as I think about this more, there are other cases that this won't
catch. For example, a position:absolute image that has top/right/bottom/left all
set.

https://codereview.chromium.org/114323004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderImage.cpp:235: setNeedsLayoutAndPrefWidthsRecalc();
I think you want to separate these out into the setNeedsLayout call and the
setPreferredLogicalWidthsDirty() call. The latter I think you need to do if
intrinsicSizeChanged || imageSizeChangedToAccomodateAltText. See
RenderReplaced::computePreferredLogicalWidths, which uses the intrinsic image
size even if the width is fixed.

[rhogan, 2014-04-02 18:11:26]

On 2014/01/22 01:45:56, ojan wrote:
> I'm not sure this is an improvement. It's less code, which is nice. But, it's
> also less general.
> 
> I listed a couple cases where I think that the new code is forcing a layout
more
> often than it needs to. And, as we add other features to the platform that
might
> affect image sizes, this code would need to be updated.
> 
>
https://codereview.chromium.org/114323004/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderImage.cpp (right):
> 
>
https://codereview.chromium.org/114323004/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderImage.cpp:231: bool hasFixedSize =
> style()->logicalWidth().isFixed() && style()->logicalHeight().isFixed();
> I think you can use isSpecified here instead of isFixed to be a bit more
> general.
> 
> You'll likely need to add back in the width percent thing on line 250.
> 
> Actually, as I think about this more, there are other cases that this won't
> catch. For example, a position:absolute image that has top/right/bottom/left
all
> set.
> 
>
https://codereview.chromium.org/114323004/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderImage.cpp:235:
setNeedsLayoutAndPrefWidthsRecalc();
> I think you want to separate these out into the setNeedsLayout call and the
> setPreferredLogicalWidthsDirty() call. The latter I think you need to do if
> intrinsicSizeChanged || imageSizeChangedToAccomodateAltText. See
> RenderReplaced::computePreferredLogicalWidths, which uses the intrinsic image
> size even if the width is fixed.

I've addressed your comments but are you of the view that this this refactoring
is too risky to proceed with? Should I just close it?

[ojan, 2014-04-10 22:36:35]

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:222: bool needsLayout = !selfNeedsLayout()
&& !hasSpecifiedSize && (intrinsicSizeChanged ||
imageSizeChangedToAccomodateAltText);
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.

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:225: return;
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.

[ojan, 2014-04-10 22:38:44]

Sorry to keep you waiting so long on this. I looked at this patch a dozen times
before finally giving it a proper review. It's a bit of a scary change, but I
like the code simplification, so I think it's worth trying once you address the
two comments.

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

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:222: bool needsLayout = !selfNeedsLayout()
&& !hasSpecifiedSize && (intrinsicSizeChanged ||
imageSizeChangedToAccomodateAltText);
On 2014/04/10 22:36:36, ojan wrote:
> 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.

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

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:225: return;
On 2014/04/10 22:36:36, ojan wrote:
> 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.

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:51]

lgtm

Sorry again that it too me so long to review this.

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/114323004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:225: return;
On 2014/04/16 20:55:42, rhogan wrote:
> On 2014/04/10 22:36:36, ojan wrote:
> > 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.
> 
> It's currently skipped if we decide you need a layout, so this isn't a change
in
> behaviour.

Ah, yes. I misread the old code.

[commit-bot: I haz the power, 2014-04-18 01:27:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robhogan@gmail.com/114323004/120001

[commit-bot: I haz the power, 2014-04-18 01:36:30]

Change committed as 171915