Whenever we're extending the logicalHeight of a flex-item that we've already laid out (e.g. due to flexing or flex-align:stretch), we only need to layout after modifying the height if the contents of the item depend on it's height (e.g. contains % height elements and is a containing block, has a background-color, etc) because we already. The tricky bit here is maintaining the function of when we need to do a relayout. In theory, in some of these cases we could take this a step further and avoid doing a full relayout, e.g. we don't actually need to relayout just to extend the background color further up/down; for percentage height decendants, we already keep a list of them and only need to relayout those items, etc.
Created attachment 174091 [details] Patch I'm getting a failure in 3 tests that I'm not sure why. css3/flexbox/inline-flex-crash.html css3/flexbox/inline-flex-crash2.html The output is different in both cases, but it doesn't crash. css3/flexbox/auto-margins.html The output is correct, but the first time it's called it fails to get the margin values, although the inspector shows the right ones and simply reloading the page fixes the test. All other css3 flexbox tests pass.
Comment on attachment 174091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174091&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:1090 > + LayoutUnit childCurrentSize = isColumnFlow() ? child->logicalHeight() : child->logicalWidth(); You want mainAxisExtentForChild(). You don't want the child's logical height since it can have an orthogonal writing mode to the flexbox.
Comment on attachment 174091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174091&action=review This is actually a separate optimization than bug 87905. That bug is about when we *do* change the height, there are many cases where we don't need to do a layout (i.e. if there are no descendants or css properties that depend on the height). This is still a good optimization though. It just needs it's own bug. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1094 > + if (child->size().isZero() || childPreferredSize != childCurrentSize) { > + child->setChildNeedsLayout(true, MarkOnlyThis); > + child->layoutIfNeeded(); > + } I don't think we need the isZero check. Instead, we should always call child->layoutIfNeeded() here (i.e. move it out of the if-statement), but only setChildNeedsLayout if childPreferredSize != childCurrentSize.
Thank you guys for the review! I've just opened a new bug with a patch addressing your comments, see: https://bugs.webkit.org/show_bug.cgi?id=102352 css3/flexbox/inline-flex-crash.html and css3/flexbox/inline-flex-crash2.html don't fail anymore with the new patch.