[webkit-reviews] review granted: [Bug 102846] [CSS Shapes] Use the float height to determine position in shape-inside : [Attachment 211916] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zoltan Horvath <zoltan at webkit.org>'s
request for review:
Bug 102846: [CSS Shapes] Use the float height to determine position in
shape-inside
https://bugs.webkit.org/show_bug.cgi?id=102846

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review


> Source/WebCore/rendering/LineWidth.cpp:210
> +void LineWidth::updateCurrentLineSegment()
> +{
> +    if (ShapeInsideInfo* shapeInsideInfo = m_block.layoutShapeInsideInfo())
> +	   m_segment = shapeInsideInfo->currentSegment();
> +}

Does this need to go inside #if ENABLE(CSS_SHAPES)?

> Source/WebCore/rendering/RenderBlock.cpp:3387
> +	   // FIXME Bug 102949: Add support for shapes with multipe segments.

multiple rather than multipe

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2580
> +    bool lineOverlapsWithFloat = lastFloatFromPreviousLine->y() <
(block->logicalHeight() + lineLogicalHeight) && (block->logicalHeight() <
lastFloatFromPreviousLine->maxY());

Parentheses here make this expression confusing. I, at least, would be able to
read it bette without those parens. If you really do want parens, I would
suggest one more set around the expression before the &&.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2587
> +    if ((block->width() - lastFloatFromPreviousLine->maxX()) < minWidth)

Again, I find the parentheses make it harder to read this expression.