PlatformCAAnimationWin is leaky and inefficient

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

PlatformCAAnimationWin is leaky and inefficient

[Adam Roben (:aroben)]

Created attachment 79752 [details]
Clean up PlatformCAAnimationWin

[Simon Fraser (smfr)]

Comment on attachment 79752 [details]
Clean up PlatformCAAnimationWin

View in context: https://bugs.webkit.org/attachment.cgi?id=79752&action=review

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:125
> -static CACFTimingFunctionRef toCACFTimingFunction(const TimingFunction* timingFunction)
> +static RetainPtr<CACFTimingFunctionRef> toCACFTimingFunction(const TimingFunction* timingFunction)

Do we use the create moniker for things that return RetainPtrs?

[Adam Roben (:aroben)]

Comment on attachment 79752 [details]
Clean up PlatformCAAnimationWin

View in context: https://bugs.webkit.org/attachment.cgi?id=79752&action=review

>> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:125
>> +static RetainPtr<CACFTimingFunctionRef> toCACFTimingFunction(const TimingFunction* timingFunction)
> 
> Do we use the create moniker for things that return RetainPtrs?

I don't think we do. Since there's no way to incorrectly handle the lifetime when a RetainPtr is returned, there's no need to use the "Create" pattern.

[Darin Adler]

(In reply to comment #3)
> >> +static RetainPtr<CACFTimingFunctionRef> toCACFTimingFunction(const TimingFunction* timingFunction)
> > 
> > Do we use the create moniker for things that return RetainPtrs?
> 
> I don't think we do.

I think sometimes we do. I’d have to look to find examples, but I believe they exist.

> Since there's no way to incorrectly handle the lifetime when a RetainPtr is returned, there's no need to use the "Create" pattern.

I agree that you don’t need Create/Copy just to avoid basic reference counting mistakes with smart pointers, but I do still think a name that alludes to the fact that it’s a new object and so can be costly for memory might be helpful. So I’d use Create but not Copy in names. And it’s not going to be as precise as the “this gives you a reference you need to balance with release or adopt” rule.

[Adam Roben (:aroben)]

Committed r76358: <http://trac.webkit.org/changeset/76358>

[Adam Roben (:aroben)]

(In reply to comment #4)
> (In reply to comment #3)
> > Since there's no way to incorrectly handle the lifetime when a RetainPtr is returned, there's no need to use the "Create" pattern.
> 
> I agree that you don’t need Create/Copy just to avoid basic reference counting mistakes with smart pointers, but I do still think a name that alludes to the fact that it’s a new object and so can be costly for memory might be helpful. So I’d use Create but not Copy in names. And it’s not going to be as precise as the “this gives you a reference you need to balance with release or adopt” rule.

This function sometimes creates a new object and sometimes returns an existing object, but we could probably still come up with an accurate name.