[CSSRegions] Parse -webkit-content-order property

• Component: CSS
• Status: RESOLVED FIXED

[Mihnea Ovidenie]

Add support for content-order property: http://dev.w3.org/csswg/css3-regions/

[Mihnea Ovidenie]

Created attachment 99636 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 99636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99636&action=review

Seems reasonable, but I'd like tony/luke to comment.

> LayoutTests/fast/regions/webkit-content-order-parsing-expected.txt:12
> +PASS testCSSText("-webkit-content-order: 1") is "1"
> +PASS testCSSText("-webkit-content-order: 0") is "0"
> +PASS testCSSText("-webkit-content-order: -1") is "-1"
> +PASS testCSSText("-webkit-content-order: 12.5") is ""

Do we need to test boundaries?  Like INT_MAX?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1663
> +            return primitiveValueCache->createValue(style->regionIndex(), CSSPrimitiveValue::CSS_NUMBER);

I thought Tony Chang recently added some wrappers for accessing the primativeValueCache?

> Source/WebCore/css/CSSStyleSelector.cpp:4883
> +    case CSSPropertyWebkitContentOrder:
> +        if (isInitial) {
> +            HANDLE_INITIAL_COND(CSSPropertyWebkitContentOrder, RegionIndex);
> +            return;
> +        }
> +        if (isInherit) {
> +            m_style->setRegionIndex(RenderStyle::initialRegionIndex());
> +            return;
> +        }
> +        if (primitiveValue)
> +            m_style->setRegionIndex(primitiveValue->getIntValue());

Luke should comment if this is the correct modern syntax.

> Source/WebCore/rendering/style/RenderStyle.cpp:348
> +        if (rareNonInheritedData->m_regionIndex != other->rareNonInheritedData->m_regionIndex)
> +            return StyleDifferenceLayout;

You might have put this in the if above.  Not sure it really matters.

[Eric Seidel (no email)]

I know simon also likes seeing some of these CSS patches.

[Dave Hyatt]

Comment on attachment 99636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99636&action=review

Minor nits.

> LayoutTests/ChangeLog:13
> +        ():
> +        (testComputedStyle):
> +        (testNotInherited):

Not sure why the changelog did this, but you can just remove these lines.

> Source/WebCore/css/CSSStyleSelector.cpp:4882
> +        if (primitiveValue)

Confused why HANDLE_INHERIT_AND_INITIAL_AND_PRIMITIVE can't be used here.

> Source/WebCore/rendering/style/RenderStyle.h:1099
> +    void setRegionIndex(int i) { SET_VAR(rareNonInheritedData, m_regionIndex, i); }

Change "i" to "index" please. We don't like single-letter variable names for parameters.

[Mihnea Ovidenie]

Created attachment 100290 [details]
Patch 2

Second version of the patch.

[Dave Hyatt]

Comment on attachment 100290 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=100290&action=review

Looks good, although I'd like to see a large index test and the added clamping, since this is so similar to z-index. I want to make sure we have the same clamping protection in place.

> Source/WebCore/css/CSSStyleSelector.cpp:4897
> +    case CSSPropertyWebkitContentOrder:
> +        HANDLE_INHERIT_AND_INITIAL_AND_PRIMITIVE(regionIndex, RegionIndex);
> +        return;

I think since this is so similar to z-index, we'll need to worry about people using ridiculously large content-order values. You should take a look at the z-index handling in CSSStyleSelector.cpp and mimic it. In particular I think you need a clampToInteger call.

m_style->setZIndex(clampToInteger(primitiveValue->getDoubleValue()));

[Mihnea Ovidenie]

Created attachment 100710 [details]
Patch 3

Reworked patch, add clamping and large values in test.

[Dave Hyatt]

Comment on attachment 100710 [details]
Patch 3

r=me

[WebKit Review Bot]

Comment on attachment 100710 [details]
Patch 3

Clearing flags on attachment: 100710

Committed r90966: <http://trac.webkit.org/changeset/90966>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.