Bug 124382 – [SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Correia (qrwteyrutiyoup)]

[SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 216984 [details]
Patch

[Darin Adler]

Comment on attachment 216984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216984&action=review

OK to land this as is, but I see many opportunities for improvement.

> Source/WebCore/platform/graphics/SimpleFontData.h:170
> +    bool isSVGFont() const { return !!m_fontData; }

I think I’d rather see m_fontData != nullptr here than !!m_fontData. Not sure if we have consensus in the team, though.

> Source/WebCore/platform/graphics/SimpleFontData.h:226
> +    SimpleFontData(std::unique_ptr<AdditionalFontData> , float fontSize, bool syntheticBold, bool syntheticItalic);

Extra space here before the comma.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:37
> +    auto animatedType = SVGAnimatedType::createAngleAndEnumeration(new pair<SVGAngle, unsigned>);

Bare new worries me. Can we make that use std::unique_ptr too?

> Source/WebCore/svg/SVGAnimatedBoolean.cpp:36
> +    auto animtedType = SVGAnimatedType::createBoolean(new bool);

Typo in this function, animatedType is missing an "a". Bare new worries me. Can we make that use std::unique_ptr too?

> Source/WebCore/svg/SVGAnimatedColor.cpp:39
> +    auto animtedType = SVGAnimatedType::createColor(new Color);

Same typo here. Bare new worries me. Can we make that use std::unique_ptr too?

> Source/WebCore/svg/SVGAnimatedEnumeration.cpp:113
> +    auto animatedType = SVGAnimatedType::createEnumeration(new unsigned);

Bare new worries me. Can we make that use std::unique_ptr too?

> Source/WebCore/svg/SVGAnimatedInteger.cpp:38
> +    auto animtedType = SVGAnimatedType::createInteger(new int);

And here. Bare new worries me. Can we make that use std::unique_ptr too?

> Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:38
> +    auto animtedType = SVGAnimatedType::createIntegerOptionalInteger(new pair<int, int>);

And here.

> Source/WebCore/svg/SVGAnimatedLength.cpp:45
>      return SVGAnimatedType::createLength(new SVGLength(m_lengthMode, string));

Bare new worries me. Can we make that use std::unique_ptr too?

> Source/WebCore/svg/SVGAnimatedLengthList.cpp:38
> +    auto animateType = SVGAnimatedType::createLengthList(new SVGLengthList);

Funny that here we have a different typo. Should be "animatedType" or "type", not "animateType". Bare new issue again.

> Source/WebCore/svg/SVGAnimatedNumber.cpp:37
> +    auto animtedType = SVGAnimatedType::createNumber(new float);

Typo and bare new again here.

> Source/WebCore/svg/SVGAnimatedNumberList.cpp:37
> +    auto animtedType = SVGAnimatedType::createNumberList(new SVGNumberList);

And here.

> Source/WebCore/svg/SVGAnimatedNumberList.h:44
>      virtual ~SVGAnimatedNumberListAnimator() { }

This line of code can and should be deleted.

> Source/WebCore/svg/SVGAnimatedNumberList.h:47
> +    virtual std::unique_ptr<SVGAnimatedType> constructFromString(const String&);
> +    virtual std::unique_ptr<SVGAnimatedType> startAnimValAnimation(const SVGElementAnimatedPropertyList&);

Should add OVERRIDE here.

> Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:38
> +    auto animtedType = SVGAnimatedType::createNumberOptionalNumber(new pair<float, float>);

Typo again and the bare new.

> Source/WebCore/svg/SVGAnimatedPointList.cpp:38
> +    auto animtedType = SVGAnimatedType::createPointList(new SVGPointList);

Both issues, again.

> Source/WebCore/svg/SVGAnimatedPreserveAspectRatio.cpp:36
> +    auto animatedType = SVGAnimatedType::createPreserveAspectRatio(new SVGPreserveAspectRatio);

Bare new again.

> Source/WebCore/svg/SVGAnimatedRect.cpp:37
> +    auto animatedType = SVGAnimatedType::createRect(new FloatRect);

Again.

> Source/WebCore/svg/SVGAnimatedString.cpp:36
> +    auto animatedType = SVGAnimatedType::createString(new String);

Again.

> Source/WebCore/svg/SVGAnimatedTransformList.cpp:46
> +    auto animatedType = SVGAnimatedType::createTransformList(new SVGTransformList);

Again.

> Source/WebCore/svg/SVGAnimatedType.h:61
> +    static std::unique_ptr<SVGAnimatedType> createAngleAndEnumeration(std::pair<SVGAngle, unsigned>*);
> +    static std::unique_ptr<SVGAnimatedType> createBoolean(bool*);
> +    static std::unique_ptr<SVGAnimatedType> createColor(Color*);
> +    static std::unique_ptr<SVGAnimatedType> createEnumeration(unsigned*);
> +    static std::unique_ptr<SVGAnimatedType> createInteger(int*);
> +    static std::unique_ptr<SVGAnimatedType> createIntegerOptionalInteger(std::pair<int, int>*);
> +    static std::unique_ptr<SVGAnimatedType> createLength(SVGLength*);
> +    static std::unique_ptr<SVGAnimatedType> createLengthList(SVGLengthList*);
> +    static std::unique_ptr<SVGAnimatedType> createNumber(float*);
> +    static std::unique_ptr<SVGAnimatedType> createNumberList(SVGNumberList*);
> +    static std::unique_ptr<SVGAnimatedType> createNumberOptionalNumber(std::pair<float, float>*);
> +    static std::unique_ptr<SVGAnimatedType> createPath(std::unique_ptr<SVGPathByteStream>);
> +    static std::unique_ptr<SVGAnimatedType> createPointList(SVGPointList*);
> +    static std::unique_ptr<SVGAnimatedType> createPreserveAspectRatio(SVGPreserveAspectRatio*);
> +    static std::unique_ptr<SVGAnimatedType> createRect(FloatRect*);
> +    static std::unique_ptr<SVGAnimatedType> createString(String*);
> +    static std::unique_ptr<SVGAnimatedType> createTransformList(SVGTransformList*);

These functions should all take std::unique_ptr arguments instead of bare pointers.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:101
> +    auto end = timeContainers.end();
> +    for (auto itr = timeContainers.begin(); itr != end; ++itr)

Lame that this uses the name "itr"; we normally use "it".

> Source/WebCore/svg/SVGFontData.h:33
> +    SVGFontData(SVGFontFaceElement*);

Should be marked explicit.

> Source/WebCore/svg/SVGPathByteStreamSource.h:32
>      SVGPathByteStreamSource(SVGPathByteStream*);

Should be marked explicit.

> Source/WebCore/svg/SVGPathSegListSource.h:34
>      SVGPathSegListSource(const SVGPathSegList&);

Should be marked explicit.

> Source/WebCore/svg/SVGPathStringSource.h:32
>      SVGPathStringSource(const String&);

Should be marked explicit.

> Source/WebCore/svg/graphics/SVGImage.cpp:382
> +    return !!m_page;

Same comment about != nullptr being better than !!

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 217127 [details]
v2

Typos fixed (also removed the extra space before the comma) and now using != nullptr instead of ls

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #3)
> Created an attachment (id=217127) [details]
> v2
> 
> Typos fixed (also removed the extra space before the comma) and now using != nullptr instead of ls


^ instead of "!!"

Darin, I am posting two other patches on top of this one to implement your other suggestions (mark functions as OVERRIDE and casses as FINAL, and replacing the bare pointers with std:unique_ptr in those create functions)

[WebKit Commit Bot]

Comment on attachment 217127 [details]
v2

Clearing flags on attachment: 217127

Committed r159503: <http://trac.webkit.org/changeset/159503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.