Bug 90998 – De-virtualize WrapShape classes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bear Travis]

The WrapShape classes do not need to use a virtual pointer for type/destructor. See bug 89670.

[Bear Travis]

Original comment from anttik:

> Source/WebCore/rendering/style/WrapShapes.h:52
> +class WrapShape : public RefCounted<WrapShape> {
> +public:
> +    enum Type {
> +        WRAP_SHAPE_RECTANGLE = 1,
> +        WRAP_SHAPE_CIRCLE = 2,
> +        WRAP_SHAPE_ELLIPSE = 3,
> +        WRAP_SHAPE_POLYGON = 4
> +    };
> +
> +    virtual Type type() const = 0;
> +    virtual ~WrapShape() { }
> +
> +protected:
> +    WrapShape() { }
> +};

This doesn't really need the vptr. You could make the type a field in the base and cast to subclass in destructor. This would save some memory (on 64bit) and avoid the virtual calls.

[Anish]

Created attachment 154147 [details]
Bug 90998 fix

Proposed fix for bug 90998.

[Andreas Kling]

Comment on attachment 154147 [details]
Bug 90998 fix

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)
{ }

[Anish]

Created attachment 154407 [details]
Updated patch for bug 90998

Made changes suggested by Kling

[Andreas Kling]

Comment on attachment 154407 [details]
Updated patch for bug 90998

View in context: https://bugs.webkit.org/attachment.cgi?id=154407&action=review

r=me, nice.

> Source/WebCore/rendering/style/WrapShapes.h:62
> +    WrapShape(Type type) 
> +    {
> +        m_type = type;
> +    }

Nit: we tend to use initializer lists in WebKit, i.e:
WrapShape(Type type)
    : m_type(type)
{ }

> Source/WebCore/rendering/style/WrapShapes.h:166
> -
> +    

Extra whitespace here.

[Anish]

Created attachment 154454 [details]
Updated patch for bug 90998

Made changes suggested by Kling

[Anish]

Created attachment 154460 [details]
Updated patch for bug 90998

Added reviewer, ready for commit

[Alexandru Chiculita]

Comment on attachment 154460 [details]
Updated patch for bug 90998

View in context: https://bugs.webkit.org/attachment.cgi?id=154460&action=review

> Source/WebCore/rendering/style/WrapShapes.h:56
>      virtual ~WrapShape() { }

Didn't you have to remove virtual functions?

[Anish]

Created attachment 154744 [details]
patch for 90998

final changes made

[Anish]

Created attachment 154758 [details]
patch for 90998

final changes made

[WebKit Review Bot]

Comment on attachment 154758 [details]
patch for 90998

Clearing flags on attachment: 154758

Committed r123830: <http://trac.webkit.org/changeset/123830>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.