[webkit-reviews] review denied: [Bug 52441] WebKit flexbox shrink algorithm ignores min-width : [Attachment 91939] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Tony Chang <tony at chromium.org>'s
request for review:
Bug 52441: WebKit flexbox shrink algorithm ignores min-width
https://bugs.webkit.org/show_bug.cgi?id=52441

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=91939&action=review

I'd strongly recommend just saving algorithmic changes for building a brand new
version of flexbox that matches the spec, and leave this deprecated legacy
version alone.

> Source/WebCore/rendering/RenderBlock.cpp:629
> +bool RenderBlock::isFlexibleChild()
> +{
> +    return parent() && parent()->isFlexibleBox() && !isPositioned() &&
style()->boxFlex() > 0.0f;
> +}

I know they don't allow it right now, but just to be forward-thinking, I'd make
it !isFloatingOrPositioned instead of just !isPositioned().

Also, this seems incomplete to me.  What about flexing replaced elements? 
Seems like all you're fixing here is RenderBlock children of a flexbox, but
there can be other types of children like images or replaced elements.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1055
> +	   if (child->style()->minWidth().isFixed() &&
child->scrollsOverflowX())

I don't completely understand this case.  I don't get why we'd ignore the
minWidth of the object when you don't scroll.  I'd need to see the latest
flexbox draft to see what they suggest, but this seems suspicious to me.