[webkit-reviews] review granted: [Bug 182762] [RenderTreeBuilder] Move RenderElement::takeChild() to RenderTreeBuilder : [Attachment 333760] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted zalan <zalan at apple.com>'s request
for review:
Bug 182762: [RenderTreeBuilder] Move RenderElement::takeChild() to
RenderTreeBuilder
https://bugs.webkit.org/show_bug.cgi?id=182762

Attachment 333760: Patch

https://bugs.webkit.org/attachment.cgi?id=333760&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 333760
  --> https://bugs.webkit.org/attachment.cgi?id=333760
Patch

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

> Source/WebCore/ChangeLog:9
> +	   This patch removes the remaining takeChild() related mutation logic
from the renderers.

Yay!

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:571
> +RenderPtr<RenderObject>
RenderTreeBuilder::takeChildFromRenderElement(RenderElement& parent,
RenderObject& child)

The order of the parameters does not match the order implied by the name of
this function. We should swap the parameters such that this function takes the
child as the first argument and the parent as the second.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:586
> +	   // We only repaint |oldChild| if we have a RenderLayer as its visual
overflow may not be tracked by its parent.

oldChild => child

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:602
> +    // If oldChild is the start or end of the selection, then clear the
selection to

Ditto.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:612
> +    // WARNING: There should be no code running between
willBeRemovedFromTree and the actual removal below.

willBeRemovedFromTree => willBeRemovedFromTree()

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:613
> +    // This is needed to avoid race conditions where willBeRemovedFromTree
would dirty the tree's structure

Ditto.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:614
> +    // and the code running here would force an untimely rebuilding, leaving
|oldChild| dangling.

oldChild => child

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:617
> +    // rendererRemovedFromTree walks the whole subtree. We can improve
performance

rendererRemovedFromTree => rendererRemovedFromTree()