[webkit-reviews] review denied: [Bug 121619] [CSS Shapes] add shape-margin support for image valued shapes : [Attachment 212995] Patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 121619: [CSS Shapes] add shape-margin support for image valued shapes
https://bugs.webkit.org/show_bug.cgi?id=121619

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

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=212995&action=review


Thanks! I think that another round will make it perfect :)

>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-image-margin-
002.html:36
> +	   -webkit-shape-margin: 50px;

We should have negative testing as well, so please add "-webkit-shape-margin:
0px" and "-webkit-shape-margin: -100px" as well as "-webkit-shape-margin:
-1000000px"

> Source/WebCore/rendering/shapes/RasterShape.cpp:55
> +    ASSERT(radius >= 0);

Use unsigned.

> Source/WebCore/rendering/shapes/RasterShape.cpp:56
> +    m_xIntercepts.resize(radius + 1);

Is there any limit defined for the radius?

> Source/WebCore/rendering/shapes/RasterShape.cpp:87
> +    ASSERT(m_intervalLists[y].size() <= 1);

You need a comment explaining what happened when this assert fails.

> Source/WebCore/rendering/shapes/RasterShape.cpp:233
> +	   intervalGenerator.set(y, intervalsAtY[0].x1(),
intervalsAtY.last().x2());

This is still not addressing the previous review comments, so it's not going to
work with non-floats exclusions that have empty spaces in the middle. 

Can you at least add a comment saying that we will need to revisit and rewrite
this function? Add a bug for it and reference it from the comments.

> Source/WebCore/rendering/shapes/RasterShape.cpp:247
> +    int marginBoundaryRadius = std::min(clampToInteger(shapeMargin()),
std::max(m_imageSize.width(), m_imageSize.height()));

Replace clampToInteger() with clampTo<unsigned>(). Look in "template<> inline
CSSPrimitiveValue::operator unsigned() const" for another example. Might be
usefull to add a function to MathExtras.h.

I suppose you need to ceil() the margin, so that 5.5px will be considered 6px
instead.