Change WebLayer::addAnimation to transfer ownership of WebAnimation.

Change WebLayer::addAnimation to transfer ownership of WebAnimation.

Introduce temporary macro, ANIMATION_OWNERSHIP_TRANSFER, to change
addAnimation() in both Blink and Chromium. After Chromium is aligned to this CL,
ANIMATION_OWNERSHIP_TRANSFER will be removed.

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

[dshwang, 2013-09-19 19:05:02]

It is follow-up CL of https://codereview.chromium.org/24165003/

[jamesr, 2013-09-19 23:23:33]

Ian / Ali - is there any reason why this was a clone and not a transfer of
ownership?  Did you have plans that would require cloning?

[ajuma, 2013-09-19 23:48:35]

On 2013/09/19 23:23:33, jamesr wrote:
> Ian / Ali - is there any reason why this was a clone and not a transfer of
> ownership?  Did you have plans that would require cloning?

There's no reason for it to be a clone now (not sure about why it wound up
this way to begin with).

Note that LinkHighlight hangs on to the WebAnimation that it passes to 
WebLayer::addAnimation, but it doesn't do anything with it. So we should
really just remove LinkHighlight's m_animation instance variable rather
than having it hold on to (what will now be) a hollowed-out WebAnimation.

[dshwang, 2013-09-20 03:31:08]

On 2013/09/19 23:48:35, ajuma wrote:
> On 2013/09/19 23:23:33, jamesr wrote:
> > Ian / Ali - is there any reason why this was a clone and not a transfer of
> > ownership?  Did you have plans that would require cloning?
> 
> There's no reason for it to be a clone now (not sure about why it wound up
> this way to begin with).
> 
> Note that LinkHighlight hangs on to the WebAnimation that it passes to 
> WebLayer::addAnimation, but it doesn't do anything with it. So we should
> really just remove LinkHighlight's m_animation instance variable rather
> than having it hold on to (what will now be) a hollowed-out WebAnimation.

Thank you for your explanation.

Patch Set 2 clarify that the WebAnimation object in both GraphicsLayer and
LinkHighlight is not used after addAnimation().

[Ian Vollick, 2013-09-20 15:19:01]

This lgtm modulo a rebase. @jamesr: There's no longer a reason to clone given
the current plumbing.

https://codereview.chromium.org/23834010/diff/6001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/23834010/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:1717: if
(animations.m_transformAnimation &&
m_graphicsLayer->addAnimation(animations.m_transformAnimation.release()))
This will need to be rebased on top of your previous patch, of course.

[dshwang, 2013-09-20 15:22:20]

Thank you for your review.

https://codereview.chromium.org/23834010/diff/6001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/23834010/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:1717: if
(animations.m_transformAnimation &&
m_graphicsLayer->addAnimation(animations.m_transformAnimation.release()))
On 2013/09/20 15:19:02, Ian Vollick wrote:
> This will need to be rebased on top of your previous patch, of course.

Yes, after CL 24198009 is landed, I'll rebase to upstream.

[jamesr, 2013-09-20 18:52:14]

https://codereview.chromium.org/23834010/diff/6001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/23834010/diff/6001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsLayer.cpp:980: return
platformLayer()->addAnimation(animation.get());
you need to leakPtr() here, right? otherwise ~OwnPtr<> for the 'animation' local
will also delete this causing a double-delete

https://codereview.chromium.org/23834010/diff/6001/Source/web/LinkHighlight.cpp
File Source/web/LinkHighlight.cpp (right):

https://codereview.chromium.org/23834010/diff/6001/Source/web/LinkHighlight.c...
Source/web/LinkHighlight.cpp:277:
m_contentLayer->layer()->addAnimation(animation.get());
this would be clearer if you wrote this line "animation.leakPtr()", since you're
releasing ownership of the object at this point

https://codereview.chromium.org/23834010/diff/6001/public/platform/WebLayer.h
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/23834010/diff/6001/public/platform/WebLayer.h...
public/platform/WebLayer.h:147: // the ownership of internal structure of
WebAnimation to Chromium.
I would simplify this to "The WebLayer takes ownership of the WebAnimation"

[dshwang, 2013-09-20 19:03:57]

Thank you for your comment. I answered.

https://codereview.chromium.org/23834010/diff/6001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/23834010/diff/6001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsLayer.cpp:980: return
platformLayer()->addAnimation(animation.get());
On 2013/09/20 18:52:14, jamesr wrote:
> you need to leakPtr() here, right? otherwise ~OwnPtr<> for the 'animation'
local
> will also delete this causing a double-delete

WebLayerImpl does not delete a WebAnimation instance. WebLayerImpl gains the
ownership of the cc::Animation instance which is the member variable of
WebAnimation instance. So GraphicsLayer deletes the WebAnimation instance like
as-is.

animation.leakPtr() causes memory leak for a while because chomium does not
delete now.  animation.leakPtr() is next step. I'm investigating how to handle
it elegantly.

https://codereview.chromium.org/23834010/diff/6001/Source/web/LinkHighlight.cpp
File Source/web/LinkHighlight.cpp (right):

