[webkit-reviews] review granted: [Bug 110654] Empty <button>s should collapse; empty <input type="button"> should not collapse : [Attachment 190166] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Christian Biesinger
<cbiesinger at chromium.org>'s request for review:
Bug 110654: Empty <button>s should collapse; empty <input type="button"> should
not collapse
https://bugs.webkit.org/show_bug.cgi?id=110654

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

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


Looks great.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:733
> +	   // We need to make sure to have a line. We might not have a line
even if we entered the above loop,
> +	   // because all our children might be positioned. Therefore, we use
this approach to ensure the
> +	   // right height.

This could be a comment more focused on the "why" and less on the "what", e.g.,


// Even if computeNextFlexLine returns true, the flexbox might not have a line
because all our
// children might be out of flow positioned. Instead of just checking if we
have a line,
// make sure the flexbox has at least a line's worth of height to cover this
case.