[webkit-reviews] review denied: [Bug 115687] Need to Remove Anonymous Wrappers When All Children Become Inline : [Attachment 201975] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 115687: Need to Remove Anonymous Wrappers When All Children Become Inline
https://bugs.webkit.org/show_bug.cgi?id=115687

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

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


r-

> Source/WebCore/rendering/RenderObject.cpp:125
> +bool RenderObject::s_noLongerAffectsParentBlock = false;

You should tighten this since you only want to run it for inlines.
s_inlineBecameOutOfFlow would be a better name.

> Source/WebCore/rendering/RenderObject.cpp:1690
> +    Vector<RenderObject*> anonymousBlocks;

Not really following why you have to collect stuff into a Vector.

> Source/WebCore/rendering/RenderObject.cpp:1698
> +    while (curr) {
> +	   if (curr->isAnonymousBlock() &&
!toRenderBlock(curr)->isAnonymousBlockContinuation())
> +	       anonymousBlocks.append(curr);
> +	   else if (!curr->style()->isFloating() &&
!curr->style()->hasOutOfFlowPosition())
> +	       return;
> +	   curr = curr->nextSibling();
> +    }

I think this code has the potential to kill a pre or post anonymous block
created by continuations. You can't get rid of those, even if they become
empty.

It seems to me that you're better off writing the loop like this:

RenderObject* curr = parent()->firstChild();
while (curr && (curr->isAnonymousBlock() &&
!toRenderBlock(curr)->isAnonymousBlockContinuation()) ||
(curr->style()->isFloating() || curr->style()->hasOutOfFlowPosition()))
    cur = curr->nextSibling();
if (curr) return;

Then you just walk the render tree children again and do the collapse.

> Source/WebCore/rendering/RenderObject.cpp:1910
> +	   s_noLongerAffectsParentBlock = ((!isFloating() &&
newStyle->isFloating()) || (!isOutOfFlowPositioned() &&
newStyle->hasOutOfFlowPosition()))
> +	       && parent() && parent()->isRenderBlock();

Add isInline() && at the start of this, since you don't want to run this for
blocks.