Bug 64318 - Make RenderObject::containingBlock virtual for better speed and clarity
Summary: Make RenderObject::containingBlock virtual for better speed and clarity
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 15:11 PDT by Julien Chaffraix
Modified: 2011-07-12 20:30 PDT (History)
1 user (show)

See Also:


Attachments
Proposed virtualization. (4.44 KB, patch)
2011-07-11 15:16 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (5.51 KB, patch)
2011-07-12 19:32 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.