Bug 94068 – [chromium] Convert WebAnimationCurve subtypes into pure virtual

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Convert WebAnimationCurve subtypes into pure virtual

[James Robinson]

Created attachment 158501 [details]
Patch

[James Robinson]

Created attachment 158703 [details]
rebased

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adrienne Walker]

Comment on attachment 158703 [details]
rebased

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

> Source/Platform/chromium/public/WebAnimation.h:53
> -    // The caller takes ownership of the returned valuev
> -    WEBKIT_EXPORT static WebAnimation* create(const WebAnimationCurve&, TargetProperty);
> +    // The caller takes ownership of the returned value.
> +    WEBKIT_EXPORT static WebAnimation* create(WebAnimationCurve*, TargetProperty);

If you're going to change the parameter type here, I think you also need some indication about the ownership/lifetime guarantees on the WebAnimationCurve, either via a comment or by reverting the signature change.  Previously it was a const ref, which I thought was much more clear.

> Source/WebKit/chromium/src/WebAnimationImpl.h:40
> +    explicit WebAnimationImpl(WebAnimationCurve*, int animationId, int groupId, TargetProperty);

s/explicit// ;)

[James Robinson]

(In reply to comment #4)
> (From update of attachment 158703 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158703&action=review
> 
> > Source/Platform/chromium/public/WebAnimation.h:53
> > -    // The caller takes ownership of the returned valuev
> > -    WEBKIT_EXPORT static WebAnimation* create(const WebAnimationCurve&, TargetProperty);
> > +    // The caller takes ownership of the returned value.
> > +    WEBKIT_EXPORT static WebAnimation* create(WebAnimationCurve*, TargetProperty);
> 
> If you're going to change the parameter type here, I think you also need some indication about the ownership/lifetime guarantees on the WebAnimationCurve, either via a comment or by reverting the signature change.  Previously it was a const ref, which I thought was much more clear.
> 

I've already seen one leak very similar to this.  We could adopt a convention that parameters that don't take ownership have a const ref and parameters that do take ownership take a pointer.  I think that would work well for everything except for WebLayerTreeView::setRootLayer(), which doesn't take ownership of the parameter but does let you pass in NULL as a value.  It's possible that we don't need that any more, though.

[James Robinson]

This might also look weird for the WebLayer hierarchy APIs.  None of them take ownership, but currently they look like:

setMaskLayer(WebLayer*);
setChildren(WebVector<WebLayer*>);

We do need to null out mask layers. We could have a clear..() call in addition.

WebVector<const WebLayer&> would be possible, but it just looks a little odd - I'm not completely sure why but something about it is strange.

What would you prefer - const refs everywhere, or document ownership semantics function-by-function?

[James Robinson]

Comment on attachment 158703 [details]
rebased

I'll update this patch (and our other APIs) to have parameters that don't transfer ownership be by const ref where possible.

[James Robinson]

Created attachment 160286 [details]
make non-ownership-transfer functions take const refs

[Adrienne Walker]

Comment on attachment 160286 [details]
make non-ownership-transfer functions take const refs

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

R=me.

> Source/WebKit/chromium/src/WebAnimationImpl.h:41
> +    explicit WebAnimationImpl(const WebAnimationCurve&, int animationId, int groupId, TargetProperty);
> +    virtual ~WebAnimationImpl();

s/explicit// >;)

[James Robinson]

Committed r126513: <http://trac.webkit.org/changeset/126513>