[webkit-reviews] review granted: [Bug 64318] Make RenderObject::containingBlock a virtual method : [Attachment 100366] Proposed virtualization.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 11 18:00:29 PDT 2011


Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 64318: Make RenderObject::containingBlock a virtual method
https://bugs.webkit.org/show_bug.cgi?id=64318

Attachment 100366: Proposed virtualization.
https://bugs.webkit.org/attachment.cgi?id=100366&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list