Bug 121771 – Remove RenderObjectChildList

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

[Antti Koivisto]

Most popular RenderElement subclasses have m_children member (>90% of instances on typical page). Child handling should move to RenderElement. This will remove need for virtual children() calls and simplify the code.

[Antti Koivisto]

Created attachment 212313 [details]
patch

[Andreas Kling]

Comment on attachment 212313 [details]
patch

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

> Source/WebCore/rendering/RenderBlock.cpp:610
> -    toBlock->children()->appendChildNode(toBlock, cloneBlock);
> +    insertChildInternal(cloneBlock, nullptr, NotifyChildren);

This looks wrong.

[Early Warning System Bot]

Comment on attachment 212313 [details]
patch

Attachment 212313 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1933079

[Early Warning System Bot]

Comment on attachment 212313 [details]
patch

Attachment 212313 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1834762

[Antti Koivisto]

Created attachment 212315 [details]
another

[Early Warning System Bot]

Comment on attachment 212315 [details]
another

Attachment 212315 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1873388

[Early Warning System Bot]

Comment on attachment 212315 [details]
another

Attachment 212315 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1979042

[Darin Adler]

Comment on attachment 212315 [details]
another

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

> Source/WebCore/rendering/RenderObject.h:267
> +    friend RenderElement;

This is the wrong syntax. Should be friend class RenderElement. Also, should go at the top of the class with the other friends. But it's really annoying that this friendship needs to exist.

[Darin Adler]

Comment on attachment 212315 [details]
another

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

> Source/WebCore/rendering/RenderElement.h:59
> +    enum NotifyChildrenType { NotifyChildren, DontNotifyChildren };
> +    void insertChildInternal(RenderObject*, RenderObject* beforeChild, NotifyChildrenType);
> +    void removeChildInternal(RenderObject*, NotifyChildrenType);

Why public instead of protected?

> Source/WebCore/rendering/RenderObject.cpp:1668
> +            block->insertChildInternal(this, 0, RenderElement::NotifyChildren);

nullptr

[Antti Koivisto]

(In reply to comment #9)
> Why public instead of protected?

There are several cases where these are called via pointers that are of different type than the class of the current function.

[Antti Koivisto]

https://trac.webkit.org/r156254

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121779

[Alexey Proskuryakov]

This caused assertion failures in two fast/regions tests: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156257%20(10332)/fast/regions/region-dynamic-after-before-crash-log.txt>, <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156257%20(10332)/fast/regions/region-generated-content-before-after-crash-log.txt>.

Antti is not available at at the moment, and there are two more regressions in the tree. Fixing everything is not an option for me, rolling out.

[Alexey Proskuryakov]

Comment on attachment 212315 [details]
another

Rolled out in <http://trac.webkit.org/changeset/156262>.

[Antti Koivisto]

Re-landed in https://trac.webkit.org/r156278 with an assertion fix.