[webkit-reviews] review granted: [Bug 23522] Use checked casts for render tree : [Attachment 34094] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 23522: Use checked casts for render tree
https://bugs.webkit.org/show_bug.cgi?id=23522

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

------- Additional Comments from David Levin <levin at chromium.org>
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.