Bug 80552 – implement flexbox wrap-reverse

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

implement wrap-reverse

[Tony Chang]

Created attachment 130728 [details]
Patch

[Ojan Vafai]

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.

[Tony Chang]

Created attachment 130858 [details]
Patch for landing

[Tony Chang]

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.

[Ojan Vafai]

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.

[Tony Chang]

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.

[Tony Chang]

Committed r110209: <http://trac.webkit.org/changeset/110209>