Bug 1081007 - Fix relationship between Play/PlayFromJS/PlayFromStyle etc.; r=dholbert

The existing relationship between the particular versions of
AnimationPlayer::Play* (particularly in the CSSAnimationPlayer) subclass are
confusing because, for example, CSSAnimationPlayer::PlayFromStyle needs to be
careful to *not* call Play on CSSAnimationPlayer, but only on the parent
object (since otherwise we reset the sticky pause behavior).

This patch reworks this relationship by adding a protected DoPlay method that
performs the common pausing behavior. Play/PlayFromJS/PlayFromStyle then add
flushing, sticky pausing etc. as necessary.

This patch also removes the UpdateFlags enum and parameters previously used to
control whether we forced an update to style. This is no longer necessary since
we no longer call 'Play' from style. Instead we make Play always post restyles.

If we come across a case where we want to call Play and *not* post restyles, we
can re-add the flags then.

Roughly the same arrangement is true for Pause except that we don't currently
flush styles for CSS animations in PauseFromJS since it currently won't make any
observable difference.

dom/animation/AnimationPlayer.cpp

@@ -67,50 +67,17 @@ AnimationPlayer::PlayState() const
 }
 
 void
-AnimationPlayer::Play(UpdateFlags aUpdateFlags)
+AnimationPlayer::Play()
 {
-  // FIXME: When we implement finishing behavior (bug 1074630) we should
-  // not return early if mIsPaused is false since we may still need to seek.
-  // (However, we will need to pass a flag so that when we start playing due to
-  //  a change in animation-play-state we *don't* trigger finishing behavior.)
-  if (!mIsPaused) {
-    return;
-  }
-  mIsPaused = false;
-
-  Nullable<TimeDuration> timelineTime = mTimeline->GetCurrentTime();
-  if (timelineTime.IsNull()) {
-    // FIXME: We should just sit in the pending state in this case.
-    // We will introduce the pending state in Bug 927349.
-    return;
-  }
-
-  // Update start time to an appropriate offset from the current timeline time
-  MOZ_ASSERT(!mHoldTime.IsNull(), "Hold time should not be null when paused");
-  mStartTime.SetValue(timelineTime.Value() - mHoldTime.Value());
-  mHoldTime.SetNull();
-
-  if (aUpdateFlags == eUpdateStyle) {
-    PostUpdate();
-  }
+  DoPlay();
+  PostUpdate();
 }
 
 void
-AnimationPlayer::Pause(UpdateFlags aUpdateFlags)
+AnimationPlayer::Pause()
 {
-  if (mIsPaused) {
-    return;
-  }
-  mIsPaused = true;
-  mIsRunningOnCompositor = false;
-
-  // Bug 927349 - check for null result here and go to pending state
-  mHoldTime = GetCurrentTime();
-  mStartTime.SetNull();
-
-  if (aUpdateFlags == eUpdateStyle) {
-    PostUpdate();
-  }
+  DoPause();
+  PostUpdate();
 }
 
 Nullable<double>
@@ -194,6 +161,45 @@ AnimationPlayer::ComposeStyle(nsRefPtr<css::AnimValuesStyleRule>& aStyleRule,
   mIsPreviousStateFinished = (playState == AnimationPlayState::Finished);
 }
 
