[CSS Shapes] Dynamically created element with image valued shape-outside doesn't update

[CSS Shapes] Dynamically created element with image valued shape-outside doesn't update

Corrected the way shape-outside handles the completion of an image load. Move the shape-outside imageChanged() logic from RenderBlock to RenderBox and call markShapeOutsideDependentsForLayout() instead of parent()->setNeedsLayoutAndPrefWidthsRecalc(). The latter did not deal with descendants of the shape element's siblings correctly and it failed when the shape element was inserted dynamically. The markShapeOutsideDependentsForLayout() method can't be called during layout, so the imageChanged() code checks for that. The only scenario where imageChanged() can be called during layout (that I've discovered so far anyway) is when an SVG Image is rendered  with drawImage(). The Shape::createRasterShape() does, and the corresponding imageChanged() notification can be safely ignored.

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

[Hans Muller, 2014-02-04 16:52:32]

This patch improves long-standing flakiness in the way shape-outside responds to
the completion of an shape value image load.  It should also correct the problem
reported here: https://code.google.com/p/chromium/issues/detail?id=339584.

[eseidel, 2014-02-04 17:40:47]

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

https://codereview.chromium.org/131133004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1634: if (!(frameView() &&
frameView()->isInPerformLayout()) && isFloating() && shapeOutsideValue &&
shapeOutsideValue->image() && shapeOutsideValue->image()->data() == image) {
We shouldn't be talking to renderers who cannot reach a frameview, so I would
turn the frameView() check into an ASSERT or remove it.  If it turns out it's
needed, we should file a bug to remove it.

Are you sure the isInLayout check is correct?  I wouldn't expect loads to
complete at arbitrary times during layout, so I would not expect images to
change, but maybe this is from an SVGImage case?

[Hans Muller, 2014-02-04 18:02:49]

On 2014/02/04 17:40:47, eseidel wrote:
>
https://codereview.chromium.org/131133004/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderBox.cpp (right):
> 
>
https://codereview.chromium.org/131133004/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderBox.cpp:1634: if (!(frameView() &&
> frameView()->isInPerformLayout()) && isFloating() && shapeOutsideValue &&
> shapeOutsideValue->image() && shapeOutsideValue->image()->data() == image) {
> We shouldn't be talking to renderers who cannot reach a frameview, so I would
> turn the frameView() check into an ASSERT or remove it.  If it turns out it's
> needed, we should file a bug to remove it.

Good point, I'll take the check out.

> 
> Are you sure the isInLayout check is correct?  I wouldn't expect loads to
> complete at arbitrary times during layout, so I would not expect images to
> change, but maybe this is from an SVGImage case?

Yes it's from the SVGImage drawImage() case. That's the only case I know of
where the image notification is triggered during layout.

[Hans Muller, 2014-02-04 18:37:16]

I've removed the unnecessary frameView() check.

[eseidel, 2014-02-13 20:34:14]

lgtm

OK.

[commit-bot: I haz the power, 2014-02-13 20:34:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/131133004/60001

[commit-bot: I haz the power, 2014-02-13 23:53:49]

Change committed as 167124