Bug 157811 – Use RenderChildIterator more for traversing a renderer's children

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

[Chris Dumez]

Use RenderChildIterator more for traversal a renderer's children.

[Chris Dumez]

Created attachment 279165 [details]
Patch

[Chris Dumez]

Created attachment 279166 [details]
Patch

[Antti Koivisto]

Comment on attachment 279166 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:3471
> +        for (auto& box : childrenOfType<RenderBox>(*this)) {
> +            if (is<RenderText>(box) || is<RenderListMarker>(box))
> +                continue;

RenderBox (or RenderElement) is never a RenderText so the test is unnecessary. For DOM classes we have static asserts against at least some types of unnecessary tests. It might be good to add something like that at least for the RenderText case.

> Source/WebCore/rendering/RenderObject.cpp:175
> +    if (!is<RenderElement>(*this))
> +        return;
> +
> +    for (auto& child : childrenOfType<RenderObject>(downcast<RenderElement>(*this))) {

Probably an indication the code should move to RenderElement in the future.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:159
> +    for (auto& current : childrenOfType<RenderObject>(container)) {

Some of these could probably be tightened to RenderElement

[Chris Dumez]

Created attachment 279169 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279169 [details]
Patch

Clearing flags on attachment: 279169

Committed r201052: <http://trac.webkit.org/changeset/201052>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 279166 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:3471
>> +                continue;
> 
> RenderBox (or RenderElement) is never a RenderText so the test is unnecessary. For DOM classes we have static asserts against at least some types of unnecessary tests. It might be good to add something like that at least for the RenderText case.

I will update the static assertion in is<>() to catch these via https://bugs.webkit.org/show_bug.cgi?id=157817

[Darin Adler]

Comment on attachment 279165 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use RenderChildIterator more for traversal a renderer's children

traversal of a

> Source/WebCore/rendering/RenderFieldset.cpp:130
> +    for (auto& legend : childrenOfType<RenderElement>(*this)) {

Should be RenderBox.

> Source/WebCore/rendering/RenderFieldset.cpp:135
> +            return &downcast<RenderBox>(legend);

Not sure why this cast is safe if someone sets an unusual display style on an HTMLLegendElement, but even if it is OK, I see no value in doing the additional processing on the other RenderElement objects that are not RenderBox.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:269
> +        if (layoutSizeChanged && is<SVGElement>(child.node())) {

This adds a null check that was not in the old code.

[Chris Dumez]

Created attachment 279185 [details]
Fix Darin's findings

[Chris Dumez]

Created attachment 279187 [details]
Fix Darin's findings

[Chris Dumez]

Reopen to fix nits.

[WebKit Commit Bot]

Comment on attachment 279187 [details]
Fix Darin's findings

Clearing flags on attachment: 279187

Committed r201064: <http://trac.webkit.org/changeset/201064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.