[webkit-reviews] review denied: [Bug 75782] Implement flex-align : [Attachment 122322] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jan 12 15:56:53 PST 2012


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

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122322&action=review


r- because of missing test cases.

> Source/WebCore/rendering/RenderBox.cpp:1803
> +	   EFlexAlign align = style()->flexItemAlign();
> +	   if (align == AlignAuto)
> +	       align = parent()->style()->flexAlign();
> +	   if (align != AlignStretch)
> +	       return true;

Nit: I would probably just do "if (itemalign == stretch || (itemalign == auto
&& parent->itemalign == stretch))".  *shrug*

> Source/WebCore/rendering/RenderFlexibleBox.cpp:621
> +static EFlexAlign flexAlign(RenderBox* flexItem)

Nit: Maybe name it flexAlignForChild?

> LayoutTests/css3/flexbox/flex-align.html:124
> +</div>
> +

It would be nice to have a test with flex-align set and flex-item-align not
set.

> LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt:147
> --webkit-flex-item-align: stretch;
> +-webkit-flex-align: stretch;

There are port specific results for these tests too, right?


More information about the webkit-reviews mailing list