[webkit-reviews] review granted: [Bug 79930] Implement flex-wrap: wrap : [Attachment 129487] 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 79930: Implement flex-wrap: wrap
https://bugs.webkit.org/show_bug.cgi?id=79930

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

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


Just the one issue for Hyatt to look at. The rest looks good modulo some nits.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:592
> +bool RenderFlexibleBox::computeFlexOrder(FlexOrderIterator& iterator,
OrderedFlexItemList& orderedChildren, LayoutUnit& preferredMainAxisExtent,
float& totalPositiveFlexibility, float& totalNegativeFlexibility)

Not a huge fan of this method name. How about....computeNextFlexLine?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:769
> +	       setCrossAxisExtent(std::max(crossAxisExtent(), crossAxisOffset -
flowAwareBorderBefore() - flowAwarePaddingBefore() +
crossAxisBorderAndPaddingExtent() + crossAxisMarginExtentForChild(child) +
childCrossAxisExtent + crossAxisScrollbarExtent()));

This is kind of gross. I'm not sure how to make it cleaner though. :(

> Source/WebCore/rendering/RenderFlexibleBox.cpp:795
> +    // Single line flexboxes fill the alignment space.

This isn't really an accurate comment. I think it just makes the code more
confusing.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:869
> +	       } else if (isColumnFlow() &&
child->style()->logicalWidth().isAuto() && isMultiline()) {
> +		   // FIXME: Handle min-width and max-width.
> +		   LayoutUnit childWidth = lineCrossAxisExtent -
crossAxisMarginExtentForChild(child);
> +		   child->setOverrideWidth(std::max(0, childWidth));
> +		   child->setChildNeedsLayout(true);
> +		   child->layoutIfNeeded();

This part scares me. I think it's right, but it would be good to get Hyatt to
confirm before committing.