[webkit-reviews] review denied: [Bug 90998] De-virtualize WrapShape classes : [Attachment 154147] Bug 90998 fix

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Anish <bhayani at adobe.com>'s request
for review:
Bug 90998: De-virtualize WrapShape classes
https://bugs.webkit.org/show_bug.cgi?id=90998

Attachment 154147: Bug 90998 fix
https://bugs.webkit.org/attachment.cgi?id=154147&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154147&action=review


Good start, just needs a bit of clean-up.

The ChangeLog is a bit wordy, focus on what you are doing and why.

> Source/WebCore/rendering/style/WrapShapes.cpp:16
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER “AS IS” AND ANY

Weird quotation marks here.

> Source/WebCore/rendering/style/WrapShapes.cpp:34
> +using namespace std;

You are not using anything from the std namespace, so this should not be here.

> Source/WebCore/rendering/style/WrapShapes.cpp:38
> +

Extra newline here.

> Source/WebCore/rendering/style/WrapShapes.cpp:57
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   delete static_cast<WrapShape*>(this);
> +	   break;

It would be better to omit the default case here. That means that any unhandled
addition to the Type enum will cause a compilation failure.
If you replace the breaks by returns, you could put an ASSERT_NOT_REACHED()
after the switch statement for good measure.
Look at CSSValue::destroy() for example.

> Source/WebCore/rendering/style/WrapShapes.h:61
> +    Type m_type;

This variable should be private.

> Source/WebCore/rendering/style/WrapShapes.h:66
> +    void setType(Type s_type)
> +    {
> +	   m_type = s_type;
> +    }

It would be slightly nicer to pass the Type to the WrapShape constructor
instead.

> Source/WebCore/rendering/style/WrapShapes.h:96
>      WrapShapeRectangle()
>	   : m_cornerRadiusX(Undefined)
>	   , m_cornerRadiusY(Undefined)
> -    { }
> +    { 
> +	   setType(WRAP_SHAPE_RECTANGLE);
> +    }

This block would then look like so:
WrapShapeRectangle()
    : WrapShape(WRAP_SHAPE_RECTANGLE)
    , m_cornerRadiusX(Undefined)
    , m_cornerRadiusY(Undefined)
{ }