Summary: | Merge RenderObjectChildList::appendChildNode and insertChildNode | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Elliott Sprehn <esprehn> | ||||||
Component: | New Bugs | Assignee: | Elliott Sprehn <esprehn> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | eric, inferno, ojan.autocc, ojan, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 106384 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Elliott Sprehn
2013-01-08 15:17:29 PST
Created attachment 181786 [details]
Patch
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.
(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 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 :( (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. (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. Created attachment 183030 [details]
Patch
Comment on attachment 183030 [details] Patch Clearing flags on attachment: 183030 Committed r139940: <http://trac.webkit.org/changeset/139940> All reviewed patches have been landed. Closing bug. |