[webkit-reviews] review granted: [Bug 112652] Make intrinsic size keywords on flexboxes work : [Attachment 193720] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 112652: Make intrinsic size keywords on flexboxes work
https://bugs.webkit.org/show_bug.cgi?id=112652

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

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


> LayoutTests/ChangeLog:15
> +	   *
platform/chromium-linux/css3/flexbox/flexbox-baseline-expected.png:
> +	   * platform/chromium-win/css3/flexbox/flexbox-baseline-expected.txt:
> +	   This looks like a rounding difference. The new result matches the
non-column result
> +	   in this same test, so it looks more correct to me.

Yeah, this change seems fine to me.  You probably need TestExpectation
suppressions for text failures on Mac and other non-chromium ports.  You
probably need image suppressions for chromium mac and chromium win.

>
LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.htm
l:10
> +    display: -webkit-flex;
> +    display: flex;
> +    -webkit-flex-direction: column;
> +    flex-direction: column;

Can we use resources/flexbox.css for these?

>
LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.htm
l:17
> +    -webkit-flex: none;
> +    flex: none;
> +    display: -webkit-flex;
> +    display: flex;

Ditto.

>
LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.htm
l:31
> +    <div class="child" style="width: -webkit-max-content;"
data-expected-width="210">

Nit: I would probably make a class for the width so when we drop the prefix,
it'll be easy to update the test.

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-flex-items.html:8
> +    display: -webkit-flex;
> +    display: flex;

Ditto.

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-flex-items.html:15
> +    display: -webkit-flex;
> +    display: flex;

Ditto.