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

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied 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 147648: Patch
https://bugs.webkit.org/attachment.cgi?id=147648&action=review

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


We should probably put this behind a compile time flag until it's more complete
(implemented for all the properties with tests). Also, please send an email to
webkit-dev explaining the new feature. See
http://trac.webkit.org/wiki/AddingFeatures and other similar emails.

> Source/WebCore/ChangeLog:11
> +	   Implement the CSS3 writing mode keywords for width and height
> +	   CSS properties (and associated min/max properties).

It looks like we're parsing the value for min-{width,height} and width/height
(not max), but currently only applying to width. We should be clear about that
in the changelog.

Since this will take more than one patch and webkit normally does one patch per
bug, you may want to file a new bug for what you're implementing and have this
bug depend on it (i.e., this bug would be a meta bug for tracking purposes).

> Source/WebCore/css/CSSParser.cpp:1906
> +	   if (id == CSSValueAuto || id == CSSValueIntrinsic || id ==
CSSValueMinIntrinsic || id == CSSValueWebkitMinContent || id ==
CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id ==
CSSValueWebkitFitContent)

Why are we parsing auto on min-width/min-height?  This will collide with Ojan's
patch on bug 88437 that adds it for real.

> Source/WebCore/css/CSSValueKeywords.in:575
> +// CSS3 writing modes keywords

Maybe CSS3 intrinsic dimensions?  writing modes keywords makes me think of
vertical-rl, horizontal-bt, etc.

> Source/WebCore/css/LengthFunctions.cpp:110
>      case MinIntrinsic:
>	   ASSERT_NOT_REACHED();
>	   return ZERO_LAYOUT_UNIT;
> +    case MinContent:
> +	   ASSERT_NOT_REACHED();
> +	   return ZERO_LAYOUT_UNIT;

Can we merge these cases like we do in the other switch statements?

> Source/WebCore/css/StyleBuilder.cpp:362
>	   else if (intrinsicEnabled && primitiveValue->getIdent() ==
CSSValueIntrinsic)
>	       setValue(styleResolver->style(), Length(Intrinsic));
> -	   else if (minIntrinsicEnabled && primitiveValue->getIdent() ==
CSSValueMinIntrinsic)
> +	   else if (intrinsicEnabled && primitiveValue->getIdent() ==
CSSValueMinIntrinsic)

Nit: I would nest ifs so you don't have to check intrinsicEnabled twice. It
might also be more readable.

> Source/WebCore/css/StyleBuilder.cpp:371
> +	   else if (fittedEnabled && primitiveValue->getIdent() ==
CSSValueWebkitMinContent)
> +	       setValue(styleResolver->style(), Length(MinContent));
> +	   else if (fittedEnabled && primitiveValue->getIdent() ==
CSSValueWebkitMaxContent)
> +	       setValue(styleResolver->style(), Length(MaxContent));
> +	   else if (fittedEnabled && primitiveValue->getIdent() ==
CSSValueWebkitFillAvailable)
> +	       setValue(styleResolver->style(), Length(FillAvailable));
> +	   else if (fittedEnabled && primitiveValue->getIdent() ==
CSSValueWebkitFitContent)
> +	       setValue(styleResolver->style(), Length(FitContent));

Nit: I would nest ifs so you don't have to check fittedEnabled twice.

> Source/WebCore/platform/Length.h:226
> +    // FIXME: This should probably be renamed, but I don't have a better
name.
> +    bool isIntrinsicOrAuto() const { return type() == Auto || isIntrinsic()
|| isFitted(); }
> +    bool isIntrinsic() const { return type() == Intrinsic || type() ==
MinIntrinsic; }
> +    bool isFitted() const { return type() == MinContent || type() ==
MaxContent || type() == FillAvailable || type() == FitContent; }

How about renaming Intrinsic and MinIntrinsic to LegacyIntrinsic and using
Intrinsic for the new values?

> LayoutTests/fast/css-writing-modes/style.css:2
> +body * {
> +    border: 5px solid red;

I would just inline the styles in the test and -expected.html files.  It will
make it more obvious that the -expected.html isn't using the new values you are
adding.

> LayoutTests/fast/css-writing-modes/style.css:37
> +    img where this overrides the intrinsic width to get regular block "auto"


Nit: End the sentence with a period.

> LayoutTests/fast/css-writing-modes/width.html:5
> +
> +<div id="container">
> +

Please add a sentence or two describing what the test is testing and how to
tell visually if it's passing or failing.