implement wrap-reverse
Created attachment 130728 [details] Patch
Comment on attachment 130728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130728&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:130 > + return (crossAxisContentExtent - startOffset - lineHeight) - startOffset; Nit: I'd rather this be: return crossAxisContentExtent - lineHeight - (2 * startOffset); > Source/WebCore/rendering/RenderFlexibleBox.cpp:598 > + wrapReverseContext.addNumberOfChildrenOnLine(orderedChildren.size()); > + wrapReverseContext.addCrossAxisOffset(crossAxisOffset); Nit: I'd rather this be something like: wrapReverseContext.add(crossAxisOffset, orderedChildren); void WrapReverseContext::add(LayoutUnit crossAxisOffset, OrderedFlexItemList childrenOnLine) { if (!isWrapReverse) return; crossAxisOffsets.append(crossAxisOffset); childrenPerLine.append(childrenOnLine.size()); } > Source/WebCore/rendering/RenderFlexibleBox.cpp:953 > + if (style()->flexWrap() == FlexWrapReverse) > + adjustAlignmentForChild(child, availableAlignmentSpaceForChild(lineCrossAxisExtent, child)); This could use a comment (e.g. FlexWrapReverse means that stretched children should align to the cross-end edge) > Source/WebCore/rendering/RenderFlexibleBox.cpp:991 > + switch (flexAlignForChild(child)) { > + case AlignBaseline: > + adjustAlignmentForChild(child, minMarginAfterBaseline); > + break; > + case AlignAuto: > + case AlignStretch: > + case AlignStart: > + case AlignEnd: > + case AlignCenter: > + break; > + } Don't need the switch. Just make it an if statement. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1021 > + LayoutRect oldRect = child->frameRect(); Can you inline this to line 1024? > Source/WebCore/rendering/RenderFlexibleBox.cpp:1026 > + ++currentChild; Nit: I'd just put this in the if-statement below. I've certainly seen other WebKit code that does.
Created attachment 130858 [details] Patch for landing
Comment on attachment 130728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130728&action=review >> Source/WebCore/rendering/RenderFlexibleBox.cpp:130 >> + return (crossAxisContentExtent - startOffset - lineHeight) - startOffset; > > Nit: I'd rather this be: > return crossAxisContentExtent - lineHeight - (2 * startOffset); I split it into two lines to make it clear what crossAxisContentExtent - startOffset - lineHeight is. >> Source/WebCore/rendering/RenderFlexibleBox.cpp:598 >> + wrapReverseContext.addCrossAxisOffset(crossAxisOffset); > > Nit: I'd rather this be something like: > wrapReverseContext.add(crossAxisOffset, orderedChildren); > > void WrapReverseContext::add(LayoutUnit crossAxisOffset, OrderedFlexItemList childrenOnLine) > { > if (!isWrapReverse) > return; > crossAxisOffsets.append(crossAxisOffset); > childrenPerLine.append(childrenOnLine.size()); > } I didn't change this because I use addCrossAxisOffset below. >> Source/WebCore/rendering/RenderFlexibleBox.cpp:953 >> + adjustAlignmentForChild(child, availableAlignmentSpaceForChild(lineCrossAxisExtent, child)); > > This could use a comment (e.g. FlexWrapReverse means that stretched children should align to the cross-end edge) Done. >> Source/WebCore/rendering/RenderFlexibleBox.cpp:991 >> + } > > Don't need the switch. Just make it an if statement. Done. >> Source/WebCore/rendering/RenderFlexibleBox.cpp:1021 >> + LayoutRect oldRect = child->frameRect(); > > Can you inline this to line 1024? setFlowAwareLocationForChild changes the frameRect. >> Source/WebCore/rendering/RenderFlexibleBox.cpp:1026 >> + ++currentChild; > > Nit: I'd just put this in the if-statement below. I've certainly seen other WebKit code that does. Done.
Comment on attachment 130728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130728&action=review >>> Source/WebCore/rendering/RenderFlexibleBox.cpp:598 >>> + wrapReverseContext.addCrossAxisOffset(crossAxisOffset); >> >> Nit: I'd rather this be something like: >> wrapReverseContext.add(crossAxisOffset, orderedChildren); >> >> void WrapReverseContext::add(LayoutUnit crossAxisOffset, OrderedFlexItemList childrenOnLine) >> { >> if (!isWrapReverse) >> return; >> crossAxisOffsets.append(crossAxisOffset); >> childrenPerLine.append(childrenOnLine.size()); >> } > > I didn't change this because I use addCrossAxisOffset below. Right, but you can just passed in orderedChildren there as well without it causing problems, no? Not a big deal I guess.
Comment on attachment 130728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130728&action=review >>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:598 >>>> + wrapReverseContext.addCrossAxisOffset(crossAxisOffset); >>> >>> Nit: I'd rather this be something like: >>> wrapReverseContext.add(crossAxisOffset, orderedChildren); >>> >>> void WrapReverseContext::add(LayoutUnit crossAxisOffset, OrderedFlexItemList childrenOnLine) >>> { >>> if (!isWrapReverse) >>> return; >>> crossAxisOffsets.append(crossAxisOffset); >>> childrenPerLine.append(childrenOnLine.size()); >>> } >> >> I didn't change this because I use addCrossAxisOffset below. > > Right, but you can just passed in orderedChildren there as well without it causing problems, no? > > Not a big deal I guess. It would add an extra entry to childrenPerLine making us think we had an additional line. Maybe we could pass in a vector of size 0 and special case that since it's not possible to have 0 children in a line. It would be more code.
Committed r110209: <http://trac.webkit.org/changeset/110209>