Bug 106392 – Merge RenderObjectChildList::appendChildNode and insertChildNode

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Merge RenderObjectChildList::appendChildNode and insertChildNode

[Elliott Sprehn]

Created attachment 181786 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 181786 [details]
Patch

It's a bit odd that insert with 0 turns into an append, but I see your'e not changing that behavior.  So this looks OK to me.

[Elliott Sprehn]

(In reply to comment #2)
> (From update of attachment 181786 [details])
> It's a bit odd that insert with 0 turns into an append, but I see your'e not changing that behavior.  So this looks OK to me.

It was probably done because that's how other web platform methods work: https://developer.mozilla.org/en-US/docs/DOM/Node.insertBefore which is ContainerNode::insertBefore.

[Abhishek Arya]

Comment on attachment 181786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181786&action=review

> Source/WebCore/rendering/RenderObjectChildList.cpp:112
> +void RenderObjectChildList::insertChildNode(RenderObject* owner, RenderObject* newChild, RenderObject* before, bool notifyRenderer)

nit: We should use beforeChild here, instead of before which is generally used in generated content.

> Source/WebCore/rendering/RenderObjectChildList.cpp:117
> +    while (before && before->parent() != owner && before->parent()->isAnonymousBlock())

before->parent()->isAnonymousBlock() does not cover all scenarios. It does not cover anonymous inline-blocks. See first testcase in https://bugs.webkit.org/show_bug.cgi?id=106384. I did cleanup a lot of this code on RenderBlock side (actually i wished we share this code) -

void RenderBlock::addChildIgnoringAnonymousColumnBlocks(RenderObject* newChild, RenderObject* beforeChild)
{
    if (beforeChild && beforeChild->parent() != this) {
.....
.....
}

Here i think we should skip before->parent()->isAnonymousBlock() check, since we should always have beforeChild->parent() = this no matter what, otherwise, we will have security crashes :(

[Elliott Sprehn]

(In reply to comment #4)
> (From update of attachment 181786 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181786&action=review
> 
> > Source/WebCore/rendering/RenderObjectChildList.cpp:112
> > +void RenderObjectChildList::insertChildNode(RenderObject* owner, RenderObject* newChild, RenderObject* before, bool notifyRenderer)
> 
> nit: We should use beforeChild here, instead of before which is generally used in generated content.

I had stopped using "beforeChild" since it doesn't actually have to be a child, it can be any descendant and we then walk up from it to find a child. I see we use this naming in other parts of the render tree so I'll change it back.

> 
> > Source/WebCore/rendering/RenderObjectChildList.cpp:117
> > +    while (before && before->parent() != owner && before->parent()->isAnonymousBlock())
> 
> before->parent()->isAnonymousBlock() does not cover all scenarios. It does not cover anonymous inline-blocks. See first testcase in https://bugs.webkit.org/show_bug.cgi?id=106384. I did cleanup a lot of this code on RenderBlock side (actually i wished we share this code) -
> 
> void RenderBlock::addChildIgnoringAnonymousColumnBlocks(RenderObject* newChild, RenderObject* beforeChild)
> {
>     if (beforeChild && beforeChild->parent() != this) {
> .....
> .....
> }
> 
> Here i think we should skip before->parent()->isAnonymousBlock() check, since we should always have beforeChild->parent() = this no matter what, otherwise, we will have security crashes :(

Hmm, perhaps I should do that in a different patch before landing this. I don't really want to mix a 1 line bug fix in with this refactoring.

[Abhishek Arya]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 181786 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181786&action=review
> > 
> > > Source/WebCore/rendering/RenderObjectChildList.cpp:112
> > > +void RenderObjectChildList::insertChildNode(RenderObject* owner, RenderObject* newChild, RenderObject* before, bool notifyRenderer)
> > 
> > nit: We should use beforeChild here, instead of before which is generally used in generated content.
> 
> I had stopped using "beforeChild" since it doesn't actually have to be a child, it can be any descendant and we then walk up from it to find a child. I see we use this naming in other parts of the render tree so I'll change it back.
> 
> > 
> > > Source/WebCore/rendering/RenderObjectChildList.cpp:117
> > > +    while (before && before->parent() != owner && before->parent()->isAnonymousBlock())
> > 
> > before->parent()->isAnonymousBlock() does not cover all scenarios. It does not cover anonymous inline-blocks. See first testcase in https://bugs.webkit.org/show_bug.cgi?id=106384. I did cleanup a lot of this code on RenderBlock side (actually i wished we share this code) -
> > 
> > void RenderBlock::addChildIgnoringAnonymousColumnBlocks(RenderObject* newChild, RenderObject* beforeChild)
> > {
> >     if (beforeChild && beforeChild->parent() != this) {
> > .....
> > .....
> > }
> > 
> > Here i think we should skip before->parent()->isAnonymousBlock() check, since we should always have beforeChild->parent() = this no matter what, otherwise, we will have security crashes :(
> 
> Hmm, perhaps I should do that in a different patch before landing this. I don't really want to mix a 1 line bug fix in with this refactoring.

Sounds good. If you can fix that 1 line as part of https://bugs.webkit.org/show_bug.cgi?id=106384, that would be great. That is a major crasher right now on ClusterFuzz and I don't want that change to conflict with your refactoring.

[Elliott Sprehn]

Created attachment 183030 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183030 [details]
Patch

Clearing flags on attachment: 183030

Committed r139940: <http://trac.webkit.org/changeset/139940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.