[chromium] Convert WebAnimationCurve subtypes into pure virtual
Created attachment 158501 [details] Patch
Created attachment 158703 [details] rebased
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 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// ;)
(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.
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 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.
Created attachment 160286 [details] make non-ownership-transfer functions take const refs
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// >;)
Committed r126513: <http://trac.webkit.org/changeset/126513>