Bug 122537 – Move RenderObject::layout() to RenderElement.

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

[Andreas Kling]

RenderText should never layout(), so this is really a RenderElement concept.

[Andreas Kling]

Created attachment 213738 [details]
Patch

[Andreas Kling]

Created attachment 213739 [details]
Patch

Eh that was not at all this patch.

[WebKit Commit Bot]

Attachment 213739 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderElement.cpp', u'Source/WebCore/rendering/RenderElement.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderTable.cpp', u'Source/WebCore/rendering/RenderText.h', u'Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLRow.cpp', u'Source/WebCore/rendering/svg/SVGRenderSupport.cpp']" exit_code: 1
Source/WebCore/rendering/RenderElement.h:96:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 213739 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:458
>      while (child) {
> -        child->layoutIfNeeded();
> +        if (child->needsLayout())
> +            toRenderElement(child)->layout();
>          ASSERT(!child->needsLayout());
>          child = child->nextSibling();

RenderBoxes can have text children. The reason this works is that everyone who has them also overriders layout(). 

Rather than relying on this a more stylish solution would be to remove RenderBox::layout() completely and move the relevant portions code to the (probably) few subclasses that actually use it. I did this when moving paint().

> Source/WebCore/rendering/RenderElement.cpp:1088
> +void RenderElement::layout()
> +{
> +    StackStats::LayoutCheckPoint layoutCheckPoint;
> +    ASSERT(needsLayout());
> +    RenderObject* child = firstChild();
> +    while (child) {
> +        if (child->needsLayout())
> +            toRenderElement(child)->layout();
> +        ASSERT(!child->needsLayout());
> +        child = child->nextSibling();
> +    }
> +    clearNeedsLayout();
> +}

Same comment as for RenderBox. RenderElement::paint() is pure virtual. Maybe this can be too?

> Source/WebCore/rendering/RenderElement.h:96
> +    /* This function performs a layout only if one is needed. */
> +    void layoutIfNeeded() { if (needsLayout()) layout(); }

This comment adds no useful information and uses non-standard format too.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:275
> -            child->layout();
> +            toRenderElement(child)->layout();

How do we know this is a RenderElement?

[WebKit Commit Bot]

Comment on attachment 213739 [details]
Patch

Clearing flags on attachment: 213739

Committed r157222: <http://trac.webkit.org/changeset/157222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.