https://codereview.chromium.org/23834010/diff/6001/Source/web/LinkHighlight.c...
Source/web/LinkHighlight.cpp:277:
m_contentLayer->layer()->addAnimation(animation.get());
On 2013/09/20 18:52:14, jamesr wrote:
> this would be clearer if you wrote this line "animation.leakPtr()", since
you're
> releasing ownership of the object at this point

Ditto.

https://codereview.chromium.org/23834010/diff/6001/public/platform/WebLayer.h
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/23834010/diff/6001/public/platform/WebLayer.h...
public/platform/WebLayer.h:147: // the ownership of internal structure of
WebAnimation to Chromium.
On 2013/09/20 18:52:14, jamesr wrote:
> I would simplify this to "The WebLayer takes ownership of the WebAnimation"

It is because now we does pass ownership of the WebAnimation. we pass just
ownership of cc::Animation that is member of WebAnimation.

[dshwang, 2013-09-20 19:06:31]

On 2013/09/20 19:03:57, dshwang wrote:
> It is because now we does pass ownership of the WebAnimation. we pass just
> ownership of cc::Animation that is member of WebAnimation.

http://crrev.com/24165003/ show how to pass ownership of cc::Animation.
Could you take a look at it?

[jamesr, 2013-09-23 17:53:34]

Hmm, so here the WebAnimation object itself still exists but it is neutered
(i.e. the internal structure is gone)?  That seems a bit iffy since the
WebAnimation object itself won't be useful, so why does blink have to still deal
with it?  I think actually transferring ownership of the object itself to the
WebLayer.

If that's too confusing, maybe we could rephrase the WebKit::WebAnimation API as
more of a builder interface where Blink asks for an object, makes a set of calls
to build up the animation data, then makes a call to set that on the layer? 
That might clarify the object lifetime from Blink's perspective a bit, although
it seems a little muddled compared to just passing an object over.

[dshwang, 2013-09-24 14:44:45]

On 2013/09/23 17:53:34, jamesr wrote:
> Hmm, so here the WebAnimation object itself still exists but it is neutered
> (i.e. the internal structure is gone)?  That seems a bit iffy since the
> WebAnimation object itself won't be useful, so why does blink have to still
deal
> with it?  I think actually transferring ownership of the object itself to the
> WebLayer.
> 
> If that's too confusing, maybe we could rephrase the WebKit::WebAnimation API
as
> more of a builder interface where Blink asks for an object, makes a set of
calls
> to build up the animation data, then makes a call to set that on the layer? 
> That might clarify the object lifetime from Blink's perspective a bit,
although
> it seems a little muddled compared to just passing an object over.

Thank you for your opinion.

I agree it is confusion. This CL is one of whole CL to complete ownership
transfer.
After completing Issue 290217, this API will be very clear.
I explain more detail in https://codereview.chromium.org/24165003/#msg9

[dshwang, 2013-10-09 16:58:45]

now this CL transfer ownership to chromium under
ANIMATION_OWNERSHIP_NOT_TRANSFER guard.
We use raw pointer to transfer ownership as we discussed in
https://codereview.chromium.org/23455058/

Ian@ jamesr@, could you review again?

The counterpart CL of chromium is https://codereview.chromium.org/24165003/

[jamesr, 2013-10-10 00:43:22]

I'm OOO without access to email until next Tuesday. I'll be happy to take a
look then.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

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

https://codereview.chromium.org/23834010/diff/24001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/23834010/diff/24001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:1034: #if
defined(ANIMATION_OWNERSHIP_NOT_TRANSFER)
there's no need to test for this macro in the Blink side of the patch since you
are setting it here.  Just add the new path (which won't need the intermediate 
OwnPtr<> at all)

[dshwang, 2013-10-16 11:40:04]

hi, thank you for review :)

https://codereview.chromium.org/23834010/diff/24001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/23834010/diff/24001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:1034: #if
defined(ANIMATION_OWNERSHIP_NOT_TRANSFER)
On 2013/10/15 21:14:05, jamesr wrote:
> there's no need to test for this macro in the Blink side of the patch since
you
> are setting it here.  Just add the new path (which won't need the intermediate

> OwnPtr<> at all)

I don't think so.
If I changes it to "animation.leakPtr()", Chromium leaks memory until
https://codereview.chromium.org/24165003/ is landed.
Is it accepted?

I'll handle it by following step.
1. land this CL
2. land https://codereview.chromium.org/24165003/
3. remove ANIMATION_OWNERSHIP_NOT_TRANSFER flag in Blink
4. remove ANIMATION_OWNERSHIP_NOT_TRANSFER flag in Chromium

WDYT?

[jamesr, 2013-10-16 15:15:16]

The behavior changes in https://codereview.chromium.org/24165003/ are guarded by
the #define you'll set in the blink patch.  What you should do is land the
chromium-side patch first, which is a no-op until the #define is set, then land
a blink patch that changes behavior and sets the #define, then clean up.

[dshwang, 2013-10-16 15:24:59]

