Bug 90998

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.

Created attachment 154147 [details] Bug 90998 fix Proposed fix for bug 90998.

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

Created attachment 154407 [details] Updated patch for bug 90998 Made changes suggested by 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.

Created attachment 154454 [details] Updated patch for bug 90998 Made changes suggested by Kling

Created attachment 154460 [details] Updated patch for bug 90998 Added reviewer, ready for commit

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?

Created attachment 154744 [details] patch for 90998 final changes made

Created attachment 154758 [details] patch for 90998 final changes made

Comment on attachment 154758 [details] patch for 90998 Clearing flags on attachment: 154758 Committed r123830: <http://trac.webkit.org/changeset/123830>

All reviewed patches have been landed. Closing bug.