Bug 123467 - Manage SVGPathByteStream through std::unique_ptr
Summary: Manage SVGPathByteStream through std::unique_ptr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-29 14:02 PDT by Zan Dobersek
Modified: 2013-10-31 08:27 PDT (History)
6 users (show)

See Also:

Attachments
Patch (10.58 KB, patch)
2013-10-29 14:17 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (10.97 KB, patch)
2013-10-30 11:10 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (11.04 KB, patch)
2013-10-31 02:44 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2013-10-29 14:02:18 PDT 
Manage SVGPathByteStream through std::unique_ptr
Comment 1 Zan Dobersek 2013-10-29 14:17:37 PDT 
Created attachment 215426 [details]
Patch
Comment 2 Anders Carlsson 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.
Comment 3 Zan Dobersek 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.
Comment 4 Zan Dobersek 2013-10-30 11:10:48 PDT 
Created attachment 215532 [details]
Patch
Comment 5 Anders Carlsson 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?
Comment 6 Zan Dobersek 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.
Comment 7 Zan Dobersek 2013-10-31 02:44:17 PDT 
Created attachment 215638 [details]
Patch
Comment 8 Zan Dobersek 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>
Comment 9 Zan Dobersek 2013-10-31 08:27:45 PDT 
All reviewed patches have been landed.  Closing bug.