Bug 23522 – Use checked casts for render tree

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Darin Adler]

We have a checked cast for RenderBox now, but we want to do that for other render tree types too.

[Darin Adler]

Here are stats on how much static_cast is used:

  73 RenderText
  44 RenderBlock
  28 RenderImage
  27 RenderView
  26 RenderTextControl
  24 RenderTableSection
  24 RenderTableCell
  21 RenderWidget
  17 RenderFlow
  11 RenderScrollbar
  10 RenderListItem
   9 RenderTable
   8 RenderSlider
   8 RenderListBox
   7 RenderTextControlSingleLine
   5 RenderTableRow
   5 RenderMenuList
   5 RenderListMarker
   4 RenderTableCol
   4 RenderFrameSet
   4 RenderFileUploadControl
   3 RenderSVGContainer
   3 RenderPartObject
   3 RenderInline
   3 RenderBox
   2 RenderPart
   2 RenderObject
   2 RenderMedia
   2 RenderArenaDebugHeader
   1 RenderVideo
   1 RenderTextControlMultiLine
   1 RenderSVGViewportContainer
   1 RenderSVGTextPath
   1 RenderSVGText
   1 RenderSVGRoot
   1 RenderSVGImage
   1 RenderMediaControlShadowRoot
   1 RenderHTMLCanvas
   1 RenderFrame
   1 RenderFieldset
   1 RenderCounter
   1 RenderContainer
   1 RenderButton
   1 RenderApplet

I used this line to compute that:

    grep-r 'static_cast *< *Render\w* *\* *>' .h .cpp .mm | perl -pe 's/^.*static_cast *< *(Render\w+).*$/\1/' | sort | uniq -c | sort -r

Where grep-r is a script I use to search source tree that uses perl.

[Darin Adler]

Created attachment 27007 [details]
step 1 -- RenderText

[Darin Adler]

Comment on attachment 27007 [details]
step 1 -- RenderText

Landed as trac.webkit.org/changeset/40229 so clearing review flag.

[Darin Adler]

Created attachment 27036 [details]
work in progress

[Darin Adler]

Hyatt and Sam are both working on this.

[Darin Adler]

Created attachment 33481 [details]
Some other work in progress

[Darin Adler]

Created attachment 33729 [details]
Next step: Includes toRenderWidget

[David Levin]

Comment on attachment 33729 [details]
Next step: Includes toRenderWidget

Verified that it is equivalent (with the added bonus of error checking).

I wish there was a way to give compile errors for instances of "static_cast<RenderWidget*>()" to prevent it from being added again (automatically), but I couldn't think of any way to do this.

[Adam Barth]

