[webkit-reviews] review granted: [Bug 82240] apply cross axis constraints before aligning children in flexbox : [Attachment 133883] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Tony Chang <tony at chromium.org>'s
request for review:
Bug 82240: apply cross axis constraints before aligning children in flexbox
https://bugs.webkit.org/show_bug.cgi?id=82240

Attachment 133883: Patch
https://bugs.webkit.org/attachment.cgi?id=133883&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133883&action=review


Yay! This looks great!

> Source/WebCore/rendering/RenderFlexibleBox.cpp:274
> +    if (!isMultiline() && lineContexts.size() == 1)

Should this also have ASSERT(lineContexts.size() <= 1)? Can we have a non-1
size if it's not multiline other than the case of no flex items? Meh. Maybe
it's overkill.

This maybe deserves a comment? I had to read a bunch of code to piece together
that we need to reset crossAxisExtent *after* computeLogicalHeight.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:285
> +	   lineContexts[0].crossAxisExtent = crossAxisContentExtent();
> +    alignChildren(flexIterator, lineContexts);
> +
> +    if (style()->flexWrap() == FlexWrapReverse) {
> +	   if (isHorizontalFlow())
> +	       oldClientAfterEdge = clientLogicalBottom();
> +	   flipForWrapReverse(flexIterator, lineContexts);
> +    }
> +
> +    // direction:rtl + flex-direction:column means the cross-axis direction
is flipped.
> +    flipForRightToLeftColumn(flexIterator);

Would make the code a bit more readable to move this into a function IMO.
repositionLogicalHeightDependentFlexItems? It's wordy, but it make the logic of
computeLogicalWidth-->layoutItems-->computeLogicalHeight-->resposition more
clear, e.g. someone modifying this code will be more likely to understand that
they can only reposition things in repositionHeightDependentFlexItems.

I guess the confusing thing there is that alignChildren isn't necessarily
height-dependent, but I think that's OK.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:929
>	   layoutColumnReverse(children, childSizes, crossAxisOffset,
availableFreeSpace);

Should we move this to after the computLogicalHeight call in layoutBlock? Then
we would just have the 1 computeLogicalHeight call in all of flexbox layout.