Bug 64318 – Make RenderObject::containingBlock virtual for better speed and clarity

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Looking at some table benchmark, this method is taking 5% of the time. However it could be split into more specialized methods which would cut the time taken to execute this one.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 100366 [details]
Proposed virtualization.

[Darin Adler]

Comment on attachment 100366 [details]
Proposed virtualization.

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

> Source/WebCore/ChangeLog:3
> +        Make RenderObject::containingBlock a virtual method

The title of the bug should talk about the motivation.

"Make RenderObject::containingBlock virtual for better speed and clarity"

The term “method” is not used in C++. It’s a virtual function.

> Source/WebCore/rendering/RenderObject.cpp:601
>  RenderBlock* RenderObject::containingBlock() const

Seems strange that this is a const member function but returns a non-const pointer to the other renderer.

> Source/WebCore/rendering/RenderTableCell.h:141
> +    virtual RenderBlock* containingBlock() const
> +    {
> +        if (parent() && section())
> +            return table();
> +        return 0;
> +    }

It is not good style to have the implementation of a virtual function in a header file like this. Instead the declaration should be here and the definition in the cpp file. It even hurts compile time to have it here.

Also, it’s better to have this override be private rather than public, because if someone actually has a RenderTableCell*, then they could do something more efficient than calling containingBlock. It’s a programming mistake to call containingBlock in a case like that.

> Source/WebCore/rendering/RenderView.h:59
> +    virtual RenderBlock* containingBlock() const
> +    {
> +        return const_cast<RenderView*>(this);
> +    }

Same comment about not inlining and making private.

[Julien Chaffraix]

Created attachment 100609 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 100609 [details]
Patch for landing

Clearing flags on attachment: 100609

Committed r90882: <http://trac.webkit.org/changeset/90882>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.