[webkit-reviews] review denied: [Bug 63897] [CSSRegions] Parse -webkit-content-order property : [Attachment 100290] Patch 2

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Mihnea Ovidenie <mihnea at adobe.com>'s
request for review:
Bug 63897: [CSSRegions] Parse -webkit-content-order property
https://bugs.webkit.org/show_bug.cgi?id=63897

Attachment 100290: Patch 2
https://bugs.webkit.org/attachment.cgi?id=100290&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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()));