Created attachment 110454 [details] Patch

Comment on attachment 110454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110454&action=review A first pass review. I'd like hyatt to do the final r+ here since I'm unsure about the baseline related stuff. > LayoutTests/css3/flexbox/flex-align.html:105 > + <div data-expected-height="100" data-offset-y="0" style="width: -webkit-flex(1 0 0); height: 100px;"></div> Would be good to have a test-case for items with different top/bottom margins. Specifically, I'd like to see flex-align:stretch and flex-align:baseline with something like the following: <div class=flexbox> <div style="width: -webkit-flex(1); -webkit-flex-align: stretch; height: 50px; margin: 10px 20px 30px 40px"></div> <div style="width: -webkit-flex(1); -webkit-flex-align: stretch; height: 50px; margin: 2px 4px 6px 8px"></div> </div> > Source/WebCore/rendering/RenderFlexibleBox.cpp:512 > + if (isFlowAwareLogicalHeightAuto()) > + setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + maxAscent + maxDescent + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight())); > + } else if (isFlowAwareLogicalHeightAuto()) > + setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight())); Can you instead move flowAwareBorderAndPaddingHeight and flowAwareMarginHeightForChild into helper functions? That would make this a lot easier to read and avoid code duplication here and above. > Source/WebCore/rendering/RenderFlexibleBox.cpp:540 > +void RenderFlexibleBox::alignChildrenBlockDirection(FlexOrderIterator& iterator, LayoutUnit maxAscent, LayoutUnit maxDescent) You don't use maxDescent. Did you not get a compiler warning? > Source/WebCore/rendering/RenderFlexibleBox.cpp:544 > + LayoutUnit currentChildHeight = flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child); You compute this on line 503 as well. Move into a helper function? flowAwareLogicalMarginBoxHeightForChild? lol. > Source/WebCore/rendering/RenderFlexibleBox.cpp:555 > + case AlignStart: > + break; Could you check the flexAlign as the beginning of the function and early return? Then here you could ASSERT_NOT_REACHED? I guess that's a premature optimization, but a FIXME at least would be good. > Source/WebCore/rendering/RenderFlexibleBox.cpp:566 > + LayoutUnit ascent = child->firstLineBoxBaseline(); > + if (ascent == -1) > + ascent = flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child); > + ascent += flowAwareMarginBeforeForChild(child); This chunk of code is exactly the same as in layoutAndPlaceChildrenInlineDirection. Pull it out into a helper function?

Created attachment 110574 [details] Patch

Created attachment 110575 [details] Patch

(In reply to comment #2) > (From update of attachment 110454 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110454&action=review > > > LayoutTests/css3/flexbox/flex-align.html:105 > > + <div data-expected-height="100" data-offset-y="0" style="width: -webkit-flex(1 0 0); height: 100px;"></div> > > Would be good to have a test-case for items with different top/bottom margins. Done. > > Source/WebCore/rendering/RenderFlexibleBox.cpp:512 > > + if (isFlowAwareLogicalHeightAuto()) > > + setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + maxAscent + maxDescent + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight())); > > + } else if (isFlowAwareLogicalHeightAuto()) > > + setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight())); > > Can you instead move flowAwareBorderAndPaddingHeight and flowAwareMarginHeightForChild into helper functions? That would make this a lot easier to read and avoid code duplication here and above. Done. > > Source/WebCore/rendering/RenderFlexibleBox.cpp:540 > > +void RenderFlexibleBox::alignChildrenBlockDirection(FlexOrderIterator& iterator, LayoutUnit maxAscent, LayoutUnit maxDescent) > > You don't use maxDescent. Did you not get a compiler warning? Removed. > > Source/WebCore/rendering/RenderFlexibleBox.cpp:544 > > + LayoutUnit currentChildHeight = flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child); > > You compute this on line 503 as well. Move into a helper function? flowAwareLogicalMarginBoxHeightForChild? lol. Switched to use flowAwareMarginLogicalHeightForChild(). > > Source/WebCore/rendering/RenderFlexibleBox.cpp:555 > > + case AlignStart: > > + break; > > Could you check the flexAlign as the beginning of the function and early return? Then here you could ASSERT_NOT_REACHED? I guess that's a premature optimization, but a FIXME at least would be good. I added availableLogicalHeightForChild to avoid the extra computation. An early return wouldn't have worked since this is applied to each child. > > Source/WebCore/rendering/RenderFlexibleBox.cpp:566 > > + LayoutUnit ascent = child->firstLineBoxBaseline(); > > + if (ascent == -1) > > + ascent = flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child); > > + ascent += flowAwareMarginBeforeForChild(child); > > This chunk of code is exactly the same as in layoutAndPlaceChildrenInlineDirection. Pull it out into a helper function? Done.

Comment on attachment 110575 [details] Patch Patch looks good to me. Would like Hyatt to do a pass just to make sure we're not doing anything egregiously wrong.

Comment on attachment 110575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110575&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:571 > + case AlignStretch: { > + Length height = isHorizontalFlow() ? child->style()->height() : child->style()->width(); > + if (height.isAuto()) > + child->setLogicalHeight(child->logicalHeight() + RenderFlexibleBox::availableLogicalHeightForChild(child)); > + break; This is wrong if the child is a perpendicular writing mode. You need setLogicalHeightForChild and to use logicalHeightForChild, or you could just use physical setters.

Created attachment 110738 [details] Patch

(In reply to comment #7) > (From update of attachment 110575 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110575&action=review > > > Source/WebCore/rendering/RenderFlexibleBox.cpp:571 > > + case AlignStretch: { > > + Length height = isHorizontalFlow() ? child->style()->height() : child->style()->width(); > > + if (height.isAuto()) > > + child->setLogicalHeight(child->logicalHeight() + RenderFlexibleBox::availableLogicalHeightForChild(child)); > > + break; > > This is wrong if the child is a perpendicular writing mode. You need setLogicalHeightForChild and to use logicalHeightForChild, or you could just use physical setters. Updated to use physical setters and logicalHeightForChild. Also added a test case.

Comment on attachment 110738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110738&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:578 > + child->setHeight(logicalHeightForChild(child) + RenderFlexibleBox::availableLogicalHeightForChild(child)); > + else > + child->setWidth(logicalHeightForChild(child) + RenderFlexibleBox::availableLogicalHeightForChild(child)); nit: move logicalHeightForChild(child) + RenderFlexibleBox::availableLogicalHeightForChild(child) into a local variable (e.g. newLogicalHeight) so you don't have to repeat the code?

Created attachment 110749 [details] Patch

Comment on attachment 110749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110749&action=review r=me > Source/WebCore/rendering/RenderFlexibleBox.cpp:573 > + Length height = isHorizontalFlow() ? child->style()->height() : child->style()->width(); RenderStyle does have helpers for margins but not for logical height. If you wanted to add a logicalHeightUsing method to RenderStyle helper so you could say child->style()->logcialHeightUsing(style()) I would be fine with that. Not required though. Also note you don't handle maxHeight clamping here. Not sure if you have to or not. I'd put a FIXME: in asking and not worry about it for now.

Created attachment 110754 [details] Patch for landing

Comment on attachment 110754 [details] Patch for landing Clearing flags on attachment: 110754 Committed r97313: <http://trac.webkit.org/changeset/97313>

All reviewed patches have been landed. Closing bug.