Bug 23857 – Knock 4 bytes off line boxes by eliminating m_height member

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

The height for CSS layout is invariant across all lines typically so doesn't need to have a separate member.

[Dave Hyatt]

Created attachment 27498 [details]
Patch

[Darin Adler]

Comment on attachment 27498 [details]
Patch

>  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.

[Dave Hyatt]

(In reply to comment #2)
> (From update of attachment 27498 [details] [review])
> >  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.
> 

Agreed.  Fixed.

> > +    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.
>

Agreed. Fixed.
 
> > +    ASSERT(isRootInlineBox());
> > +    return static_cast<const RootInlineBox*>(this);
> 
> You should make a toRootInlineBox casting macro instead of writing the assert
> by hand.
>

This is just a clone of a non-const form of the function.  While I completely agree with you, I don't want to mix cleanup work with this patch.  There are many places that do line box casts of all types, and we should add toXXX methods for all of those, but I don't want to mix that in with this patch.
 
> > +    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.
>

Agreed. Fixed.
 
> >      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.
>

Agreed, and moving this caught a place in SVG where they were doing just that.
 
> > +    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.
> 

No.  This one can't be private.

> > -    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()?
>

Yes. SVGInlineTextBox.
 
> > --- 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.
>

Can't be private.
 
> Same applies to other classes like RootInlineBox.
>

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

Unfortunately I can't really improve on this.  SVG subclasses RootInlineBox, InlineTextBox, and InlineFlowBox and supplies different heights.  SVG also subclasses RenderText and RenderInline.  The places that you are noticing that now use virtual height() are shared with SVG.

[Dave Hyatt]

Created attachment 27503 [details]
Patch addressing most of Darin's comments

[Dave Hyatt]

I built release and ran the PLT.  None of the height() functions even show up.  height() really is used more for event handling than anything else, and I don't think the virtual function calls are going to hurt at all there.

[Darin Adler]

Comment on attachment 27503 [details]
Patch addressing most of Darin's comments

r=me

[Mark Rowe (bdash)]

Did this get landed?

[David Levin]

Committed as r40802.

Resolving to move out of the commit queue.