Bug 121708 – [CSS Shapes] Determining if a line is inside of a shape should only happen in one place

• Component: CSS
• Status: RESOLVED FIXED

[Bem Jones-Bey]

Right now, we are checking if a line is inside a shape both in the ShapeInfo classes and in the Shape classes. And right now, they're not doing is the same thing all the time.

The expected behavior is that both lines and shapes are closed on the top and open on the bottom.

Also, for the purposes of shape-outside, a zero height shape should still affect a line that it intersects. (eg. shapeTop == lineTop).

[Hans Muller]

Created attachment 216710 [details]
Patch

The ShapeInfo::lineOverlapsShapeBounds() methods now delegate to the Shape object. The logic for the Shape overlap test is now the same for ShapeInsideInfo and ShapeOutsideInfo.

[Hans Muller]

Created attachment 216711 [details]
Patch

Resync'd

[Zoltan Horvath]

Comment on attachment 216711 [details]
Patch

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

I like the change. 2 comments from me:

> Source/WebCore/rendering/shapes/Shape.h:90
> +    bool lineOverlapsLayoutRect(LayoutUnit lineTop, LayoutUnit lineHeight, const LayoutRect& rect) const

The naming of this function is a bit confusing to me, because we usually don't include the typename in the function, but rather the functionality. What do you think about lineOverlapsRectangle or lineOverlapsBoundingBox?

> Source/WebCore/rendering/shapes/Shape.h:92
> +        return !rect.isEmpty() && ((lineTop < rect.maxY() && lineTop + lineHeight > rect.y()) || (!lineHeight && lineTop == rect.y()));

I would early return false for rect.isEmpty then the second part of the condition would be more straightforward in its own line.

[Hans Muller]

Created attachment 216809 [details]
Patch

Made the suggested changes.

[WebKit Commit Bot]

Comment on attachment 216809 [details]
Patch

Clearing flags on attachment: 216809

Committed r159205: <http://trac.webkit.org/changeset/159205>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.