[webkit-reviews] review denied: [Bug 98676] [CSS Exclusions] shape-outside on floats for polygon shapes : [Attachment 191038] Updated Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Bem Jones-Bey <bjonesbe at adobe.com>'s
request for review:
Bug 98676: [CSS Exclusions] shape-outside on floats for polygon shapes
https://bugs.webkit.org/show_bug.cgi?id=98676

Attachment 191038: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=191038&action=review

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


r- for the writing mode issue, since that looks wrong to me.

> Source/WebCore/rendering/ExclusionShapeInfo.h:92
> -    LayoutUnit shapeLogicalRight() const { return
computedShape()->shapeLogicalBoundingBox().y() + logicalLeftOffset(); }
> +    LayoutUnit shapeLogicalRight() const { return
computedShape()->shapeLogicalBoundingBox().maxX() + logicalLeftOffset(); }

LOL.

> Source/WebCore/rendering/RenderBlock.h:1143
> +	   const FloatingObject* lastFloat() const { return m_last; }

I think it is worth a comment explaining what this is.

> Source/WebCore/rendering/RenderBlock.h:1151
> +	   mutable const FloatingObject* m_last;

Ditto.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:181
> +	   shapeOutsideInfo->computeSegmentsForLine(m_block->logicalHeight() -
newFloat->y(), logicalHeightForLine(m_block, m_isFirstLine));

This looks wrong. I suspect you mean m_block->logicalTopForFloat(newFloat)
rather than newFloat->y().

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:188
> +	   if (shapeOutsideInfo)
> +	       newLeft += shapeOutsideInfo->logicalRightOffsetForLine();

This looks confusing to me. The only reason we're getting the right edge of the
float is that is where the left edge of the line begins (i.e., after the right
edge of the float). 

It reads very confusingly that you are calling a method named
logicalRightOffsetForLine. If it's a delta applied to the float's right edge,
then this method name seems very strange.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:199
> +#if ENABLE(CSS_EXCLUSIONS)
> +	   if (shapeOutsideInfo)
> +	       newRight += shapeOutsideInfo->logicalLeftOffsetForLine();
> +#endif

Same here. This seems very confusing.