[webkit-reviews] review denied: [Bug 23857] Knock 4 bytes off line boxes by eliminating m_height member : [Attachment 27498] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dave Hyatt <hyatt at apple.com>'s request
for review:
Bug 23857: Knock 4 bytes off line boxes by eliminating m_height member
https://bugs.webkit.org/show_bug.cgi?id=23857

Attachment 27498: Patch
https://bugs.webkit.org/attachment.cgi?id=27498&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>  class EllipsisBox : public InlineBox {
>  public:
>      EllipsisBox(RenderObject* obj, const AtomicString& ellipsisStr,
InlineFlowBox* parent,
> -		   int width, int y, int height, int baseline, bool firstLine,
InlineBox* markupBox)
> -	   : InlineBox(obj, 0, y, width, height, baseline, firstLine, true,
false, false, 0, 0, parent)
> +		   int width, int height, int y, int baseline, bool firstLine,
InlineBox* markupBox)
> +	   : InlineBox(obj, 0, y, width, baseline, firstLine, true, false,
false, 0, 0, parent)
> +	   , m_height(height)
>	   , m_str(ellipsisStr)
>	   , m_markupBox(markupBox)
>      {
>      }
>  
> +    virtual int height() const { return m_height; }

This is the perfect example of a virtual function that should be private. If
anyone ever calls height() on a pointer knowing that it's an EllipsisBox, we
don't want them to do that -- we'll want to add a faster version that just
returns m_height. So you make the virtual function private to make sure nobody
makes that mistake. The function in the base class is still public.

> +    ASSERT(m_object->isBox());
> +    return toRenderBox(m_object)->height();

The assertion here is redundant. Just sit back and relax and let toRenderBox
handle the assertions for you.

> +    ASSERT(isRootInlineBox());
> +    return static_cast<const RootInlineBox*>(this);

You should make a toRootInlineBox casting macro instead of writing the assert
by hand.

> +    const InlineFlowBox* box = static_cast<const InlineFlowBox*>(this);
> +    result += box->boxModelObject()->borderTop() +
box->boxModelObject()->paddingTop() +
> +		 box->boxModelObject()->borderBottom() +
box->boxModelObject()->paddingBottom();

Seems to me that putting boxModelObject() into a local variable with a short
name (renderer, perhaps) would make this fit all on one line and be easier to
read.

>      virtual ~InlineFlowBox();
>  #endif
>  
> -    virtual bool isInlineFlowBox() { return true; }
> +    virtual bool isInlineFlowBox() const { return true; }

This should be private. If anyone ever calls it on something and already knows
it's an InlineFlowBox then it's a bug.

> +    virtual int height() const;

This too, maybe. Why pay for a virtual function dispatch if some particular
caller doesn't need to? Lets have the compiler notice that for us.

> -    IntRect rect(tx + m_x, ty + m_y, m_width, m_height);
> +    IntRect rect(tx + m_x, ty + m_y, m_width, height());

Seems lame to pay for a virtual function call here. Are there any classes
derived from InlineTextBox that need to override height()?

> --- rendering/InlineTextBox.h (revision 40769)
> +++ rendering/InlineTextBox.h (working copy)
> @@ -50,6 +50,8 @@ public:
>      InlineTextBox* nextTextBox() const { return
static_cast<InlineTextBox*>(nextLineBox()); }
>      InlineTextBox* prevTextBox() const { return
static_cast<InlineTextBox*>(prevLineBox()); }
>  
> +    virtual int height() const;

Same comment about private inline functions.

Same applies to other classes like RootInlineBox.

I'm going to say review- because I'd like to see another patch that calls
virtual functions a bit less often.