[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