Web Animations: Make AnimationPlayer an ActiveDOMObject

Web Animations: Make AnimationPlayer an ActiveDOMObject

Previously the AnimationPlayer's finish event could have been dispatched
after the event handlers had been collected. As these events are queued
then dispatched asynchronously via ScriptedAnimationController some
special handling is required (tracking when the event is actually
dispatched).

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

[haraken, 2014-05-23 07:15:05]

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:59: :
ActiveDOMObject(timeline.document()->contextDocument().get())

Nit: This is a bit redundant way to get an ExecutionContext. I'd explicitly pass
an ExecutionContext* parameter when the AnimatoinTimeline creates the
AnimationPlayer object.

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:84: virtual bool hasPendingActivity()
const OVERRIDE;

Don't we need to override ActiveDOMObject::stop()/resume()? I guess you need to
clear the event when the ActiveDOMObject is stopped, for example.

[dstockwell, 2014-05-26 00:47:50]

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:59: :
ActiveDOMObject(timeline.document()->contextDocument().get())
On 2014/05/23 07:15:05, haraken wrote:
> 
> Nit: This is a bit redundant way to get an ExecutionContext. I'd explicitly
pass
> an ExecutionContext* parameter when the AnimatoinTimeline creates the
> AnimationPlayer object.

Done.

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:84: virtual bool hasPendingActivity()
const OVERRIDE;
On 2014/05/23 07:15:05, haraken wrote:
> 
> Don't we need to override ActiveDOMObject::stop()/resume()? I guess you need
to
> clear the event when the ActiveDOMObject is stopped, for example.

Yes, I suppose stop() is necessary. suspend/resume are handled by
ScriptedAnimationController.

[haraken, 2014-05-26 01:16:08]

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:84: virtual bool hasPendingActivity()
const OVERRIDE;
On 2014/05/26 00:47:50, dstockwell wrote:
> On 2014/05/23 07:15:05, haraken wrote:
> > 
> > Don't we need to override ActiveDOMObject::stop()/resume()? I guess you need
> to
> > clear the event when the ActiveDOMObject is stopped, for example.
> 
> Yes, I suppose stop() is necessary. suspend/resume are handled by
> ScriptedAnimationController.

Makes sense.

- I'm just curious, but why are suspend/resume handled by
ScriptedAnimationController instead of using the infrastructure of
ActiveDOMObject?

- ScriptedAnimationController's suspend/resume does nothing about
AnimationPlayer. This is expected, right? (For example, you don't need to clear
m_pendingFinishedEvent when the Document is suspended.)

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:301: m_pendingFinishedEvent = nullptr;

Just to confirm: You don't need to change m_finished to true, right?

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:85: virtual void stop();

Add OVERRIDE.

[dstockwell, 2014-05-26 01:32:39]

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/284323004/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:84: virtual bool hasPendingActivity()
const OVERRIDE;
On 2014/05/26 01:16:09, haraken wrote:
> On 2014/05/26 00:47:50, dstockwell wrote:
> > On 2014/05/23 07:15:05, haraken wrote:
> > > 
> > > Don't we need to override ActiveDOMObject::stop()/resume()? I guess you
need
> > to
> > > clear the event when the ActiveDOMObject is stopped, for example.
> > 
> > Yes, I suppose stop() is necessary. suspend/resume are handled by
> > ScriptedAnimationController.
> 
> Makes sense.
> 
> - I'm just curious, but why are suspend/resume handled by
> ScriptedAnimationController instead of using the infrastructure of
> ActiveDOMObject?

There's nothing that we need to suspend here. Animation is not involved with
ActiveDOMObject, it's just the event dispatch that we need to be concerned with,
and it's the ScriptedAnimationController that does the event dispatch.

> - ScriptedAnimationController's suspend/resume does nothing about
> AnimationPlayer. This is expected, right? (For example, you don't need to
clear
> m_pendingFinishedEvent when the Document is suspended.)

Right, we don't need to clear it on suspend. We're just using
m_pendingFinishedEvent to track the current event, it would still be dispatched
after suspend/resume for example.

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:301: m_pendingFinishedEvent = nullptr;
On 2014/05/26 01:16:09, haraken wrote:
> 
> Just to confirm: You don't need to change m_finished to true, right?

Correct, this is not needed here.

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:85: virtual void stop();
On 2014/05/26 01:16:09, haraken wrote:
> 
> Add OVERRIDE.

Done.

[haraken, 2014-05-26 02:37:02]

Thanks for the clarification, LGTM!

[dstockwell, 2014-05-26 03:08:04]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-05-26 03:08:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dstockwell@chromium.org/284323004/80001

[dstockwell, 2014-05-26 03:26:55]

The CQ bit was unchecked by dstockwell@chromium.org

[dstockwell, 2014-05-28 01:28:44]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-05-28 01:29:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dstockwell@chromium.org/284323004/100001

[commit-bot: I haz the power, 2014-05-28 01:29:34]

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

[commit-bot: I haz the power, 2014-05-28 01:29:35]

Failed to apply patch for Source/core/animation/AnimationPlayer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/animation/AnimationPlayer.cpp
  Hunk #1 FAILED at 48.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/animation/AnimationPlayer.cpp.rej

