[webkit-reviews] review granted: [Bug 38919] Add support for fit-content etc : [Attachment 148379] Patch

Tue Jun 19 13:07:12 PDT 2012

Tony Chang <tony at chromium.org> has granted Elliott Sprehn <esprehn at gmail.com>'s
request for review:
Bug 38919: Add support for fit-content etc
https://bugs.webkit.org/show_bug.cgi?id=38919

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

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

Looks great, just some small nits.  If you fix the nits, you can use
"webkit-patch land-safely" to upload a new patch with cq? .

> Source/WebCore/ChangeLog:25
> +	   Implement the CSS3 intrinsic dimension keywords for width properties
and
> +	   add most of the plumbing for height properties but don't expose them

> +	   yet since this patch doesn't enforce them (matching current Gecko).
> +	   http://dev.w3.org/csswg/css3-writing-modes/#intrinsic-sizing
> +
> +	   This patch implements -webkit-min-content, -webkit-max-content,
> +	   -webkit-fill-available and -webkit-fit-content for all width
> +	   properties.

Nit: Move this between "Reviewed by ..." and "Tests: ..."

> Source/WebCore/css/CSSParser.cpp:1945
> +    case CSSPropertyWidth: // <length> | <percentage> | auto | inherit
>      case CSSPropertyWebkitLogicalWidth:
> +	   if (id == CSSValueAuto || id == CSSValueIntrinsic || id ==
CSSValueMinIntrinsic || id == CSSValueWebkitMinContent || id ==
CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id ==
CSSValueWebkitFitContent)

Can we have width and webkitlogicalwidth check for the new values and fall
through (/* no break */) to height/webkitlogicalheight?

> LayoutTests/fast/css-intrinsic-dimensions/height-property-value.html:4
> +<!--
> +    Tests that the height keywords are not respected by the parser yet.
> +-->

Put this in description().

> LayoutTests/fast/css-intrinsic-dimensions/width-dynamic-property-value.html:4

> +<!doctype html>
> +<script src="../js/resources/js-test-pre.js"></script>
> +<script>
> +    description('Tests that the width keywords are respected when
dynamically assigned.');

Nit: Maybe merge the text output tests (tests using js-test-pre) together into
a single test? Having fewer test files is a bit easier on to manage (better for
svn and easier to mark failing tests).

> LayoutTests/fast/css-intrinsic-dimensions/width-keyword-classes.css:2
> +/*
> +    Take every possible line break so the box width is the width of largest

Please move this test into a subdirectory called resources.