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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.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 140725: Patch
https://bugs.webkit.org/attachment.cgi?id=140725&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140725&action=review


Nice approach in general! Some nitpicks:

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:58
> +typedef HashMap<const RenderSVGShape*, SVGMarkerLayoutInfo*>
MarkerLayoutInfoMap;
> +static MarkerLayoutInfoMap* gMarkerLayoutInfoMap = 0;

Why don't you keep this in the getMarkerLayoutInfo method, and make it static??

Also it should keep OwnPtr<SVGMarkerLayoutInfo>, this is possible in trunk
since a while.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:60
> +SVGMarkerLayoutInfo* getMarkerLayoutInfo(const RenderSVGShape* shape)

Proposal (see below as well):
SVGMarkerLayoutInfo* markerLayoutInfoForShape(RenderSVGShape*)
SVGMarkerLayoutInfo* ensureMarkerLayoutInfoForShape(RenderSVGShape*).

I don't think we need const RenderSVGShape here, correct me if I'm wrong.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:428
> +	   gMarkerLayoutInfoMap = new MarkerLayoutInfoMap();

s/()//

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:434
> +    SVGMarkerLayoutInfo* markerLayoutInfo = getMarkerLayoutInfo(this);
> +    if (!markerLayoutInfo) {
> +	   markerLayoutInfo = new SVGMarkerLayoutInfo();
> +	   gMarkerLayoutInfoMap->add(this, markerLayoutInfo);
> +    }

This should be done in a function named "ensureMarkerLayoutInfo" which gives
you a SVGMarkerLayoutInfo* and stashes it in the cache.