Bug 1541767 - Drop Animation::CancelNoUpdate; r=hiro
authorBrian Birtles <birtles@gmail.com>
Thu, 18 Apr 2019 06:24:52 +0000
changeset 470031 67900e2680ff84192ebfd3b5d0c5124ddc49c624
parent 470030 25f3c9bd8831811d6b1c585071cf99fe7913514a
child 470032 a751128e51f71ddd7df3eaa1ba8c5c34b6876e31
push id112839
push userapavel@mozilla.com
push dateThu, 18 Apr 2019 21:50:57 +0000
treeherdermozilla-inbound@e0a826fcd85b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1541767
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1541767 - Drop Animation::CancelNoUpdate; r=hiro CancelNoUpdate actually can and does trigger restyles via its call to KeyframeEffect::NotifyAnimationTimingUpdated so at very least its name is wrong. Furthermore, we actually want canceling to trigger restyles in most cases since when an animation is canceled we need to trigger a subsequent restyle to apply the (no-longer-animated) result. This wasn't necessary when CancelNoUpdate was first introduced but since then we have introduced the Servo style engine where we use a separate traversal to apply the result from creating/deleting/modifying animations. This change will mean that we now trigger a "layer" restyle when canceling an animation when we previously didn't. That, however, seems more correct if anything. This patch also makes CancelFromStyle no longer virtual since it doesn't seem necessary anymore (perhaps because we now point to the concrete type: CSSAnimation/CSSTransition from nsAnimationManager/nsTransitionManager whereas previously we didn't). Differential Revision: https://phabricator.services.mozilla.com/D28019
dom/animation/Animation.cpp
dom/animation/Animation.h
layout/style/nsAnimationManager.h
layout/style/nsTransitionManager.h
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -462,18 +462,43 @@ Promise* Animation::GetFinished(ErrorRes
     return nullptr;
   }
   if (mFinishedIsResolved) {
     MaybeResolveFinishedPromise();
   }
   return mFinished;
 }
 
