[webkit-reviews] review denied: [Bug 69808] Implement -webkit-flex-align for cross axis alignment : [Attachment 110454] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Tony Chang <tony at chromium.org>'s
request for review:
Bug 69808: Implement -webkit-flex-align for cross axis alignment
https://bugs.webkit.org/show_bug.cgi?id=69808

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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?