Summary: | De-virtualize WrapShape classes | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bear Travis <betravis> | ||||||||||||||
Component: | CSS | Assignee: | Anish <bhayani> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | betravis, eric, gyuyoung.kim, rakuco, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 89670 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Bear Travis
2012-07-11 11:40:07 PDT
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. |