Bug 130350 – [CSS Shapes] clamp RasterShape shapeMargin to reference box size

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hans Muller]

Image valued shapes are represented internally as a vector of above threshold horizontal "runs"; just the start,end column for the above threshold pixels on a row. The vector's size is currently the image's height plus the user supplied shapeMargin length.  The limiting case for the run-vector's size is just one above threshold pixel one the first or last image row. In that case the shapeMargin can be limited to the shape's reference box height, since the shape is clipped to the reference box after it's expanded by shapeMargin.

Currently shapeMargin is incorrectly clamped to the image size in RasterShape::marginIntervals()

int marginBoundaryRadius = std::min(clampToInteger(ceil(shapeMargin())), std::max(m_imageSize.width(), m_imageSize.height()));

[Hans Muller]

Note also: once this is fixed, the m_imageSize member can be removed.

[Hans Muller]

Created attachment 227304 [details]
Patch

Corrected the way the maximum shape-margin value was computed for image valued shapes. The limiting case is an image with just one above threshold pixel. The largest possible value for shapeMargin is the radius of a circle that encloses the size of the shape-outside element's margin-box, since the shape-outside's boundary is clipped to the margin-box. The radius is just sqrt(2) * the margin-box's largest dimension.

Also cleaned up up a few poorly named parameters.

[Dean Jackson]

Comment on attachment 227304 [details]
Patch

You're missing ChangeLogs. Looks ok otherwise though.

[Hans Muller]

Created attachment 227345 [details]
Patch

Added the ChangeLogs.  Sorry about that.

[Dean Jackson]

Comment on attachment 227345 [details]
Patch

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

> Source/WebCore/rendering/shapes/Shape.h:71
> -    static PassOwnPtr<Shape> createRasterShape(Image*, float threshold, const LayoutRect& drawRect, const LayoutRect& clipRect, WritingMode, Length margin, Length padding);
> +    static PassOwnPtr<Shape> createRasterShape(Image*, float threshold, const LayoutRect& imageRect, const LayoutRect& marginRect, WritingMode, Length margin, Length padding);

Interesting that Shape.cpp already had that change.

[Hans Muller]

(In reply to comment #5)
> (From update of attachment 227345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227345&action=review
> 
> > Source/WebCore/rendering/shapes/Shape.h:71
> > -    static PassOwnPtr<Shape> createRasterShape(Image*, float threshold, const LayoutRect& drawRect, const LayoutRect& clipRect, WritingMode, Length margin, Length padding);
> > +    static PassOwnPtr<Shape> createRasterShape(Image*, float threshold, const LayoutRect& imageRect, const LayoutRect& marginRect, WritingMode, Length margin, Length padding);
> 
> Interesting that Shape.cpp already had that change.

Yes.  I had trouble deciding what to call this parameter and the code reflected as much. It's the rect that the overall shape is clipped to (so clipRect) and it's defined by the shape element's margin-box (so marginRect), which can be a little confusing since it's shape-margin that can grow the shape into the margin-box.  The names still aren't great, but now at least they're consistent.

[WebKit Commit Bot]

Comment on attachment 227345 [details]
Patch

Clearing flags on attachment: 227345

Committed r166019: <http://trac.webkit.org/changeset/166019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.