On 2013/10/16 15:15:16, jamesr wrote:
> The behavior changes in https://codereview.chromium.org/24165003/ are guarded
by
> the #define you'll set in the blink patch.  What you should do is land the
> chromium-side patch first, which is a no-op until the #define is set, then
land
> a blink patch that changes behavior and sets the #define, then clean up.

ah, your way need only three steps. great. I buy it. just a minute :)

[dshwang, 2013-10-16 15:32:04]

On 2013/10/16 15:24:59, dshwang wrote:
> On 2013/10/16 15:15:16, jamesr wrote:
> > The behavior changes in https://codereview.chromium.org/24165003/ are
guarded
> by
> > the #define you'll set in the blink patch.  What you should do is land the
> > chromium-side patch first, which is a no-op until the #define is set, then
> land
> > a blink patch that changes behavior and sets the #define, then clean up.
> 
> ah, your way need only three steps. great. I buy it. just a minute :)

After rethinking, your way also needs 4 steps.

1. in chromium
#if guard
  delete animation;
#endif

2. in Blink
#define guard

#if guard
  foo(animation.leakPtr());
#else
  foo(animation.get());
#endif

3. in chromium
- #if guard
  delete animation;
- #endif

4. in Blink
- #define guard

- #if guard
  foo(animation.leakPtr());
- #else
-   foo(animation.get());
- #endif

In addition, I think Blink first is better, because we can sync it in Chromium
as follows:
in DEPS
-"webkit_revision": "159695",
+"webkit_revision": "160000",

WDYT?

[jamesr, 2013-10-16 15:44:18]

On 2013/10/16 15:32:04, dshwang wrote:
> 2. in Blink
> #define guard
> 
> #if guard

It's pointless to have an #if check for a #define in Blink that you will be
setting in Blink.  You need to set the #define in one repo and check it with an
#if in the other repo.  Within Blink, you either have this set or not.

> In addition, I think Blink first is better, because we can sync it in Chromium
> as follows:
> in DEPS
> -"webkit_revision": "159695",
> +"webkit_revision": "160000",

No - don't try to land code changes at the same time as DEPS changes.  It just
leads to lots of pain.

[dshwang, 2013-10-16 17:04:06]

On 2013/10/16 15:44:18, jamesr wrote:
> It's pointless to have an #if check for a #define in Blink that you will be
> setting in Blink.  You need to set the #define in one repo and check it with
an
> #if in the other repo.  Within Blink, you either have this set or not.

I see. you are right.

1. in chromium
#if guard
  delete animation;
#endif

2. in Blink
#define guard

-  foo(animation.get());
+  foo(animation.leakPtr());

2.5 sync using DEP

3. in chromium
- #if guard
  delete animation;
- #endif

4. in Blink
- #define guard

  foo(animation.leakPtr());


I need to change https://codereview.chromium.org/24165003/
 196 #ifndef ANIMATION_OWNERSHIP_NOT_TRANSFER <- this will be #if
defined(ANIMATION_OWNERSHIP_TRANSFER)
 197   delete animation;
 198 #endif

Thank you for your patient :)

[jamesr, 2013-10-16 17:15:44]

Precisely!

Learning how to do cross-repo changes is a skill that we hopefully won't need
forever if we merge repositories.

[dshwang, 2013-10-16 17:16:42]

I changed what you advice. thx.

jamesr@, could you review again? :)

[dshwang, 2013-10-16 17:19:11]

On 2013/10/16 17:15:44, jamesr wrote:
> Precisely!
> 
> Learning how to do cross-repo changes is a skill that we hopefully won't need
> forever if we merge repositories.

wow, there is brave plan!

[jamesr, 2013-10-16 17:22:30]

lgtm

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

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

[dshwang, 2013-10-16 17:30:12]

On 2013/10/16 17:22:30, jamesr wrote:
> lgtm

Thank you!

[commit-bot: I haz the power, 2013-10-17 06:29:25]

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

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

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

[commit-bot: I haz the power, 2013-10-17 17:11:20]

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

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

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

[commit-bot: I haz the power, 2013-10-17 19:26:42]

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

[commit-bot: I haz the power, 2013-10-17 19:29:06]

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

[commit-bot: I haz the power, 2013-10-17 22:41:10]

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

[commit-bot: I haz the power, 2013-10-17 23:02:23]

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

[commit-bot: I haz the power, 2013-10-18 02:19:35]

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

[commit-bot: I haz the power, 2013-10-18 07:14:46]

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

[commit-bot: I haz the power, 2013-10-18 19:40:49]

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

[commit-bot: I haz the power, 2013-10-18 20:15:08]

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

[commit-bot: I haz the power, 2013-10-21 10:42:01]

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

[commit-bot: I haz the power, 2013-10-21 12:25:38]

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

[commit-bot: I haz the power, 2013-10-21 12:55:45]

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

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

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

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

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

[commit-bot: I haz the power, 2013-10-21 19:13:22]

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

[commit-bot: I haz the power, 2013-10-21 19:26:10]

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

[commit-bot: I haz the power, 2013-10-22 00:31:19]

Change committed as 160110