[webkit-reviews] review granted: [Bug 124382] [SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr : [Attachment 216984] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Correia (qrwteyrutiyoup)
<sergio.correia at openbossa.org>'s request for review:
Bug 124382: [SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr
https://bugs.webkit.org/show_bug.cgi?id=124382

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

------- Additional Comments from Darin Adler <darin at apple.com>
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 !!