+void
+AnimationPlayer::DoPlay()
+{
+  // FIXME: When we implement finishing behavior (bug 1074630) we should
+  // not return early if mIsPaused is false since we may still need to seek.
+  // (However, we will need to pass a flag so that when we start playing due to
+  //  a change in animation-play-state we *don't* trigger finishing behavior.)
+  if (!mIsPaused) {
+    return;
+  }
+  mIsPaused = false;
+
+  Nullable<TimeDuration> timelineTime = mTimeline->GetCurrentTime();
+  if (timelineTime.IsNull()) {
+    // FIXME: We should just sit in the pending state in this case.
+    // We will introduce the pending state in Bug 927349.
+    return;
+  }
+
+  // Update start time to an appropriate offset from the current timeline time
+  MOZ_ASSERT(!mHoldTime.IsNull(), "Hold time should not be null when paused");
+  mStartTime.SetValue(timelineTime.Value() - mHoldTime.Value());
+  mHoldTime.SetNull();
+}
+
+void
+AnimationPlayer::DoPause()
+{
+  if (mIsPaused) {
+    return;
+  }
+  mIsPaused = true;
+  mIsRunningOnCompositor = false;
+
+  // Bug 927349 - check for null result here and go to pending state
+  mHoldTime = GetCurrentTime();
+  mStartTime.SetNull();
+}
+
 void
 AnimationPlayer::FlushStyle() const
 {

dom/animation/AnimationPlayer.h

@@ -60,29 +60,27 @@ public:
   virtual CSSAnimationPlayer* AsCSSAnimationPlayer() { return nullptr; }
   virtual CSSTransitionPlayer* AsCSSTransitionPlayer() { return nullptr; }
 
-  enum UpdateFlags {
-    eNoUpdate,
-    eUpdateStyle
-  };
-
   // AnimationPlayer methods
   Animation* GetSource() const { return mSource; }
   AnimationTimeline* Timeline() const { return mTimeline; }
   Nullable<double> GetStartTime() const;
   Nullable<TimeDuration> GetCurrentTime() const;
   AnimationPlayState PlayState() const;
-  virtual void Play(UpdateFlags aUpdateFlags);
-  virtual void Pause(UpdateFlags aUpdateFlags);
+  virtual void Play();
+  virtual void Pause();
   bool IsRunningOnCompositor() const { return mIsRunningOnCompositor; }
 
   // Wrapper functions for AnimationPlayer DOM methods when called
   // from script. We often use the same methods internally and from
-  // script but when called from script we perform extra steps such
-  // as flushing style or converting the return type.
+  // script but when called from script we (or one of our subclasses) perform
+  // extra steps such as flushing style or converting the return type.
   Nullable<double> GetCurrentTimeAsDouble() const;
   virtual AnimationPlayState PlayStateFromJS() const { return PlayState(); }
-  virtual void PlayFromJS() { Play(eUpdateStyle); }
-  void PauseFromJS() { Pause(eUpdateStyle); }
+  virtual void PlayFromJS() { Play(); }
+  // PauseFromJS is currently only here for symmetry with PlayFromJS but
+  // in future we will likely have to flush style in
+  // CSSAnimationPlayer::PauseFromJS so we leave it for now.
+  void PauseFromJS() { Pause(); }
 
   void SetSource(Animation* aSource);
   void Tick();
@@ -124,6 +122,9 @@ public:
   Nullable<TimeDuration> mStartTime; // Timeline timescale
 
 protected:
+  void DoPlay();
+  void DoPause();
+
   void FlushStyle() const;
   void PostUpdate();
   StickyTimeDuration SourceContentEnd() const;

layout/style/nsAnimationManager.cpp

@@ -28,17 +28,17 @@ using mozilla::dom::AnimationPlayer;
 using mozilla::CSSAnimationPlayer;
 
 void
-CSSAnimationPlayer::Play(UpdateFlags aUpdateFlags)
+CSSAnimationPlayer::Play()
 {
   mPauseShouldStick = false;
-  AnimationPlayer::Play(aUpdateFlags);
+  AnimationPlayer::Play();
 }
 
 void
-CSSAnimationPlayer::Pause(UpdateFlags aUpdateFlags)
+CSSAnimationPlayer::Pause()
 {
   mPauseShouldStick = true;
-  AnimationPlayer::Pause(aUpdateFlags);
+  AnimationPlayer::Pause();
 }
 
 mozilla::dom::AnimationPlayState
@@ -64,7 +64,7 @@ CSSAnimationPlayer::PlayFromStyle()
 {
   mIsStylePaused = false;
   if (!mPauseShouldStick) {
-    AnimationPlayer::Play(eNoUpdate);
+    DoPlay();
   }
 }
 
@@ -77,7 +77,7 @@ CSSAnimationPlayer::PauseFromStyle()
   }
 
   mIsStylePaused = true;
-  AnimationPlayer::Pause(eNoUpdate);
+  DoPause();
 }
 
 void

layout/style/nsAnimationManager.h

@@ -63,8 +63,8 @@ public:
   virtual CSSAnimationPlayer*
   AsCSSAnimationPlayer() MOZ_OVERRIDE { return this; }
 
-  virtual void Play(UpdateFlags aUpdateFlags) MOZ_OVERRIDE;
-  virtual void Pause(UpdateFlags aUpdateFlags) MOZ_OVERRIDE;
+  virtual void Play() MOZ_OVERRIDE;
+  virtual void Pause() MOZ_OVERRIDE;
 
   virtual dom::AnimationPlayState PlayStateFromJS() const MOZ_OVERRIDE;
   virtual void PlayFromJS() MOZ_OVERRIDE;