Avoid conditional Animation prefinalizers.

Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=673645
Committed: https://crrev.com/4a50af8bd437d768140c896e644d4a3918c7f2d0
Cr-Commit-Position: refs/heads/master@{#438089}

[sof, 2016-12-12 12:50:54]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 12:51:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2016-12-12 13:33:29]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2016-12-12 13:33:29]

please take a look.

see https://codereview.chromium.org/2565983002/#msg22 (and tha CL) for
motivation.

[commit-bot: I haz the power, 2016-12-12 14:40:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 14:40:06]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[haraken, 2016-12-12 15:33:29]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-12-12 15:33:29]

LGTM

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/Animation.cpp:1143:
DCHECK(!m_animation);

Hmm? You need to clear m_animation after line 1142, right?

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/Animation.h (right):

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/Animation.h:302: //
CompositorAnimationPlayer objects need to eagerly sever

serve

[sof, 2016-12-12 15:41:22]

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/Animation.cpp:1143:
DCHECK(!m_animation);
On 2016/12/12 15:33:29, haraken wrote:
> 
> Hmm? You need to clear m_animation after line 1142, right?
> 

It's done via detach(), which the Animation disposal calls.
 
(Animation::destroyCompositorPlayer() needs to be supported still, which makes
the dependencies a bit tight. Cleanest way I could think of.)

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/Animation.h (right):

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/Animation.h:302: //
CompositorAnimationPlayer objects need to eagerly sever
On 2016/12/12 15:33:29, haraken wrote:
> 
> serve

We're severing the connection.

[haraken, 2016-12-12 15:44:41]

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2570503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/Animation.cpp:1143:
DCHECK(!m_animation);
On 2016/12/12 15:41:22, sof wrote:
> On 2016/12/12 15:33:29, haraken wrote:
> > 
> > Hmm? You need to clear m_animation after line 1142, right?
> > 
> 
> It's done via detach(), which the Animation disposal calls.
>  
> (Animation::destroyCompositorPlayer() needs to be supported still, which makes
> the dependencies a bit tight. Cleanest way I could think of.)

ah, got it.

[sof, 2016-12-12 15:49:19]

Description was changed from

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=
BUG=
==========

to

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=
==========

[sof, 2016-12-12 15:49:24]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-12 15:49:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 16:38:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 16:38:18]

Failed to apply patch for
third_party/WebKit/Source/core/animation/Animation.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/animation/Animation.cpp:1126
  error: third_party/WebKit/Source/core/animation/Animation.cpp: patch does not
apply

