[SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr
Created attachment 216984 [details] Patch
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 !!
Created attachment 217127 [details] v2 Typos fixed (also removed the extra space before the comma) and now using != nullptr instead of ls
(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)
Comment on attachment 217127 [details] v2 Clearing flags on attachment: 217127 Committed r159503: <http://trac.webkit.org/changeset/159503>
All reviewed patches have been landed. Closing bug.