[Webkit-unassigned] [Bug 38919] Add support for fit-content etc

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38919


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #147648|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #9 from Tony Chang <tony at chromium.org>  2012-06-15 13:27:40 PST ---
(From update of attachment 147648)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.