Patch:       third_party/WebKit/Source/core/animation/Animation.cpp
Index: third_party/WebKit/Source/core/animation/Animation.cpp
diff --git a/third_party/WebKit/Source/core/animation/Animation.cpp
b/third_party/WebKit/Source/core/animation/Animation.cpp
index
f6ba49b2c9d18e2958dc412ddce925d12d139838..278b1b85e0baca4f1aabeb32f080cd39db4b49e3
100644
--- a/third_party/WebKit/Source/core/animation/Animation.cpp
+++ b/third_party/WebKit/Source/core/animation/Animation.cpp
@@ -99,7 +99,6 @@ Animation::Animation(ExecutionContext* executionContext,
       m_compositorState(nullptr),
       m_compositorPending(false),
       m_compositorGroup(0),
-      m_preFinalizerRegistered(false),
       m_currentTimePending(false),
       m_stateIsBeingUpdated(false),
       m_effectSuppressed(false) {
@@ -115,7 +114,8 @@ Animation::Animation(ExecutionContext* executionContext,
 }
 
 Animation::~Animation() {
-  destroyCompositorPlayer();
+  // Verify that m_compositorPlayer has been disposed of.
+  DCHECK(!m_compositorPlayer);
 }
 
 void Animation::dispose() {
@@ -910,17 +910,9 @@ void Animation::endUpdatingState() {
 void Animation::createCompositorPlayer() {
   if (Platform::current()->isThreadedAnimationEnabled() &&
       !m_compositorPlayer) {
-    // We only need to pre-finalize if we are running animations on the
-    // compositor.
-    if (!m_preFinalizerRegistered) {
-      ThreadState::current()->registerPreFinalizer(this);
-      m_preFinalizerRegistered = true;
-    }
-
     DCHECK(Platform::current()->compositorSupport());
-    m_compositorPlayer = CompositorAnimationPlayer::create();
+    m_compositorPlayer = CompositorAnimationPlayerHolder::create(this);
     DCHECK(m_compositorPlayer);
-    m_compositorPlayer->setAnimationDelegate(this);
     attachCompositorTimeline();
   }
 
@@ -932,8 +924,8 @@ void Animation::destroyCompositorPlayer() {
 
   if (m_compositorPlayer) {
     detachCompositorTimeline();
-    m_compositorPlayer->setAnimationDelegate(nullptr);
-    m_compositorPlayer.reset();
+    m_compositorPlayer->detach();
+    m_compositorPlayer = nullptr;
   }
 }
 
@@ -966,8 +958,8 @@ void Animation::attachCompositedLayers() {
 }
 
 void Animation::detachCompositedLayers() {
-  if (m_compositorPlayer && m_compositorPlayer->isElementAttached())
-    m_compositorPlayer->detachElement();
+  if (m_compositorPlayer && m_compositorPlayer->player()->isElementAttached())
+    m_compositorPlayer->player()->detachElement();
 }
 
 void Animation::notifyAnimationStarted(double monotonicTime, int group) {
@@ -1126,8 +1118,36 @@ DEFINE_TRACE(Animation) {
   visitor->trace(m_pendingCancelledEvent);
   visitor->trace(m_finishedPromise);
   visitor->trace(m_readyPromise);
+  visitor->trace(m_compositorPlayer);
   EventTargetWithInlineData::trace(visitor);
   ActiveDOMObject::trace(visitor);
 }
 
+Animation::CompositorAnimationPlayerHolder*
+Animation::CompositorAnimationPlayerHolder::create(Animation* animation) {
+  return new CompositorAnimationPlayerHolder(animation);
+}
+
+Animation::CompositorAnimationPlayerHolder::CompositorAnimationPlayerHolder(
+    Animation* animation)
+    : m_animation(animation) {
+  ThreadState::current()->registerPreFinalizer(this);
+  m_compositorPlayer = CompositorAnimationPlayer::create();
+  m_compositorPlayer->setAnimationDelegate(m_animation);
+}
+
+void Animation::CompositorAnimationPlayerHolder::dispose() {
+  if (!m_animation)
+    return;
+  m_animation->dispose();
+  DCHECK(!m_animation);
+  DCHECK(!m_compositorPlayer);
+}
+
+void Animation::CompositorAnimationPlayerHolder::detach() {
+  DCHECK(m_compositorPlayer);
+  m_compositorPlayer->setAnimationDelegate(nullptr);
+  m_animation = nullptr;
+  m_compositorPlayer.reset();
+}
 }  // namespace blink

[sof, 2016-12-12 18:16:09]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-12-12 18:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2570503002/#ps20001
(title: "rebased upto r437900")

[commit-bot: I haz the power, 2016-12-12 18:16:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 23:50:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 23:50:04]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2016-12-13 06:20:25]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-13 06:20:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2016-12-13 06:31:29]

Description was changed from

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=
==========

to

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=673645
==========

[commit-bot: I haz the power, 2016-12-13 07:08:18]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481610025045860,
"parent_rev": "a423abb292a9aaf100c96e9b428f748626521dd5", "commit_rev":
"ef1a53a00b5b9336b0d3733a9c2a00aa185fbea4"}

[commit-bot: I haz the power, 2016-12-13 07:08:52]

Description was changed from

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=673645
==========

to

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=673645

Review-Url: https://codereview.chromium.org/2570503002
==========

[commit-bot: I haz the power, 2016-12-13 07:08:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-13 07:15:12]

Description was changed from

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=673645

Review-Url: https://codereview.chromium.org/2570503002
==========

to

==========
Avoid conditional Animation prefinalizers.

Recast the conditionally-eager finalization of Animation objects - only
needed if the Animation object has a CompositorAnimationPlayer attached -
wrapping instead the player object inside an eagerly-finalized object.

By doing so, we remove the need to support explicit prefinalizer
registration.

R=haraken
BUG=673645

Committed: https://crrev.com/4a50af8bd437d768140c896e644d4a3918c7f2d0
Cr-Commit-Position: refs/heads/master@{#438089}
==========

[commit-bot: I haz the power, 2016-12-13 07:15:13]

Patchset 2 (id:??) landed as
https://crrev.com/4a50af8bd437d768140c896e644d4a3918c7f2d0
Cr-Commit-Position: refs/heads/master@{#438089}