[webkit-reviews] review denied: [Bug 89993] [CSS Exclusions] Enable css exclusions for multiple blocks per element : [Attachment 163241] Updating 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 89993: [CSS Exclusions] Enable css exclusions for multiple blocks per
element
https://bugs.webkit.org/show_bug.cgi?id=89993

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

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


Just some minor suggestions at this point. Make sure to follow up on the rename
of wrapShapeInfo, if it really is only about "inside".

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1313
> +	   absoluteLogicalTop = logicalTop();
> +	   // FIXME: If layout state is disabled, the offset will be incorrect
> +	   if (isHorizontalWritingMode())
> +	       absoluteLogicalTop +=
view()->layoutState()->layoutOffset().height();
> +	   else
> +	       absoluteLogicalTop +=
view()->layoutState()->layoutOffset().width();

Minor nitpick, but I think this would read better if you cache in a local.
Consider changing this block of code to:

LayoutSize layoutOffset = view()->layoutState()->layoutOffset();
absoluteLogicalTop = logicalTop() + (isHorizontalWritingMode() ?
layoutOffset.height() : layoutOffset.width());

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1317
> +    if (wrapShapeInfo && logicalHeight() + absoluteLogicalTop <
wrapShapeInfo->shapeLogicalTop())
> +	   setLogicalHeight(wrapShapeInfo->shapeLogicalTop() -
absoluteLogicalTop);

Can't this go inside the previous if statement that assigned to wrapShapeInfo?
Then it would be inside that if and just turn into:

if (logicalHeight() + absoluteLogicalTop < wrapShapeInfo->shapeLogicalTop())

since you already know wrapShapeInfo isn't null.