Bug 23522 - Use checked casts for render tree
Summary: Use checked casts for render tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-24 14:07 PST by Darin Adler
Modified: 2009-08-05 16:23 PDT (History)
4 users (show)

See Also:


Attachments
step 1 -- RenderText (82.60 KB, patch)
2009-01-24 22:50 PST, Darin Adler
no flags Details | Formatted Diff | Diff
work in progress (106.18 KB, patch)
2009-01-26 09:30 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Some other work in progress (25.82 KB, patch)
2009-07-24 18:02 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Next step: Includes toRenderWidget (29.92 KB, patch)
2009-07-29 12:06 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Next step: Includes toRenderTable* (66.35 KB, patch)
2009-07-30 13:20 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch (159.73 KB, patch)
2009-08-04 15:54 PDT, Darin Adler
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2009-01-24 14:07:57 PST
We have a checked cast for RenderBox now, but we want to do that for other render tree types too.
Comment 1 Darin Adler 2009-01-24 14:08:55 PST
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.
Comment 2 Darin Adler 2009-01-24 22:50:35 PST
Created attachment 27007 [details]
step 1 -- RenderText
Comment 3 Darin Adler 2009-01-25 12:24:37 PST
Comment on attachment 27007 [details]
step 1 -- RenderText

Landed as trac.webkit.org/changeset/40229 so clearing review flag.
Comment 4 Darin Adler 2009-01-26 09:30:06 PST
Created attachment 27036 [details]
work in progress
Comment 5 Darin Adler 2009-01-30 13:36:02 PST
Hyatt and Sam are both working on this.
Comment 6 Darin Adler 2009-07-24 18:02:46 PDT
Created attachment 33481 [details]
Some other work in progress
Comment 7 Darin Adler 2009-07-29 12:06:56 PDT
Created attachment 33729 [details]
Next step: Includes toRenderWidget
Comment 8 David Levin 2009-07-29 23:56:01 PDT
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.
Comment 9 Adam Barth 2009-07-30 00:03:47 PDT
(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.
Comment 10 Darin Adler 2009-07-30 13:19:18 PDT
Comment on attachment 33729 [details]
Next step: Includes toRenderWidget

Landed, clearing review flag.
Comment 11 Darin Adler 2009-07-30 13:20:25 PDT
Created attachment 33821 [details]
Next step: Includes toRenderTable*
Comment 12 David Levin 2009-07-30 17:41:41 PDT
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.
Comment 13 Darin Adler 2009-07-30 18:54:14 PDT
(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".
Comment 14 Darin Adler 2009-07-31 17:52:35 PDT
Comment on attachment 33821 [details]
Next step: Includes toRenderTable*

Clearing review flag since this was landed in http://trac.webkit.org/changeset/46647
Comment 15 Darin Adler 2009-08-04 15:54:11 PDT
Created attachment 34094 [details]
patch
Comment 16 David Levin 2009-08-05 02:55:48 PDT
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.
Comment 17 Darin Adler 2009-08-05 09:07:39 PDT
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!
Comment 18 Darin Adler 2009-08-05 10:35:07 PDT
(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.
Comment 19 Darin Adler 2009-08-05 16:23:20 PDT
http://trac.webkit.org/changeset/46815