Bug 142063 – Remove PassRefPtr from svg/properties classes

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

In the revision http://trac.webkit.org/changeset/180129, I changed the return type of some functions from PassRefPtr<> to Ref<>. But I did not check the callers of these functions to correct the type of variables which are assigned the return of the modified functions.

[Said Abou-Hallawa]

Created attachment 247466 [details]
Patch

[Chris Dumez]

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<>?

[Said Abou-Hallawa]

Created attachment 248131 [details]
Patch

[Said Abou-Hallawa]

(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.

[Darin Adler]

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?

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

Created attachment 248333 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 248333 [details]
Patch

Clearing flags on attachment: 248333

Committed r181345: <http://trac.webkit.org/changeset/181345>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.