Bug 106392 - Merge RenderObjectChildList::appendChildNode and insertChildNode
Summary: Merge RenderObjectChildList::appendChildNode and insertChildNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on: 106384
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-08 15:17 PST by Elliott Sprehn
Modified: 2013-01-16 17:33 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.33 KB, patch)
2013-01-08 15:26 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (7.93 KB, patch)
2013-01-16 13:44 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2013-01-08 15:17:29 PST
Merge RenderObjectChildList::appendChildNode and insertChildNode
Comment 1 Elliott Sprehn 2013-01-08 15:26:21 PST
Created attachment 181786 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-01-08 18:28:13 PST
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.
Comment 3 Elliott Sprehn 2013-01-09 01:47:27 PST
(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.
Comment 4 Abhishek Arya 2013-01-09 07:09:21 PST
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 :(
Comment 5 Elliott Sprehn 2013-01-09 14:25:00 PST
(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.
Comment 6 Abhishek Arya 2013-01-09 20:19:04 PST
(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.
Comment 7 Elliott Sprehn 2013-01-16 13:44:16 PST
Created attachment 183030 [details]
Patch
Comment 8 WebKit Review Bot 2013-01-16 17:33:34 PST
Comment on attachment 183030 [details]
Patch

Clearing flags on attachment: 183030

Committed r139940: <http://trac.webkit.org/changeset/139940>
Comment 9 WebKit Review Bot 2013-01-16 17:33:38 PST
All reviewed patches have been landed.  Closing bug.