Implement new AnimationTimeline superclass

Implement new AnimationTimeline superclass

Most of the logic in the AnimationTimeline class really belongs in
DocumentTimeline, having been written before the distinction was
introduced to the spec.

This patch introduces a new, abstract superclass (called
SuperAnimationTimeline) that provides only the bare minimum to satisfy
the AnimationTimeline.idl interface. The patch changes
AnimationTimeline.idl to be implemented by SuperAnimationTimeline.h and
changes DocumentTimeline.idl to be implemented by AnimationTimeline.h.
The patch introduces no behaviour change.

A follow-up patch will rename SuperAnimationTimeline to
AnimationTimeline and AnimationTimeline to DocumentTimeline.

BUG=624639, 600248
Review-Url: https://codereview.chromium.org/2944423003
Cr-Commit-Position: refs/heads/master@{#482520}
Committed: https://chromium.googlesource.com/chromium/src/+/7ef6f8b862ad3fcc5f3e9634f77031d14d73862c

[suzyh_UTC10 (ex-contributor), 2017-06-21 05:04:56]

suzyh@chromium.org changed reviewers:
+ alancutter@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-06-21 05:05:56]

Hi Alan,

What do you think of this approach? Note it depends on a couple of other
patchsets to simplify AnimationTimeline.idl first.

[suzyh_UTC10 (ex-contributor), 2017-06-21 05:06:02]

The CQ bit was checked by suzyh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 05:06:19]

Dry run: 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, 2017-06-21 05:08:54]

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

[commit-bot: I haz the power, 2017-06-21 05:08:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alancutter (OOO until 2018), 2017-06-21 05:36:17]

Looks good so far, I'd like to see the next patch before this lands though.

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

https://codereview.chromium.org/2944423003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/Animation.cpp:79: AnimationTimeline*
subtimeline = ToAnimationTimeline(timeline);
Add TODO to remove the dependency on (future) DocumentTimeline.

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

https://codereview.chromium.org/2944423003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/Animation.h:145: AnimationTimeline*
TimelineInternal() { return timeline_; }
Add TODO to remove the dependency on (future) DocumentTimeline.

[alancutter (OOO until 2018), 2017-06-22 01:21:16]

lgtm

[suzyh_UTC10 (ex-contributor), 2017-06-26 03:43:29]

The CQ bit was checked by suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-06-26 03:43:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2944423003/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-26 03:43:42]

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, 2017-06-26 03:53:11]

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

[commit-bot: I haz the power, 2017-06-26 03:53:12]

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

[suzyh_UTC10 (ex-contributor), 2017-06-26 03:59:35]

The CQ bit was checked by suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-06-26 03:59:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2944423003/#ps40001
(title: "Fix syntax error in IDL files")

[commit-bot: I haz the power, 2017-06-26 03:59:42]

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, 2017-06-26 06:38:26]

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

[commit-bot: I haz the power, 2017-06-26 06:38:31]

Failed to apply patch for
third_party/WebKit/Source/core/animation/Animation.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/core/animation/Animation.cpp:769
  Falling back to three-way merge...
  Applied patch to 'third_party/WebKit/Source/core/animation/Animation.cpp' with
conflicts.
  U third_party/WebKit/Source/core/animation/Animation.cpp

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
355364e365b0103bb0452ffbc7abb170dfe8cb96..6fff06190a87bad0336044ac6c36076b20930293
100644
--- a/third_party/WebKit/Source/core/animation/Animation.cpp
+++ b/third_party/WebKit/Source/core/animation/Animation.cpp
@@ -34,6 +34,7 @@
 #include "core/animation/CompositorPendingAnimations.h"
 #include "core/animation/DocumentTimeline.h"
 #include "core/animation/KeyframeEffectReadOnly.h"
+#include "core/animation/SuperAnimationTimeline.h"
 #include "core/animation/css/CSSAnimations.h"
 #include "core/dom/DOMNodeIds.h"
 #include "core/dom/Document.h"
