[webkit-reviews] review granted: [Bug 119979] REGRESSION(r127163): Respect clearance set on ancestors when placing floats : [Attachment 217066] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted Robert Hogan <robert at webkit.org>'s
request for review:
Bug 119979: REGRESSION(r127163): Respect clearance set on ancestors when
placing floats
https://bugs.webkit.org/show_bug.cgi?id=119979

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

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


Something to possibly consider in a follow-up patch would be to add
isSelfCollapsingBlock as a virtual function so that you don't have to ask
isRenderBlockFlow and then cast to ask isSelfCollapsingBlock... would be nice
to just be able to ask any child if they are self-collapsing block without
having to call other methods first.

> Source/WebCore/rendering/RenderBlockFlow.cpp:221
> +    if (parentHasFloats || (parentBlock->lowestFloatLogicalBottom() >
logicalTopOffset && prev && toRenderBlockFlow(prev)->isSelfCollapsingBlock()))

Minor quibble. I'd move prev && prev->isSelfCollapsingBlock before
parentBlock->lowestFloatLogicalBottom() > logicalTopOffset.

> Source/WebCore/rendering/RenderBlockFlow.cpp:714
> +    ASSERT(isSelfCollapsingBlock());

Good call.