Bug 95479

Created attachment 164778 [details] Initial patch Adding line bottom to segment and line state calculations.

*** Bug 97042 has been marked as a duplicate of this bug. ***

Comment on attachment 164778 [details] Initial patch Attachment 164778 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13903679 New failing tests: fast/exclusions/shape-inside/shape-inside-rounded-rectangle.html

(In reply to comment #1) > Created an attachment (id=164778) [details] > Initial patch > > Adding line bottom to segment and line state calculations. Source/WebCore/rendering/ExclusionRectangle.cpp:76 The patch for https://bugs.webkit.org/show_bug.cgi?id=96156 also includes the ExclusionRectangle change. Source/WebCore/rendering/RenderBlockLineLayout.cpp:85 (wrapShapeInfo && wrapShapeInfo->lineState() & WrapShapeInfo::LINE_INSIDE_SHAPE && wrapShapeInfo->hasSegments()) An inline function like bool wrapShapeInfoHasSegments(WrapShapeInfo*) might be a useful simplification. Also: not clear why you're checking the lineState() is needed, isn't hasSegments() enough? Source/WebCore/rendering/RenderBlockLineLayout.cpp:86,87 // All interior shape positions should have at least one segment, // unless they extend beyond the edges of the shape. Maybe "shape positions" isn't the right term here. I think that what you mean is that if both the top and bottom edges of a line are within a shape, then there should be at least one segment. However that's definitely not true for polygons, and it will not be true when ExclusionShape objects correspond to sets of primitive shapes. I wouldn't characterize these exceptions as "they extend beyond the edges of the shape". Maybe just letting the code speak for itself here would be enough. Source/WebCore/rendering/RenderBlockLineLayout.cpp:809,810 logicalLeft = max<float>(roundToInt(wrapShapeInfo->segments()[0].logicalLeft), logicalLeft); logicalRight = min<float>(floorToInt(wrapShapeInfo->segments()[0].logicalRight), logicalRight); A pair of inlines whose names clarified what the min/max round/floor int/float means in these expressions, might improve readability. Source/WebCore/rendering/WrapShapeInfo.h Maybe, instead of making the enums and lineState() public, it would be better to just provide a set of inline functions that cover the tests needed elsewhere? bool lineOverlapsShapeBoundsTop() bool lineOverlapsShapeBoundsBottom() bool lineIsWithinShapeBounds() etc..

Created attachment 164945 [details] Updated patch Incorporating Hans' feedback, and fixing the test failure. Line positions within a shape are no longer guaranteed to have line segments. Removing the appropriate comments and tests. Changing lineState tests to use boolean inlines, right now just lineOverlapsShapeBounds. Removing lineState. Going to leave the pixel snapping logicalLeft/Right code in place and file a bug for it (bug 97236).

Created attachment 165476 [details] Updating patch Merging with master branch

Comment on attachment 165476 [details] Updating patch View in context: https://bugs.webkit.org/attachment.cgi?id=165476&action=review > Source/WebCore/rendering/WrapShapeInfo.cpp:112 > + if (lineOverlapsShapeBounds()) { > ASSERT(m_shape); > - m_shape->getIncludedIntervals(lineTop, lineTop, m_segments); // FIXME: Bug 95479, workaround for now > + m_shape->getIncludedIntervals(lineTop, std::min(lineBottom, shapeLogicalBottom()), m_segments); So this will call getIncludedIntervals when the line intersects but is outside of the shape. Help me understand why?

Linking to bug 96125, which is the bug for correctly laying out lines that overflow the shape-inside. Overflow for shape-inside is under active specification. This patch allows the last line to overflow the shape-inside, but that behavior may need to change as the specification develops. See http://dev.w3.org/csswg/css3-exclusions/#shape-inside-property

Created attachment 165660 [details] Updating changelog Making the changelog more descriptive.

Comment on attachment 165660 [details] Updating changelog Apparently I'm a ChangeLog reviewer :) Please add a link to that spec on overflow to the ChangeLog as well so someone glancing can find this without reading the bug. Then I think we're good to go.

Created attachment 165667 [details] Adding links to changelog

Comment on attachment 165667 [details] Adding links to changelog Clearing flags on attachment: 165667 Committed r129590: <http://trac.webkit.org/changeset/129590>

All reviewed patches have been landed. Closing bug.