Bug 94068 - [chromium] Convert WebAnimationCurve subtypes into pure virtual
Summary: [chromium] Convert WebAnimationCurve subtypes into pure virtual
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-14 22:11 PDT by James Robinson
Modified: 2012-08-23 17:35 PDT (History)
8 users (show)

See Also:


Attachments
Patch (62.96 KB, patch)
2012-08-14 22:12 PDT, James Robinson
no flags Details | Formatted Diff | Diff
rebased (73.44 KB, patch)
2012-08-15 21:06 PDT, James Robinson
no flags Details | Formatted Diff | Diff
make non-ownership-transfer functions take const refs (73.65 KB, patch)
2012-08-23 17:05 PDT, James Robinson
enne: review+
enne: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-08-14 22:11:08 PDT
[chromium] Convert WebAnimationCurve subtypes into pure virtual
Comment 1 James Robinson 2012-08-14 22:12:00 PDT
Created attachment 158501 [details]
Patch
Comment 2 James Robinson 2012-08-15 21:06:14 PDT
Created attachment 158703 [details]
rebased
Comment 3 WebKit Review Bot 2012-08-15 21:08:57 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Adrienne Walker 2012-08-21 09:17:54 PDT
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// ;)
Comment 5 James Robinson 2012-08-21 15:37:26 PDT
(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.
Comment 6 James Robinson 2012-08-21 15:40:43 PDT
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?
Comment 7 James Robinson 2012-08-22 13:54:40 PDT
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.
Comment 8 James Robinson 2012-08-23 17:05:45 PDT
Created attachment 160286 [details]
make non-ownership-transfer functions take const refs
Comment 9 Adrienne Walker 2012-08-23 17:24:54 PDT
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// >;)
Comment 10 James Robinson 2012-08-23 17:35:38 PDT
Committed r126513: <http://trac.webkit.org/changeset/126513>