[webkit-reviews] review denied: [Bug 60625] Cleanup RenderSVGPath to offer a non-marker variant : [Attachment 140764] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 60625: Cleanup RenderSVGPath to offer a non-marker variant
https://bugs.webkit.org/show_bug.cgi?id=60625

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140764&action=review


I don't like this approach.  Unless you have data to suggest this is worth the
extra code, I would prefer an OwnPtr/RarePtr approach or leaving the code
as-is.	If your goal is to make this object smaller, we should have evidence
that this actually affects memory usage, and should COMPILE_ASSERT so we never
get it bigger.	OTherwise we're just making work for ourselves here. :)

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:59
> +static SVGMarkerLayoutInfo* getMarkerLayoutInfo(const RenderSVGShape* shape,
bool ensure = false)

We dont' normally use get in method names.

"Precede setters with the word "set". Use bare words for getters. Setter and
getter names should match the names of the variables being set/gotten."
http://www.webkit.org/coding/coding-style.html

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:89
> +	   delete markerLayoutInfo;

You could also have your HashMap OwnPtr the marker and then just always empty
the hash map for every shape destruction.