Summary: | Make RenderObject::containingBlock virtual for better speed and clarity | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Julien Chaffraix
2011-07-11 15:11:17 PDT
Created attachment 100366 [details]
Proposed virtualization.
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. Created attachment 100609 [details]
Patch for landing
Comment on attachment 100609 [details] Patch for landing Clearing flags on attachment: 100609 Committed r90882: <http://trac.webkit.org/changeset/90882> All reviewed patches have been landed. Closing bug. |