[webkit-reviews] review granted: [Bug 122537] Move RenderObject::layout() to RenderElement. : [Attachment 213739] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 122537: Move RenderObject::layout() to RenderElement.
https://bugs.webkit.org/show_bug.cgi?id=122537

Attachment 213739: Patch
https://bugs.webkit.org/attachment.cgi?id=213739&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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?