[webkit-reviews] review denied: [Bug 115687] Need to Remove Anonymous Wrappers When All Children Become Inline : [Attachment 201751] 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 201751: Patch
https://bugs.webkit.org/attachment.cgi?id=201751&action=review

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


r-

> Source/WebCore/rendering/RenderObject.cpp:1680
> +void RenderObject::removeAnonymousWrappersFromLineIfNecessary()
> +{

It sure looks like you only intend to run this code for inlines, but I see no
check for this.

I'd also rename it to removeAnonymousWrappersForInlinesIfNecessary.

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

Sorry, I do not understand this code at all. You'll have to explain it. It
seems like your parent() could just be some random RenderInline, so why would
you want to look at an inline's kids here? I am not following this.

> Source/WebCore/rendering/RenderObject.cpp:1698
> +    RenderBlock* parentBlock = toRenderBlock(parent());
> +    for (unsigned i = 0; i < anonymousBlocks.size(); i++)
> +	   parentBlock->collapseAnonymousBoxChild(parentBlock,
toRenderBlock(anonymousBlocks[i]));

How do you know your parent is a block? Your s_noLongerAffectsParentBlock
boolean is set to true when your parent is a RenderInline().