[webkit-reviews] review denied: [Bug 102352] Avoid a second layout of flex items in layoutAndPlaceChildren() : [Attachment 176919] Updated patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 102352: Avoid a second layout of flex items in layoutAndPlaceChildren()
https://bugs.webkit.org/show_bug.cgi?id=102352

Attachment 176919: Updated patch
https://bugs.webkit.org/attachment.cgi?id=176919&action=review

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


> LayoutTests/css3/flexbox/stretch-after-sibling-size-change.html:17
> +  <div id=a class="flex-one" data-expected-width="50"
data-expected-height="20" style="background-color: blue; height: 30px;"></div>
> +  <div id=b class="flex-one" data-expected-width="50"
data-expected-height="20" style="background-color: green"></div>

Nit: 2 spec indent.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:186
> +	   // doesn't move the thumb to the wrong place. FIXME: Make a custom
Render class

The FIXME should go at the beginning of a line.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1074
> +    bool isColumn = isColumnFlow();
> +    return (!isColumn && child->style()->logicalHeight().isAuto()) ||
(isColumn && child->style()->logicalWidth().isAuto());

I'd find this easier to read as follows:
var crossAxisLength = isColumnFlow() ? child->style->logicalWidth() :
child->style->logicalHeight();
return crossAxisLength.isAuto();

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1081
> +    Length start = isHorizontal ? child->style()->marginTop() :
child->style()->marginLeft();
> +    Length end = isHorizontal ? child->style()->marginBottom() :
child->style()->marginRight();

s/start/before
s/end/after

You could use flowAwareMarginBeforeForChild and flowAwareMarginAfterForChild.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1114
> +	       resetAutoMarginsAndLogicalTopInCrossAxis(child);

This could use a comment explaining why we need to do this. I had to go read
the comments in the bug to make sense of this.