[webkit-reviews] review denied: [Bug 123467] Manage SVGPathByteStream through std::unique_ptr : [Attachment 215426] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 123467: Manage SVGPathByteStream through std::unique_ptr
https://bugs.webkit.org/show_bug.cgi?id=123467

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

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215426&action=review


Patch looks good but I'd like to see a new iteration that doesn't change the
createPath overload to take a raw pointer.

> Source/WebCore/svg/SVGAnimatedPath.cpp:38
> +    std::unique_ptr<SVGPathByteStream> byteStream =
std::make_unique<SVGPathByteStream>();

Use auto here.

> Source/WebCore/svg/SVGAnimatedPath.cpp:50
> +    std::unique_ptr<SVGPathByteStream> byteStream =
std::make_unique<SVGPathByteStream>();

Use auto here.

> Source/WebCore/svg/SVGAnimatedType.cpp:183
> -PassOwnPtr<SVGAnimatedType>
SVGAnimatedType::createPath(PassOwnPtr<SVGPathByteStream> path)
> +PassOwnPtr<SVGAnimatedType> SVGAnimatedType::createPath(SVGPathByteStream*
path)

I don't understand this change. This should take a unique_ptr and call
.release() on it.

> Source/WebCore/svg/SVGPathByteStream.h:54
> +    std::unique_ptr<SVGPathByteStream> copy()

This can be const.

> Source/WebCore/svg/SVGPathUtilities.cpp:151
> +    std::unique_ptr<SVGPathByteStream> appendedByteStream =
std::make_unique<SVGPathByteStream>();

Auto.

> Source/WebCore/svg/SVGPathUtilities.cpp:273
> +    std::unique_ptr<SVGPathByteStream> fromStreamCopy = fromStream->copy();

Auto.