[webkit-reviews] review denied: [Bug 117573] [CSS Shapes] Shape's content gets extra left offset when left-border is positive on the content box : [Attachment 206482] Initial patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Bear Travis <betravis at adobe.com>'s
request for review:
Bug 117573: [CSS Shapes] Shape's content gets extra left offset when
left-border is positive on the content box
https://bugs.webkit.org/show_bug.cgi?id=117573

Attachment 206482: Initial patch
https://bugs.webkit.org/attachment.cgi?id=206482&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=206482&action=review


r-

> Source/WebCore/rendering/RenderBlock.cpp:1439
> +LayoutSize RenderBlock::offsetFromShapeAncestorContainer(const RenderBlock*
container) const

Please rename this to logicalOffsetFromShapeAncestorContainer to reflect that
you're putting the y into the width() variable in vertical writing modes.

> Source/WebCore/rendering/RenderBlock.cpp:1442
> +    LayoutRect blockRect(0, 0, currentBlock->width(),
currentBlock->height());

This is borderBoxRect(), so you can initialize to that, e.g., LayoutRect
blockRect(currentBlock->borderBoxRect());

> Source/WebCore/rendering/RenderBlock.cpp:1450
> +	   LayoutPoint currentBlockLocation = currentBlock->location();
> +
> +	   blockRect.moveBy(currentBlockLocation);

This doesn't work when you cross writing mode boundaries. Remember that block
coordinates can be flipped. When you cross into writing modes with different
flips, you have to adjust accordingly. You'll want to test with mixed
vertical-rl and vertical-lr and/or horizontal-tb/horizontal-bt to see what I
mean.

> Source/WebCore/rendering/RenderBlock.cpp:2741
> +    LayoutSize childOffset = child->location() - oldRect.location();
> +    if (childOffset.width() && childRenderBlock && layoutShapeInsideInfo()
&& !childRenderBlock->shapeInsideInfo()) {
> +	   childRenderBlock->setNormalChildNeedsLayout(true);
> +	   childRenderBlock->markShapeInsideDescendantsForLayout();
> +	   childRenderBlock->layoutIfNeeded();
> +    }

Put this in a helper function. Also, why are you only checking width? Is the
intent to look for a change in logical left position? If so, you need to check
height() if vertical.

> Source/WebCore/rendering/RenderBlock.h:591
> +    LayoutSize offsetFromShapeAncestorContainer(const RenderBlock*
container) const;

logicalOffset...

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1688
> +    shapeInsideInfo->computeSegmentsForLine(lineLeft, lineTop, lineHeight);

Use a LayoutSize rather than two arguments here.

> Source/WebCore/rendering/shapes/ShapeInsideInfo.h:71
> +    bool computeSegmentsForLine(LayoutUnit lineLeft, LayoutUnit lineTop,
LayoutUnit lineHeight)

Why not combine lineLeft/lineTop into a LayoutSize offset?