Bug 63897

Created attachment 99636 [details] Patch

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.

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

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.

Created attachment 100290 [details] Patch 2 Second version of the patch.

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()));

Created attachment 100710 [details] Patch 3 Reworked patch, add clamping and large values in test.

Comment on attachment 100710 [details] Patch 3 r=me

Comment on attachment 100710 [details] Patch 3 Clearing flags on attachment: 100710 Committed r90966: <http://trac.webkit.org/changeset/90966>

All reviewed patches have been landed. Closing bug.