Bug 79319 – Cleanup RenderBlock::moveChildrenTo

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

Seen during a review, the function is pretty difficult to read but also has some redundant code hidden in it.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 128376 [details]
Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo.

[Abhishek Arya]

(In reply to comment #1)
> Created an attachment (id=128376) [details]
> Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo.

Yay! now i can just fix stuff in central RenderBlock::moveChildTo

[Eric Seidel (no email)]

Comment on attachment 128376 [details]
Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo.

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

I wonder if this will have a performance effect.

> Source/WebCore/rendering/RenderBlock.cpp:992
> +    for (RenderObject* child = startChild; child && child != endChild; ) {
> +        // Save our next sibling as moveChildTo will clear it.
> +        RenderObject* nextSibling = child->nextSibling();
> +        moveChildTo(toBlock, child, beforeChild, fullRemoveInsert);
> +        child = nextSibling;
>      }

Just to make sure... there are no mutation events (or other JavaScript) run by moveChildTo such that we would need to hold a RefPtr for child?

[Abhishek Arya]

(In reply to comment #3)
> (From update of attachment 128376 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128376&action=review
> 
> I wonder if this will have a performance effect.
> 
> > Source/WebCore/rendering/RenderBlock.cpp:992
> > +    for (RenderObject* child = startChild; child && child != endChild; ) {
> > +        // Save our next sibling as moveChildTo will clear it.
> > +        RenderObject* nextSibling = child->nextSibling();
> > +        moveChildTo(toBlock, child, beforeChild, fullRemoveInsert);
> > +        child = nextSibling;
> >      }
> 
> Just to make sure... there are no mutation events (or other JavaScript) run by moveChildTo such that we would need to hold a RefPtr for child?

No Refptrs in the rendering world :(

[Julien Chaffraix]

Comment on attachment 128376 [details]
Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo.

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

> I wonder if this will have a performance effect.

I was wondering that too. Some archaeology did not show the split was done for performance so we should be fine. In case, we can always inline moveChildTo.

>>> Source/WebCore/rendering/RenderBlock.cpp:992
>>>      }
>> 
>> Just to make sure... there are no mutation events (or other JavaScript) run by moveChildTo such that we would need to hold a RefPtr for child?
> 
> No Refptrs in the rendering world :(

Indeed, but more importantly, the operations are atomic from a JS perspective.

[WebKit Review Bot]

Comment on attachment 128376 [details]
Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo.

Clearing flags on attachment: 128376

Committed r108645: <http://trac.webkit.org/changeset/108645>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.