@@ -68,18 +69,20 @@ static unsigned NextSequenceNumber() {
 }
 
 Animation* Animation::Create(AnimationEffectReadOnly* effect,
-                             AnimationTimeline* timeline) {
-  if (!timeline) {
+                             SuperAnimationTimeline* timeline) {
+  if (!timeline || !timeline->IsAnimationTimeline()) {
     // FIXME: Support creating animations without a timeline.
     NOTREACHED();
     return nullptr;
   }
 
+  AnimationTimeline* subtimeline = ToAnimationTimeline(timeline);
+
   Animation* animation = new Animation(
-      timeline->GetDocument()->ContextDocument(), *timeline, effect);
+      subtimeline->GetDocument()->ContextDocument(), *subtimeline, effect);
 
-  if (timeline) {
-    timeline->AnimationAttached(*animation);
+  if (subtimeline) {
+    subtimeline->AnimationAttached(*animation);
     animation->AttachCompositorTimeline();
   }
 
@@ -97,7 +100,7 @@ Animation* Animation::Create(ExecutionContext*
execution_context,
 
 Animation* Animation::Create(ExecutionContext* execution_context,
                              AnimationEffectReadOnly* effect,
-                             AnimationTimeline* timeline,
+                             SuperAnimationTimeline* timeline,
                              ExceptionState& exception_state) {
   DCHECK(RuntimeEnabledFeatures::WebAnimationsAPIEnabled());
 
@@ -769,7 +772,7 @@ bool Animation::CanStartAnimationOnCompositorInternal(
 
   // FIXME: Timeline playback rates should be compositable
   if (playback_rate_ == 0 || (std::isinf(EffectEnd()) && playback_rate_ < 0) ||
-      (timeline() && timeline()->PlaybackRate() != 1))
+      (TimelineInternal() && TimelineInternal()->PlaybackRate() != 1))
     return false;
 
   if (!timeline_ || !content_ || !content_->IsKeyframeEffectReadOnly())
@@ -811,7 +814,7 @@ void Animation::StartAnimationOnCompositor(
 
   bool reversed = playback_rate_ < 0;
 
-  double start_time = timeline()->ZeroTime() + StartTimeInternal();
+  double start_time = TimelineInternal()->ZeroTime() + StartTimeInternal();
   if (reversed) {
     start_time -= EffectEnd() / fabs(playback_rate_);
   }
@@ -844,7 +847,8 @@ void Animation::SetCompositorPending(bool effect_changed) {
       compositor_state_->playback_rate != playback_rate_ ||
       compositor_state_->start_time != start_time_) {
     compositor_pending_ = true;
-    timeline()->GetDocument()->GetCompositorPendingAnimations().Add(this);
+    TimelineInternal()->GetDocument()->GetCompositorPendingAnimations().Add(
+        this);
   }
 }
 
@@ -900,8 +904,9 @@ bool Animation::Update(TimingUpdateReason reason) {
         const AtomicString& event_type = EventTypeNames::cancel;
         if (GetExecutionContext() && HasEventListeners(event_type)) {
           double event_current_time = NullValue();
-          pending_cancelled_event_ = AnimationPlaybackEvent::Create(
-              event_type, event_current_time, timeline()->currentTime());
+          pending_cancelled_event_ =
+              AnimationPlaybackEvent::Create(event_type, event_current_time,
+                                            
TimelineInternal()->currentTime());
           pending_cancelled_event_->SetTarget(this);
           pending_cancelled_event_->SetCurrentTarget(this);
           timeline_->GetDocument()->EnqueueAnimationFrameEvent(
@@ -911,8 +916,9 @@ bool Animation::Update(TimingUpdateReason reason) {
         const AtomicString& event_type = EventTypeNames::finish;
         if (GetExecutionContext() && HasEventListeners(event_type)) {
           double event_current_time = CurrentTimeInternal() * 1000;
-          pending_finished_event_ = AnimationPlaybackEvent::Create(
-              event_type, event_current_time, timeline()->currentTime());
+          pending_finished_event_ =
+              AnimationPlaybackEvent::Create(event_type, event_current_time,
+                                            
TimelineInternal()->currentTime());
           pending_finished_event_->SetTarget(this);
           pending_finished_event_->SetCurrentTarget(this);
           timeline_->GetDocument()->EnqueueAnimationFrameEvent(
@@ -1024,7 +1030,7 @@ void Animation::DetachCompositedLayers() {
 }
 
 void Animation::NotifyAnimationStarted(double monotonic_time, int group) {
-  timeline()
+  TimelineInternal()
       ->GetDocument()
       ->GetCompositorPendingAnimations()
       .NotifyCompositorAnimationStarted(monotonic_time, group);
@@ -1128,9 +1134,9 @@ Animation::PlayStateUpdateScope::~PlayStateUpdateScope() {
   animation_->EndUpdatingState();
 
   if (old_play_state != new_play_state) {
-    probe::animationPlayStateChanged(animation_->timeline()->GetDocument(),
-                                     animation_, old_play_state,
-                                     new_play_state);
+    probe::animationPlayStateChanged(
+        animation_->TimelineInternal()->GetDocument(), animation_,
+        old_play_state, new_play_state);
   }
 }

[suzyh_UTC10 (ex-contributor), 2017-06-26 07:10:30]

The CQ bit was checked by suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-06-26 07:10:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2944423003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-26 07:10:45]

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, 2017-06-26 08:02:26]

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

[commit-bot: I haz the power, 2017-06-26 08:02:27]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[suzyh_UTC10 (ex-contributor), 2017-06-27 00:35:34]

The CQ bit was checked by suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-06-27 00:35:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2944423003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-27 00:35:48]

CQ is trying da patch.

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

[smcgruer, 2017-06-27 02:01:22]

Description was changed from

==========
Implement new AnimationTimeline superclass

Most of the logic in the AnimationTimeline class really belongs in
DocumentTimeline, having been written before the distinction was
introduced to the spec.

This patch introduces a new, abstract superclass (called
SuperAnimationTimeline) that provides only the bare minimum to satisfy
the AnimationTimeline.idl interface. The patch changes
AnimationTimeline.idl to be implemented by SuperAnimationTimeline.h and
changes DocumentTimeline.idl to be implemented by AnimationTimeline.h.
The patch introduces no behaviour change.

A follow-up patch will rename SuperAnimationTimeline to
AnimationTimeline and AnimationTimeline to DocumentTimeline.

BUG=624639,600248
==========

to

==========
Implement new AnimationTimeline superclass

Most of the logic in the AnimationTimeline class really belongs in
DocumentTimeline, having been written before the distinction was
introduced to the spec.

This patch introduces a new, abstract superclass (called
SuperAnimationTimeline) that provides only the bare minimum to satisfy
the AnimationTimeline.idl interface. The patch changes
AnimationTimeline.idl to be implemented by SuperAnimationTimeline.h and
changes DocumentTimeline.idl to be implemented by AnimationTimeline.h.
The patch introduces no behaviour change.

A follow-up patch will rename SuperAnimationTimeline to
AnimationTimeline and AnimationTimeline to DocumentTimeline.

BUG=624639,600248
==========

[commit-bot: I haz the power, 2017-06-27 02:35:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498523734548430,
"parent_rev": "e683d49f011eabc28844a825588aa3f0fe50f2c4", "commit_rev":
"7ef6f8b862ad3fcc5f3e9634f77031d14d73862c"}

[commit-bot: I haz the power, 2017-06-27 02:35:13]

Description was changed from

==========
Implement new AnimationTimeline superclass

Most of the logic in the AnimationTimeline class really belongs in
DocumentTimeline, having been written before the distinction was
introduced to the spec.

This patch introduces a new, abstract superclass (called
SuperAnimationTimeline) that provides only the bare minimum to satisfy
the AnimationTimeline.idl interface. The patch changes
AnimationTimeline.idl to be implemented by SuperAnimationTimeline.h and
changes DocumentTimeline.idl to be implemented by AnimationTimeline.h.
The patch introduces no behaviour change.

A follow-up patch will rename SuperAnimationTimeline to
AnimationTimeline and AnimationTimeline to DocumentTimeline.

BUG=624639,600248
==========

to

==========
Implement new AnimationTimeline superclass

Most of the logic in the AnimationTimeline class really belongs in
DocumentTimeline, having been written before the distinction was
introduced to the spec.

This patch introduces a new, abstract superclass (called
SuperAnimationTimeline) that provides only the bare minimum to satisfy
the AnimationTimeline.idl interface. The patch changes
AnimationTimeline.idl to be implemented by SuperAnimationTimeline.h and
changes DocumentTimeline.idl to be implemented by AnimationTimeline.h.
The patch introduces no behaviour change.

A follow-up patch will rename SuperAnimationTimeline to
AnimationTimeline and AnimationTimeline to DocumentTimeline.

BUG=624639,600248

Review-Url: https://codereview.chromium.org/2944423003
Cr-Commit-Position: refs/heads/master@{#482520}
Committed:
https://chromium.googlesource.com/chromium/src/+/7ef6f8b862ad3fcc5f3e9634f770...
==========

[commit-bot: I haz the power, 2017-06-27 02:35:14]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7ef6f8b862ad3fcc5f3e9634f770...