Bug 182762 - [RenderTreeBuilder] Move RenderElement::takeChild() to RenderTreeBuilder
Summary: [RenderTreeBuilder] Move RenderElement::takeChild() to RenderTreeBuilder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-13 19:19 PST by zalan
Modified: 2018-02-14 09:11 PST (History)
7 users (show)

See Also:


Attachments
Patch (24.40 KB, patch)
2018-02-13 19:27 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (24.39 KB, patch)
2018-02-14 08:35 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2018-02-13 19:19:27 PST
It means no more takeChild() mutation logic in the renderers.
Comment 1 Radar WebKit Bug Importer 2018-02-13 19:19:54 PST
<rdar://problem/37523756>
Comment 2 zalan 2018-02-13 19:27:04 PST
Created attachment 333760 [details]
Patch
Comment 3 Daniel Bates 2018-02-13 23:09:39 PST
Comment on attachment 333760 [details]
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()
Comment 4 zalan 2018-02-14 08:35:23 PST
Created attachment 333803 [details]
Patch
Comment 5 zalan 2018-02-14 08:37:24 PST
> > 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.
Indeed. Since, at this point all the takeChild*() functions have this order, I'd rather address this issue in a separate patch.
Comment 6 WebKit Commit Bot 2018-02-14 09:11:16 PST
Comment on attachment 333803 [details]
Patch

Clearing flags on attachment: 333803

Committed r228464: <https://trac.webkit.org/changeset/228464>
Comment 7 WebKit Commit Bot 2018-02-14 09:11:18 PST
All reviewed patches have been landed.  Closing bug.