(In reply to comment #8)
> I wish there was a way to give compile errors for instances of
> "static_cast<RenderWidget*>()" to prevent it from being added again
> (automatically), but I couldn't think of any way to do this.

I wonder if we can provide a specialized template for that with a COMPILE_ASSERT(false) in it.  Not sure if that works with static_cast.

[Darin Adler]

Comment on attachment 33729 [details]
Next step: Includes toRenderWidget

Landed, clearing review flag.

[Darin Adler]

Created attachment 33821 [details]
Next step: Includes toRenderTable*

[David Levin]

Comment on attachment 33821 [details]
Next step: Includes toRenderTable*


> Index: WebCore/page/Frame.cpp
> ===================================================================
>      if (cellRenderer && cellRenderer->isTableCell()) {
> -        RenderTableCell* cellAboveRenderer = cellRenderer->table()->cellAbove(cellRenderer);
> +        RenderTableCell* cellAboveRenderer = toRenderTableCell(cellRenderer)->table()->cellAbove(toRenderTableCell(cellRenderer));

Although it is no runtime expense, it is harder to read with two "toRenderTableCell(cellRenderer)" in the same line.

Consider a local variable to avoid this.  For example,
        RenderTableCell* tableCell = toRenderTableCell(cellRenderer);
        RenderTableCell* cellAboveRenderer = tableCell->table()->cellAbove(tableCell);


> Index: WebCore/page/mac/FrameMac.mm
> ===================================================================
>      if (cellRenderer && cellRenderer->isTableCell()) {
> -        RenderTableCell* cellAboveRenderer = cellRenderer->table()->cellAbove(cellRenderer);
> +        RenderTableCell* cellAboveRenderer = toRenderTableCell(cellRenderer)->table()->cellAbove(toRenderTableCell(cellRenderer));

Same comment as before "It is harder to read with two "toRenderTableCell(cellRenderer)" in the same line...."


> Index: WebCore/rendering/RenderTable.cpp
> ===================================================================

47	RenderTable::RenderTable(Node* node)
...
53	    , m_tableLayout(0)

This initialization may be deleted now.


> Index: WebCore/rendering/RenderTable.h
> ===================================================================
> +    virtual void paintMask(PaintInfo& paintInfo, int tx, int ty);

"paintInfo" param name is redundant.  (It existed before due to the move it looks like a new line so it is a good time to remove this.)

> +inline RenderTable* toRenderTable(RenderObject* o)
> +inline const RenderTable* toRenderTable(const RenderObject* o)

Consider using renderObject instead of o (so that we have full words for the variable names).


> Index: WebCore/rendering/RenderTableCell.h
> ===================================================================
> +inline RenderTableCell* toRenderTableCell(RenderObject* o)
> +inline const RenderTableCell* toRenderTableCell(const RenderObject* o)

Consider using renderObject instead of o.


> Index: WebCore/rendering/RenderTableCol.h
> ===================================================================
> +    void setSpan(int s) { m_span = s; }

Consider using span instead of s.


> +inline RenderTableCol* toRenderTableCol(RenderObject* o)
> +inline const RenderTableCol* toRenderTableCol(const RenderObject* o)

Consider using renderObject instead of o.



> Index: WebCore/rendering/RenderTableRow.h
> ===================================================================

> +inline RenderTableRow* toRenderTableRow(RenderObject* o)
> +inline const RenderTableRow* toRenderTableRow(const RenderObject* o)

Consider using renderObject instead of o.



> Index: WebCore/rendering/RenderTableSection.h
> ===================================================================
> +inline RenderTableSection* toRenderTableSection(RenderObject* o)
> +inline const RenderTableSection* toRenderTableSection(const RenderObject* o)

Consider using renderObject instead of o.

[Darin Adler]

(In reply to comment #12)
> > +inline RenderTableSection* toRenderTableSection(RenderObject* o)
> > +inline const RenderTableSection* toRenderTableSection(const RenderObject* o)
> 
> Consider using renderObject instead of o.

I think I'll just use "object".

[Darin Adler]

Comment on attachment 33821 [details]
Next step: Includes toRenderTable*

Clearing review flag since this was landed in http://trac.webkit.org/changeset/46647

[Darin Adler]

Created attachment 34094 [details]
patch

[David Levin]

Comment on attachment 34094 [details]
patch

Some things to consider changing on landing.  The biggest concern is the changes in writeSVGText and writeSVGImage where the code doesn't appear to do the same thing as before (missing "\n").


> Index: WebCore/rendering/RenderFrameSet.h
> +    virtual void paint(PaintInfo& paintInfo, int tx, int ty);

Please remove "paintInfo".



> Index: WebCore/rendering/RenderInline.h
>  class RenderInline : public RenderBoxModelObject {
>  public:
...
> +    void paintOutline(GraphicsContext*, int tx, int ty);
> +
> +public:

Is this suppose to be protected: instead of public: ?

> +    int verticalPositionFromCache(bool firstLine) const;
> +    void invalidateVerticalPosition() { m_verticalPosition = PositionUndefined; }


> Index: WebCore/rendering/RenderMenuList.h
>  class RenderMenuList : public RenderFlexibleBox, private PopupMenuClient {
>  public:
>      RenderMenuList(Element*);
> -    ~RenderMenuList();
> +    virtual~RenderMenuList();

Missing space after virtual.


> Index: WebCore/rendering/RenderPartObject.h
> +inline RenderPartObject* toRenderPartObject(RenderObject* object)
> +{
> +    ASSERT(!object || (object->isRenderPart() && !object->isFrame()));

Why not use !strmp(object->renderName,"RenderPartObject") as that doesn't rely on the current object hierarchy?

I guess if RenderPartObject isn't a leaf in the hierarchy, that would make sense but I didn't see any derived classes (in my cursory search).



> Index: WebCore/rendering/RenderScrollbar.cpp

> +        case NoPart:
> +        case ScrollbarBGPart:
> +        case AllParts:
> +            break;

If this break was "return SCROLLBAR;", there could be an ASSERT_NOT_REACHED below to check against bad values being passed in at runtime.

>      }
> +    return SCROLLBAR;


> Index: WebCore/rendering/SVGRenderTreeAsText.cpp

Add 2009 to Apple copyright?

> @@ -36,7 +36,7 @@
>  #include "NodeRenderStyle.h"
>  #include "RenderPath.h"
>  #include "RenderSVGContainer.h"
> -#include "RenderSVGImage.h"
> +#include "RenderImage.h"

This include is out of place now (sort it).

> +void writeSVGText(TextStream& ts, const RenderBlock& text, int indent)
>  {
>      writeStandardPrefix(ts, text, indent);
> -    ts << text << "\n";
> +    writeRenderSVGTextBox(ts, text);

It looks like this line should be added here:
    ts << "\n";
to be equivalent to the previous code.

>      writeChildren(ts, text, indent);
>  }

> +void writeSVGImage(TextStream& ts, const RenderImage& image, int indent)
>  {
>      writeStandardPrefix(ts, image, indent);
> -    ts << image << "\n";
> +    writePositionAndStyle(ts, image);

It looks like this line should be added here:
    ts << "\n";
to be equivalent to the previous code.

[Darin Adler]

I'll make sure to run the regression tests before landing. Those "\n" omissions make it pretty clear I didn't before posting the patch!

[Darin Adler]

(In reply to comment #16)
> > +    virtual void paint(PaintInfo& paintInfo, int tx, int ty);
> 
> Please remove "paintInfo".

Done.

> >  class RenderInline : public RenderBoxModelObject {
> >  public:
> ...
> > +    void paintOutline(GraphicsContext*, int tx, int ty);
> > +
> > +public:
> 
> Is this suppose to be protected: instead of public: ?

No, apparently those need to be public. I removed the extra public: I left in by accident.

> >  class RenderMenuList : public RenderFlexibleBox, private PopupMenuClient {
> >  public:
> >      RenderMenuList(Element*);
> > -    ~RenderMenuList();
> > +    virtual~RenderMenuList();
> 
> Missing space after virtual.

> > Index: WebCore/rendering/RenderPartObject.h
> > +inline RenderPartObject* toRenderPartObject(RenderObject* object)
> > +{
> > +    ASSERT(!object || (object->isRenderPart() && !object->isFrame()));
> 
> Why not use !strmp(object->renderName,"RenderPartObject") as that doesn't rely
> on the current object hierarchy?
> 
> I guess if RenderPartObject isn't a leaf in the hierarchy, that would make
> sense but I didn't see any derived classes (in my cursory search).

I thought it wasn't a leaf. But I guess that was wrong. Changed to use strcmp as you suggest.

> > +        case NoPart:
> > +        case ScrollbarBGPart:
> > +        case AllParts:
> > +            break;
> 
> If this break was "return SCROLLBAR;", there could be an ASSERT_NOT_REACHED
> below to check against bad values being passed in at runtime.

I did that. But NoPart and AllParts also need ASSERT_NOT_REACHED, so I left those as break. I'm not even really sure about ScrollbarBGPart, but for now I had it return SCROLLBAR without an assertion.

> > Index: WebCore/rendering/SVGRenderTreeAsText.cpp
> 
> Add 2009 to Apple copyright?

Done.

> > @@ -36,7 +36,7 @@
> >  #include "NodeRenderStyle.h"
> >  #include "RenderPath.h"
> >  #include "RenderSVGContainer.h"
> > -#include "RenderSVGImage.h"
> > +#include "RenderImage.h"
> 
> This include is out of place now (sort it).

Done.

> >      writeStandardPrefix(ts, text, indent);
> > -    ts << text << "\n";
> > +    writeRenderSVGTextBox(ts, text);
> 
> It looks like this line should be added here:
>     ts << "\n";
> to be equivalent to the previous code.

Yes. Done.

> > +void writeSVGImage(TextStream& ts, const RenderImage& image, int indent)
> >  {
> >      writeStandardPrefix(ts, image, indent);
> > -    ts << image << "\n";
> > +    writePositionAndStyle(ts, image);
> 
> It looks like this line should be added here:
>     ts << "\n";
> to be equivalent to the previous code.

Done.

Running regression tests now.

[Darin Adler]

http://trac.webkit.org/changeset/46815