Bug 64318

Summary: Make RenderObject::containingBlock virtual for better speed and clarity
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Severity: Normal CC: webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Proposed virtualization.
Patch for landing none

Description Julien Chaffraix 2011-07-11 15:11:17 PDT
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.
Comment 1 Julien Chaffraix 2011-07-11 15:16:37 PDT
Created attachment 100366 [details]
Proposed virtualization.
Comment 2 Darin Adler 2011-07-11 18:00:29 PDT
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.
Comment 3 Julien Chaffraix 2011-07-12 19:32:42 PDT
Created attachment 100609 [details]
Patch for landing
Comment 4 WebKit Review Bot 2011-07-12 20:30:50 PDT
Comment on attachment 100609 [details]
Patch for landing

Clearing flags on attachment: 100609

Committed r90882: <http://trac.webkit.org/changeset/90882>
Comment 5 WebKit Review Bot 2011-07-12 20:30:54 PDT
All reviewed patches have been landed.  Closing bug.