Patch:       Source/core/animation/AnimationPlayer.cpp
Index: Source/core/animation/AnimationPlayer.cpp
diff --git a/Source/core/animation/AnimationPlayer.cpp
b/Source/core/animation/AnimationPlayer.cpp
index
4fa0bf6b782bfe7614c698f3c4b6aa57f9e88a5d..f6a1053971b293997bd126c841b0a7193c21ee43
100644
--- a/Source/core/animation/AnimationPlayer.cpp
+++ b/Source/core/animation/AnimationPlayer.cpp
@@ -48,13 +48,16 @@ static unsigned nextSequenceNumber()
 
 }
 
-PassRefPtrWillBeRawPtr<AnimationPlayer>
AnimationPlayer::create(AnimationTimeline& timeline, AnimationSource* content)
+PassRefPtrWillBeRawPtr<AnimationPlayer>
AnimationPlayer::create(ExecutionContext* executionContext, AnimationTimeline&
timeline, AnimationSource* content)
 {
-    return adoptRefWillBeRefCountedGarbageCollected(new
AnimationPlayer(timeline, content));
+    RefPtrWillBeRawPtr<AnimationPlayer> player =
adoptRefWillBeRefCountedGarbageCollected(new AnimationPlayer(executionContext,
timeline, content));
+    player->suspendIfNeeded();
+    return player.release();
 }
 
-AnimationPlayer::AnimationPlayer(AnimationTimeline& timeline, AnimationSource*
content)
-    : m_playbackRate(1)
+AnimationPlayer::AnimationPlayer(ExecutionContext* executionContext,
AnimationTimeline& timeline, AnimationSource* content)
+    : ActiveDOMObject(executionContext)
+    , m_playbackRate(1)
     , m_startTime(nullValue())
     , m_holdTime(nullValue())
     , m_storedTimeLag(0)
@@ -285,11 +288,24 @@ const AtomicString& AnimationPlayer::interfaceName() const
 
 ExecutionContext* AnimationPlayer::executionContext() const
 {
-    if (m_timeline) {
-        if (Document* document = m_timeline->document())
-            return document->contextDocument().get();
-    }
-    return 0;
+    return ActiveDOMObject::executionContext();
+}
+
+bool AnimationPlayer::hasPendingActivity() const
+{
+    return m_pendingFinishedEvent || (!m_finished &&
hasEventListeners(EventTypeNames::finish));
+}
+
+void AnimationPlayer::stop()
+{
+    m_pendingFinishedEvent = nullptr;
+}
+
+bool AnimationPlayer::dispatchEvent(PassRefPtrWillBeRawPtr<Event> event)
+{
+    if (m_pendingFinishedEvent == event)
+        m_pendingFinishedEvent = nullptr;
+    return EventTargetWithInlineData::dispatchEvent(event);
 }
 
 void AnimationPlayer::setPlaybackRate(double playbackRate)
@@ -367,10 +383,10 @@ bool AnimationPlayer::update(TimingUpdateReason reason)
         if (reason == TimingUpdateForAnimationFrame && hasStartTime()) {
             const AtomicString& eventType = EventTypeNames::finish;
             if (executionContext() && hasEventListeners(eventType)) {
-                RefPtrWillBeRawPtr<AnimationPlayerEvent> event =
AnimationPlayerEvent::create(eventType, currentTime(),
timeline()->currentTime());
-                event->setTarget(this);
-                event->setCurrentTarget(this);
-               
m_timeline->document()->enqueueAnimationFrameEvent(event.release());
+                m_pendingFinishedEvent =
AnimationPlayerEvent::create(eventType, currentTime(),
timeline()->currentTime());
+                m_pendingFinishedEvent->setTarget(this);
+                m_pendingFinishedEvent->setCurrentTarget(this);
+               
m_timeline->document()->enqueueAnimationFrameEvent(m_pendingFinishedEvent);
             }
             m_finished = true;
         }

[dstockwell, 2014-05-28 05:38:30]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-05-28 05:38:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dstockwell@chromium.org/284323004/120001

[commit-bot: I haz the power, 2014-05-28 07:02:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9413)

[commit-bot: I haz the power, 2014-05-28 08:02:28]

Change committed as 174943

[kouhei (in TOK), 2014-05-30 02:04:15]

I think this patch lead to leak:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[kouhei (in TOK), 2014-05-30 02:09:37]

On 2014/05/30 02:04:15, kouhei wrote:
> I think this patch lead to leak:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

Filed bug: https://code.google.com/p/chromium/issues/detail?id=379032

[haraken, 2014-05-30 02:54:01]

On 2014/05/30 02:09:37, kouhei wrote:
> On 2014/05/30 02:04:15, kouhei wrote:
> > I think this patch lead to leak:
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
> 
> Filed bug: https://code.google.com/p/chromium/issues/detail?id=379032

The leak detector detected a new leak! Great :)

[dstockwell, 2014-05-30 09:28:21]

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/284323004/diff/60001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:301: m_pendingFinishedEvent = nullptr;
On 2014/05/26 01:32:40, dstockwell wrote:
> On 2014/05/26 01:16:09, haraken wrote:
> > 
> > Just to confirm: You don't need to change m_finished to true, right?
> 
> Correct, this is not needed here.

I was wrong. I thought m_finished only needed to become true to dispatch the
event, but we do need to set it here in order to satisfy the conditions of
hasPendingActivity() correctly after stop(). That's the reason for the leak.

Will send a followup patch.