Don't clone WebAnimation in WebLayerImpl::addAnimation().

Don't clone WebAnimation in WebLayerImpl::addAnimation().

Blink does not need a WebAnimation object after passing it to Chromium, so we
can pass the ownership of cc:Animation in WebAnimation to Chromium compositor
instead of cloning.

ANIMATION_OWNERSHIP_TRANSFER is a temporary macro to change addAnimation() in
both Blink and Chromium. If ANIMATION_OWNERSHIP_TRANSFER is defined,
Blink passes ownership of WebAnimation to Chromium, so Chromium must delete the
given pointer.

BUG=290217
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229128

[dshwang, 2013-09-19 19:15:39]

On 2013/09/19 18:58:35, dshwang wrote:

https://codereview.chromium.org/23834010/ is submitted to note the changed
semantic of WebLayer::addAnimation() that this CL changes.

[Ian Vollick, 2013-09-20 15:17:08]

On 2013/09/19 19:15:39, dshwang wrote:
> On 2013/09/19 18:58:35, dshwang wrote:
> 
> https://codereview.chromium.org/23834010/ is submitted to note the changed
> semantic of WebLayer::addAnimation() that this CL changes.

lgtm.

[dshwang, 2013-09-20 15:18:28]

On 2013/09/20 15:17:08, Ian Vollick wrote:
> On 2013/09/19 19:15:39, dshwang wrote:
> > On 2013/09/19 18:58:35, dshwang wrote:
> > 
> > https://codereview.chromium.org/23834010/ is submitted to note the changed
> > semantic of WebLayer::addAnimation() that this CL changes.
> 
> lgtm.

thx :)

[commit-bot: I haz the power, 2013-09-20 15:19:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24165003/1

[commit-bot: I haz the power, 2013-09-20 15:31:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[dshwang, 2013-09-20 15:34:11]

@nduca, @jamesr, could you review?

Missing LGTM from an OWNER for these files:
    webkit/renderer/compositor_bindings/web_animation_impl.cc
    webkit/renderer/compositor_bindings/web_animation_impl.h
    webkit/renderer/compositor_bindings/web_layer_impl.cc

[jamesr, 2013-09-23 18:09:31]

Patch itself looks good, but what's the staging plan?  Currently the Blink code
deletes the WebAnimation but we want to have Chromium take ownership and delete
it.  How do you plan to avoid having a state where the object is deleted twice
or not deleted at all?

https://codereview.chromium.org/24165003/diff/1/webkit/renderer/compositor_bi...
File webkit/renderer/compositor_bindings/web_animation_impl.h (right):

https://codereview.chromium.org/24165003/diff/1/webkit/renderer/compositor_bi...
webkit/renderer/compositor_bindings/web_animation_impl.h:43: cc::Animation*
Animation() const {
this doesn't seem all that useful to me - when you're in the .cc, just
dereference animation_

[dshwang, 2013-09-24 14:41:33]

On 2013/09/23 18:09:31, jamesr wrote:
> Patch itself looks good, but what's the staging plan?  Currently the Blink
code
> deletes the WebAnimation but we want to have Chromium take ownership and
delete
> it.  How do you plan to avoid having a state where the object is deleted twice
> or not deleted at all?

After this CL is landed, I'll introduce WebPassOwnPtr to transfer WebAnimation
from Blink to Chromium.
See https://codereview.chromium.org/23455058/

Introducing WebPassOwnPtr needs 4 CLs
1. Add new API in Blink: WebLayer::addAnimation(WebPassOwnPtr<WebAnimation>) {
return false; }
2. Implement new API in Chromium:
WebLayerImpl::addAnimation(WebPassOwnPtr<WebAnimation>)
3. Use new API in Blink: GraphicsLayer can use
WebLayer::addAnimation(WebPassOwnPtr<WebAnimation>) now
4. Clean up old API in Chromium: Remove
WebLayerImpl::addAnimation(WebAnimation*)

Before easy-to-understanding but burdensome above tasks, I want to change all
client code to prepare this ownership transfer semantics.

How do you think?

[dshwang, 2013-09-24 14:49:58]

WebLayerImpl::addAnimation will be as follows:

bool WebLayerImpl::addAnimation(WebPassOwnPtr<WebKit::WebAnimation> animation) {
  return layer_->AddAnimation(
      static_cast<WebAnimationImpl*>(animation)->PassAnimation());
}

If we want more clear code,
bool WebLayerImpl::addAnimation(WebPassOwnPtr<WebKit::WebAnimation>
pop_animation) {
  scoped_ptr<WebKit::WebAnimation> animation(pop_animation.leakPtr());
  return layer_->AddAnimation(
      static_cast<WebAnimationImpl*>(animation)->PassAnimation());
}

[jamesr, 2013-09-24 17:55:05]

I don't think your WebPassOwnPtr<> type is viable (see comments on the other
side), so you'll need to use raw pointers + comments to document the ownership
transfer.  Whenever you do that it will be an incompatible API change and you'll
need to either provide a new entry point or use a macro to guard the change.

I think switching this API to do a proper ownership transfer is a good idea and
you should do it.  I don't think that this intermediate step of transfering the
"guts" of the WebAnimation object but not ownership of the object itself is a
useful intermediate state and think you should just go straight for the final
approach.

[dshwang, 2013-09-24 18:24:57]

On 2013/09/24 17:55:05, jamesr wrote:
> I don't think your WebPassOwnPtr<> type is viable (see comments on the other
> side), so you'll need to use raw pointers + comments to document the ownership
> transfer.  Whenever you do that it will be an incompatible API change and
you'll
> need to either provide a new entry point or use a macro to guard the change.
> 
> I think switching this API to do a proper ownership transfer is a good idea
and
> you should do it.  I don't think that this intermediate step of transfering
the
> "guts" of the WebAnimation object but not ownership of the object itself is a
> useful intermediate state and think you should just go straight for the final
> approach.

Thank you for explanation. I see.
I'll go to the goal directly.
Could you reply to my opinion in crrev.com/23455058 ?

[dshwang, 2013-10-09 16:59:53]

After https://codereview.chromium.org/23834010/ is landed, this CL will pick
Blink commit to sync with Blink.

[jamesr, 2013-10-15 21:14:35]

lgtm

[commit-bot: I haz the power, 2013-10-16 17:09:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24165003/35001

[commit-bot: I haz the power, 2013-10-16 20:37:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24165003/35001

[commit-bot: I haz the power, 2013-10-16 21:21:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24165003/35001

[commit-bot: I haz the power, 2013-10-17 00:13:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24165003/35001

[commit-bot: I haz the power, 2013-10-17 01:27:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24165003/35001

[commit-bot: I haz the power, 2013-10-17 08:55:46]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-17 10:54:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24165003/35001

[commit-bot: I haz the power, 2013-10-17 15:04:35]

Change committed as 229128