Bug 122537

Summary: Move RenderObject::layout() to RenderElement.
Product: WebKit Reporter: Andreas Kling <kling>
Component: Layout and RenderingAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbarton, d-r, esprehn+autocc, fmalita, fred.wang, glenn, gyuyoung.kim, kondapallykalyan, mrobinson, pdr, schenney
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Andreas Kling 2013-10-08 18:04:57 PDT
RenderText should never layout(), so this is really a RenderElement concept.
Comment 1 Andreas Kling 2013-10-08 18:06:16 PDT
Created attachment 213738 [details]
Patch
Comment 2 Andreas Kling 2013-10-08 18:07:15 PDT
Created attachment 213739 [details]
Patch

Eh that was not at all this patch.
Comment 3 WebKit Commit Bot 2013-10-08 18:09:54 PDT
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.
Comment 4 Antti Koivisto 2013-10-08 18:16:53 PDT
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?
Comment 5 WebKit Commit Bot 2013-10-10 06:53:59 PDT
Comment on attachment 213739 [details]
Patch

Clearing flags on attachment: 213739

Committed r157222: <http://trac.webkit.org/changeset/157222>
Comment 6 WebKit Commit Bot 2013-10-10 06:54:02 PDT
All reviewed patches have been landed.  Closing bug.