RESOLVED FIXED123467
Manage SVGPathByteStream through std::unique_ptr 
https://bugs.webkit.org/show_bug.cgi?id=123467
Summary Manage SVGPathByteStream through std::unique_ptr
Zan Dobersek
Reported 2013-10-29 14:02:18 PDT
Manage SVGPathByteStream through std::unique_ptr
Attachments
Patch (10.58 KB, patch)
2013-10-29 14:17 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (10.97 KB, patch)
2013-10-30 11:10 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (11.04 KB, patch)
2013-10-31 02:44 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Zan Dobersek
Comment 1 2013-10-29 14:17:37 PDT
Created attachment 215426 [details] Patch
Anders Carlsson
Comment 2 2013-10-30 07:25:51 PDT
Comment on attachment 215426 [details] Patch 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.
Zan Dobersek
Comment 3 2013-10-30 10:57:43 PDT
Comment on attachment 215426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215426&action=review >> Source/WebCore/svg/SVGPathByteStream.h:54 >> + std::unique_ptr<SVGPathByteStream> copy() > > This can be const. That enforces constness on the m_data member and calls the non-existent SVGPathByteStream(const Data&) constructor, causing compilation failures.
Zan Dobersek
Comment 4 2013-10-30 11:10:48 PDT
Created attachment 215532 [details] Patch
Anders Carlsson
Comment 5 2013-10-30 15:28:27 PDT
Comment on attachment 215426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215426&action=review >>> Source/WebCore/svg/SVGPathByteStream.h:54 >>> + std::unique_ptr<SVGPathByteStream> copy() >> >> This can be const. > > That enforces constness on the m_data member and calls the non-existent SVGPathByteStream(const Data&) constructor, causing compilation failures. Why does the SVGPathByteStream need to take a non-const reference?
Zan Dobersek
Comment 6 2013-10-31 02:27:04 PDT
Comment on attachment 215426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215426&action=review >>>> Source/WebCore/svg/SVGPathByteStream.h:54 >>>> + std::unique_ptr<SVGPathByteStream> copy() >>> >>> This can be const. >> >> That enforces constness on the m_data member and calls the non-existent SVGPathByteStream(const Data&) constructor, causing compilation failures. > > Why does the SVGPathByteStream need to take a non-const reference? No need actually, the constructor can work with a const reference. copy() can be const as well then.
Zan Dobersek
Comment 7 2013-10-31 02:44:17 PDT
Created attachment 215638 [details] Patch
Zan Dobersek
Comment 8 2013-10-31 08:27:38 PDT
Comment on attachment 215638 [details] Patch Clearing flags on attachment: 215638 Committed r158359: <http://trac.webkit.org/changeset/158359>
Zan Dobersek
Comment 9 2013-10-31 08:27:45 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.