Summary: | Remove PassRefPtr from svg/properties classes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Component: | SVG | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, zimmermann | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 144092 | ||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2015-02-26 16:02:23 PST
Created attachment 247466 [details]
Patch
Comment on attachment 247466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247466&action=review > Source/WebCore/svg/SVGMarkerElement.cpp:242 > +RefPtr<SVGAnimatedProperty> SVGMarkerElement::lookupOrCreateOrientTypeWrapper(SVGElement* contextElement) Shouldn't this return a Ref<>? > Source/WebCore/svg/SVGPathElement.cpp:314 > +RefPtr<SVGAnimatedProperty> SVGPathElement::lookupOrCreateDWrapper(SVGElement* contextElement) Shouldn't this return a Ref<>? > Source/WebCore/svg/SVGPolyElement.cpp:140 > +RefPtr<SVGAnimatedProperty> SVGPolyElement::lookupOrCreatePointsWrapper(SVGElement* contextElement) Shouldn't this return a Ref<>? > Source/WebCore/svg/SVGTextContentElement.cpp:84 > +RefPtr<SVGAnimatedProperty> SVGTextContentElement::lookupOrCreateTextLengthWrapper(SVGElement* contextElement) Shouldn't this return a Ref<>? > Source/WebCore/svg/SVGViewSpec.cpp:170 > +RefPtr<SVGAnimatedProperty> SVGViewSpec::lookupOrCreateViewBoxWrapper(SVGViewSpec* ownerType) Shouldn't this return a Ref<>? Created attachment 248131 [details]
Patch
(In reply to comment #2) > Comment on attachment 247466 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247466&action=review > > > Source/WebCore/svg/SVGMarkerElement.cpp:242 > > +RefPtr<SVGAnimatedProperty> SVGMarkerElement::lookupOrCreateOrientTypeWrapper(SVGElement* contextElement) > > Shouldn't this return a Ref<>? > > > Source/WebCore/svg/SVGPathElement.cpp:314 > > +RefPtr<SVGAnimatedProperty> SVGPathElement::lookupOrCreateDWrapper(SVGElement* contextElement) > > Shouldn't this return a Ref<>? > > > Source/WebCore/svg/SVGPolyElement.cpp:140 > > +RefPtr<SVGAnimatedProperty> SVGPolyElement::lookupOrCreatePointsWrapper(SVGElement* contextElement) > > Shouldn't this return a Ref<>? > > > Source/WebCore/svg/SVGTextContentElement.cpp:84 > > +RefPtr<SVGAnimatedProperty> SVGTextContentElement::lookupOrCreateTextLengthWrapper(SVGElement* contextElement) > > Shouldn't this return a Ref<>? > > > Source/WebCore/svg/SVGViewSpec.cpp:170 > > +RefPtr<SVGAnimatedProperty> SVGViewSpec::lookupOrCreateViewBoxWrapper(SVGViewSpec* ownerType) > > Shouldn't this return a Ref<>? Done. Comment on attachment 248131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248131&action=review > Source/WTF/wtf/Ref.h:159 > +template<typename T, typename U> inline Ref<T> static_reference_cast(Ref<U>& reference) > +{ > + return Ref<T>(static_cast<T&>(reference.get())); > +} > + > +template<typename T, typename U> inline Ref<T> static_reference_cast(const Ref<U>& reference) > +{ > + return Ref<T>(static_cast<T&>(reference.copyRef().get())); > +} It would be nice to avoid adding this if we can. Read below to see why we may not need it. > Source/WebCore/svg/properties/SVGAnimatedProperty.h:51 > + static Ref<TearOffType> lookupOrCreateWrapper(OwnerType* element, const SVGPropertyInfo* info, PropertyType& property) Because of the caching approach, the return type here could be TearOffType&. Whether it’s found in the cache or created, the cache has a reference to the return value, so there’s no need for the return type to do reference counting at all; the caller can ref the object if they intend to keep it by storing it in a Ref, or not. This could simplify all the callers, which in turn don’t need to use static_reference_cast. I think there will be a cascading effect of this that should turn many return values into simple references rather than Ref. > Source/WebCore/svg/properties/SVGListProperty.h:145 > return 0; If you are refactoring like this, these should change into nullptr. > Source/WebCore/svg/properties/SVGListProperty.h:166 > + return newItem; Without an explicit call to WTF::move, does this result in reference count churn, or is the language/compiler smart enough to do a move with the WTF::move? (In reply to comment #5) > Comment on attachment 248131 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248131&action=review > > > Source/WTF/wtf/Ref.h:159 > > +template<typename T, typename U> inline Ref<T> static_reference_cast(Ref<U>& reference) > > +{ > > + return Ref<T>(static_cast<T&>(reference.get())); > > +} > > + > > +template<typename T, typename U> inline Ref<T> static_reference_cast(const Ref<U>& reference) > > +{ > > + return Ref<T>(static_cast<T&>(reference.copyRef().get())); > > +} > > It would be nice to avoid adding this if we can. Read below to see why we > may not need it. > > > Source/WebCore/svg/properties/SVGAnimatedProperty.h:51 > > + static Ref<TearOffType> lookupOrCreateWrapper(OwnerType* element, const SVGPropertyInfo* info, PropertyType& property) > > Because of the caching approach, the return type here could be TearOffType&. > Whether it’s found in the cache or created, the cache has a reference to the > return value, so there’s no need for the return type to do reference > counting at all; the caller can ref the object if they intend to keep it by > storing it in a Ref, or not. This could simplify all the callers, which in > turn don’t need to use static_reference_cast. I think there will be a > cascading effect of this that should turn many return values into simple > references rather than Ref. > Ic could convert SVGAnimatedProperty::lookupOrCreateWrapper() to return a TearOffType& but I could not go any deeper than that. The reason is SVGAttributeToPropertyMap packs the properties with their values of an SVG element in a Vector<RefPtr<SVGAnimatedProperty>>. This vector is put in a SVGElementAnimatedProperties which is assigned to m_animatedProperties of the class SVGAnimateElementBase. I am not sure about the lifetime of SVGAnimatedProperty and its cache versus SVGAnimateElementBase: which one gets released first? So I left all the calls after SVGAnimatedProperty::lookupOrCreateWrapper() to be Ref<>. > > Source/WebCore/svg/properties/SVGListProperty.h:145 > > return 0; > > If you are refactoring like this, these should change into nullptr. > Done. > > Source/WebCore/svg/properties/SVGListProperty.h:166 > > + return newItem; > > Without an explicit call to WTF::move, does this result in reference count > churn, or is the language/compiler smart enough to do a move with the > WTF::move? I do not think we need to explicitly WTF::move the passed argument to the return value. Since they are both of type RefPtr<> the move constructor is called for the return statement and hence the ref-count should not be affected. Created attachment 248333 [details]
Patch
Comment on attachment 248333 [details] Patch Clearing flags on attachment: 248333 Committed r181345: <http://trac.webkit.org/changeset/181345> All reviewed patches have been landed. Closing bug. |