+// https://drafts.csswg.org/web-animations/#cancel-an-animation
 void Animation::Cancel() {
-  CancelNoUpdate();
+  if (PlayState() != AnimationPlayState::Idle) {
+    ResetPendingTasks();
+
+    if (mFinished) {
+      mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
+    }
+    ResetFinishedPromise();
+
+    QueuePlaybackEvent(NS_LITERAL_STRING("cancel"),
+                       GetTimelineCurrentTimeAsTimeStamp());
+  }
+
+  StickyTimeDuration activeTime =
+      mEffect ? mEffect->GetComputedTiming().mActiveTime : StickyTimeDuration();
+
+  mHoldTime.SetNull();
+  mStartTime.SetNull();
+
+  UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);
+
+  if (mTimeline) {
+    mTimeline->RemoveAnimation(this);
+  }
+  MaybeQueueCancelEvent(activeTime);
+
   PostUpdate();
 }
 
 // https://drafts.csswg.org/web-animations/#finish-an-animation
 void Animation::Finish(ErrorResult& aRv) {
   double effectivePlaybackRate = CurrentOrPendingPlaybackRate();
 
   if (effectivePlaybackRate == 0 ||
@@ -763,44 +788,16 @@ void Animation::SilentlySetCurrentTime(c
     mStartTime =
         StartTimeFromTimelineTime(mTimeline->GetCurrentTimeAsDuration().Value(),
                                   aSeekTime, mPlaybackRate);
   }
 
   mPreviousCurrentTime.SetNull();
 }
 
-// https://drafts.csswg.org/web-animations/#cancel-an-animation
-void Animation::CancelNoUpdate() {
-  if (PlayState() != AnimationPlayState::Idle) {
-    ResetPendingTasks();
-
-    if (mFinished) {
-      mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
-    }
-    ResetFinishedPromise();
-
-    QueuePlaybackEvent(NS_LITERAL_STRING("cancel"),
-                       GetTimelineCurrentTimeAsTimeStamp());
-  }
-
-  StickyTimeDuration activeTime =
-      mEffect ? mEffect->GetComputedTiming().mActiveTime : StickyTimeDuration();
-
-  mHoldTime.SetNull();
-  mStartTime.SetNull();
-
-  UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);
-
-  if (mTimeline) {
-    mTimeline->RemoveAnimation(this);
-  }
-  MaybeQueueCancelEvent(activeTime);
-}
-
 bool Animation::ShouldBeSynchronizedWithMainThread(
     const nsCSSPropertyIDSet& aPropertySet, const nsIFrame* aFrame,
     AnimationPerformanceWarning::Type& aPerformanceWarning) const {
   // Only synchronize playing animations
   if (!IsPlaying()) {
     return false;
   }
 
--- a/dom/animation/Animation.h
+++ b/dom/animation/Animation.h
@@ -122,17 +122,16 @@ class Animation : public DOMEventTargetH
 
   virtual Promise* GetReady(ErrorResult& aRv);
   Promise* GetFinished(ErrorResult& aRv);
 
   IMPL_EVENT_HANDLER(finish);
   IMPL_EVENT_HANDLER(cancel);
 
   void Cancel();
-  virtual void CancelFromStyle() { CancelNoUpdate(); }
 
   void Finish(ErrorResult& aRv);
 
   virtual void Play(ErrorResult& aRv, LimitBehavior aLimitBehavior);
   virtual void PlayFromJS(ErrorResult& aRv) {
     Play(aRv, LimitBehavior::AutoRewind);
   }
 
--- a/layout/style/nsAnimationManager.h
+++ b/layout/style/nsAnimationManager.h
@@ -79,34 +79,34 @@ class CSSAnimation final : public Animat
   // following override), then we should update tabbrowser.xml to check
   // the playState instead.
   AnimationPlayState PlayStateFromJS() const override;
   bool PendingFromJS() const override;
   void PlayFromJS(ErrorResult& aRv) override;
 
   void PlayFromStyle();
   void PauseFromStyle();
-  void CancelFromStyle() override {
+  void CancelFromStyle() {
     // When an animation is disassociated with style it enters an odd state
     // where its composite order is undefined until it first transitions
     // out of the idle state.
     //
     // Even if the composite order isn't defined we don't want it to be random
     // in case we need to determine the order to dispatch events associated
     // with an animation in this state. To solve this we treat the animation as
     // if it had been added to the end of the global animation list so that
     // its sort order is defined. We'll update this index again once the
     // animation leaves the idle state.
     mAnimationIndex = sNextAnimationIndex++;
     mNeedsNewAnimationIndexWhenRun = true;
 
-    Animation::CancelFromStyle();
+    Animation::Cancel();
 
-    // We need to do this *after* calling CancelFromStyle() since
-    // CancelFromStyle might synchronously trigger a cancel event for which
+    // We need to do this *after* calling Cancel() since
+    // Cancel() might synchronously trigger a cancel event for which
     // we need an owning element to target the event at.
     mOwningElement = OwningElementRef();
   }
 
   void Tick() override;
   void QueueEvents(
       const StickyTimeDuration& aActiveTime = StickyTimeDuration());
 
--- a/layout/style/nsTransitionManager.h
+++ b/layout/style/nsTransitionManager.h
@@ -137,30 +137,30 @@ class CSSTransition final : public Anima
   // is expected to be called whilst already updating style.
   void PlayFromStyle() {
     ErrorResult rv;
     PlayNoUpdate(rv, Animation::LimitBehavior::Continue);
     // play() should not throw when LimitBehavior is Continue
     MOZ_ASSERT(!rv.Failed(), "Unexpected exception playing transition");
   }
 
-  void CancelFromStyle() override {
+  void CancelFromStyle() {
     // The animation index to use for compositing will be established when
     // this transition next transitions out of the idle state but we still
     // update it now so that the sort order of this transition remains
     // defined until that moment.
     //
     // See longer explanation in CSSAnimation::CancelFromStyle.
     mAnimationIndex = sNextAnimationIndex++;
     mNeedsNewAnimationIndexWhenRun = true;
 
-    Animation::CancelFromStyle();
+    Animation::Cancel();
 
-    // It is important we do this *after* calling CancelFromStyle().
-    // This is because CancelFromStyle() will end up posting a restyle and
+    // It is important we do this *after* calling Cancel().
+    // This is because Cancel() will end up posting a restyle and
     // that restyle should target the *transitions* level of the cascade.
     // However, once we clear the owning element, CascadeLevel() will begin
     // returning CascadeLevel::Animations.
     mOwningElement = OwningElementRef();
   }
 
   void SetEffectFromStyle(AnimationEffect* aEffect);