WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
124382
[SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr
https://bugs.webkit.org/show_bug.cgi?id=124382
Summary
[SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr
Sergio Correia (qrwteyrutiyoup)
Reported
2013-11-14 14:48:28 PST
[SVG] Convert OwnPtr/PassOwnPtr to std::unique_ptr
Attachments
Patch
(114.46 KB, patch)
2013-11-14 14:50 PST
,
Sergio Correia (qrwteyrutiyoup)
no flags
Details
Formatted Diff
Diff
v2
(117.23 KB, patch)
2013-11-16 10:19 PST
,
Sergio Correia (qrwteyrutiyoup)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Sergio Correia (qrwteyrutiyoup)
Comment 1
2013-11-14 14:50:08 PST
Created
attachment 216984
[details]
Patch
Darin Adler
Comment 2
2013-11-14 17:58:56 PST
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)
Comment 3
2013-11-16 10:19:25 PST
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)
Comment 4
2013-11-16 10:21:35 PST
(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 5
2013-11-19 09:20:39 PST
Comment on
attachment 217127
[details]
v2 Clearing flags on attachment: 217127 Committed
r159503
: <
http://trac.webkit.org/changeset/159503
>
WebKit Commit Bot
Comment 6
2013-11-19 09:20:43 PST
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.
Top of Page
Format For Printing
XML
Clone This Bug