Bug 63897 - [CSSRegions] Parse -webkit-content-order property
Summary: [CSSRegions] Parse -webkit-content-order property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2011-07-04 03:57 PDT by Mihnea Ovidenie
Modified: 2011-07-13 17:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (14.81 KB, patch)
2011-07-04 09:17 PDT, Mihnea Ovidenie
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch 2 (14.63 KB, patch)
2011-07-11 06:35 PDT, Mihnea Ovidenie
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch 3 (15.13 KB, patch)
2011-07-13 14:22 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2011-07-04 03:57:25 PDT
Add support for content-order property: http://dev.w3.org/csswg/css3-regions/
Comment 1 Mihnea Ovidenie 2011-07-04 09:17:58 PDT
Created attachment 99636 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-07-05 18:12:53 PDT
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.
Comment 3 Eric Seidel (no email) 2011-07-05 18:13:26 PDT
I know simon also likes seeing some of these CSS patches.
Comment 4 Dave Hyatt 2011-07-06 14:41:42 PDT
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.
Comment 5 Mihnea Ovidenie 2011-07-11 06:35:31 PDT
Created attachment 100290 [details]
Patch 2

Second version of the patch.
Comment 6 Dave Hyatt 2011-07-13 11:28:03 PDT
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()));
Comment 7 Mihnea Ovidenie 2011-07-13 14:22:35 PDT
Created attachment 100710 [details]
Patch 3

Reworked patch, add clamping and large values in test.
Comment 8 Dave Hyatt 2011-07-13 14:51:41 PDT
Comment on attachment 100710 [details]
Patch 3

r=me
Comment 9 WebKit Review Bot 2011-07-13 17:30:08 PDT
Comment on attachment 100710 [details]
Patch 3

Clearing flags on attachment: 100710

Committed r90966: <http://trac.webkit.org/changeset/90966>
Comment 10 WebKit Review Bot 2011-07-13 17:30:12 PDT
All reviewed patches have been landed.  Closing bug.