Bug 1865637 - Make sure animation ticks are consistent. r=hiro

This makes ticking the document timeline happens only when we make
forward progress. It seems otherwise we can end up with an extra no-op
tick which might resolve the ready time unexpectedly.

It's also cleaner.

Differential Revision: https://phabricator.services.mozilla.com/D194406

dom/animation/DocumentTimeline.cpp

@@ -47,11 +47,14 @@ DocumentTimeline::DocumentTimeline(Document* aDocument,
       mDocument(aDocument),
       mIsObservingRefreshDriver(false),
       mOriginTime(aOriginTime) {
-  if (mDocument) {
-    mDocument->Timelines().insertBack(this);
-  }
+  mDocument->Timelines().insertBack(this);
   // Ensure mLastRefreshDriverTime is valid.
-  UpdateLastRefreshDriverTime();
+  if (nsDOMNavigationTiming* timing = mDocument->GetNavigationTiming()) {
+    mLastRefreshDriverTime = timing->GetNavigationStartTimeStamp();
+  }
+  if (nsRefreshDriver* rd = GetRefreshDriver()) {
+    MaybeUpdateLastRefreshDriverTime(rd->MostRecentRefresh());
+  }
 }
 
 DocumentTimeline::~DocumentTimeline() {
@@ -100,41 +103,28 @@ bool DocumentTimeline::TracksWallclockTime() const {
 }
 
 TimeStamp DocumentTimeline::GetCurrentTimeStamp() const {
-  nsRefreshDriver* refreshDriver = GetRefreshDriver();
-  return refreshDriver ? refreshDriver->MostRecentRefresh()
-                       : mLastRefreshDriverTime;
+  if (nsRefreshDriver* refreshDriver = GetRefreshDriver()) {
+    auto ts = refreshDriver->MostRecentRefresh();
+    if (ts > mLastRefreshDriverTime) {
+      return ts;
+    }
+  }
+  return mLastRefreshDriverTime;
 }
 
-void DocumentTimeline::UpdateLastRefreshDriverTime(TimeStamp aKnownTime) {
-  TimeStamp result = [&] {
-    if (!aKnownTime.IsNull()) {
-      return aKnownTime;
-    }
-    if (auto* rd = GetRefreshDriver()) {
-      return rd->MostRecentRefresh();
-    };
-    return mLastRefreshDriverTime;
-  }();
-
-  if (nsDOMNavigationTiming* timing = mDocument->GetNavigationTiming()) {
-    // If we don't have a refresh driver and we've never had one use the
-    // timeline's zero time.
-    // In addition, it's possible that our refresh driver's timestamp is behind
-    // from the navigation start time because the refresh driver timestamp is
-    // sent through an IPC call whereas the navigation time is set by calling
-    // TimeStamp::Now() directly. In such cases we also use the timeline's zero
-    // time.
-    // Also, let this time represent the current refresh time. This way we'll
-    // save it as the last refresh time and skip looking up navigation start
-    // time each time.
-    if (result.IsNull() || result < timing->GetNavigationStartTimeStamp()) {
-      result = timing->GetNavigationStartTimeStamp();
-    }
+bool DocumentTimeline::MaybeUpdateLastRefreshDriverTime(TimeStamp aTime) {
+  // If we don't have a refresh driver and we've never had one use the
+  // timeline's zero time.
+  // It's possible that our refresh driver's timestamp is behind from the
+  // navigation start time because the refresh driver timestamp is sent
+  // through an IPC call whereas the navigation time is set by calling
+  // TimeStamp::Now() directly. Make sure we only advance.
+  if (aTime < mLastRefreshDriverTime) {
+    return false;
   }
 
-  if (!result.IsNull()) {
-    mLastRefreshDriverTime = result;
-  }
+  mLastRefreshDriverTime = aTime;
+  return true;
 }
 
 Nullable<TimeDuration> DocumentTimeline::ToTimelineTime(
@@ -169,10 +159,34 @@ void DocumentTimeline::NotifyAnimationUpdated(Animation& aAnimation) {
   }
 }
 
-void DocumentTimeline::MostRecentRefreshTimeUpdated() {
+void DocumentTimeline::TriggerAllPendingAnimationsNow() {
+  for (Animation* animation : mAnimationOrder) {
+    animation->TryTriggerNow();
+  }
+}
+
+void DocumentTimeline::WillRefresh(TimeStamp aTime) { MaybeTick(aTime); }
+
+void DocumentTimeline::NotifyTimerAdjusted(TimeStamp aTime) {
+  MaybeTick(aTime);
+}
+
+void DocumentTimeline::MaybeTick(TimeStamp aTime) {
+  if (NS_WARN_IF(!MaybeUpdateLastRefreshDriverTime(aTime))) {
+    // Note that with test-only code the time might go backwards (when restoring
+    // normal refresh after test refresh or vice versa).
+    // FIXME(emilio): Seems like we should not allow that.
+    auto* rd = GetRefreshDriver();
+    if (rd && rd->IsTestControllingRefreshesEnabled()) {
+      mLastRefreshDriverTime = aTime;
+    } else {
+      return;
+    }
+  }
+
   MOZ_ASSERT(mIsObservingRefreshDriver);
   MOZ_ASSERT(GetRefreshDriver(),
-             "Should be able to reach refresh driver from within WillRefresh");
+             "Should be able to reach refresh driver from within the tick");
 
   nsAutoAnimationMutationBatch mb(mDocument);
 
@@ -200,21 +214,6 @@ void DocumentTimeline::MostRecentRefreshTimeUpdated() {
   }
 }
 
-void DocumentTimeline::TriggerAllPendingAnimationsNow() {
-  for (Animation* animation : mAnimationOrder) {
-    animation->TryTriggerNow();
-  }
-}
-
-void DocumentTimeline::WillRefresh(TimeStamp aTime) {
-  UpdateLastRefreshDriverTime();
-  MostRecentRefreshTimeUpdated();
-}
-
-void DocumentTimeline::NotifyTimerAdjusted(TimeStamp aTime) {
-  MostRecentRefreshTimeUpdated();
-}
-
 void DocumentTimeline::ObserveRefreshDriver(nsRefreshDriver* aDriver) {
   MOZ_ASSERT(!mIsObservingRefreshDriver);
   // Set the mIsObservingRefreshDriver flag before calling AddRefreshObserver
@@ -241,7 +240,7 @@ void DocumentTimeline::NotifyRefreshDriverCreated(nsRefreshDriver* aDriver) {
     // could perform a paint before the first refresh driver tick happens.  To
     // ensure we're in a consistent state in that case we run the first tick
     // manually.
-    MostRecentRefreshTimeUpdated();
+    MaybeTick(aDriver->MostRecentRefresh());
   }
 }
 

dom/animation/DocumentTimeline.h

@@ -71,7 +71,8 @@ class DocumentTimeline final : public AnimationTimeline,
 
   Document* GetDocument() const override { return mDocument; }
 
-  void UpdateLastRefreshDriverTime(TimeStamp aKnownTime = {});
+  bool MaybeUpdateLastRefreshDriverTime(TimeStamp);
+  void MaybeTick(TimeStamp);
 
   bool IsMonotonicallyIncreasing() const override { return true; }
 

dom/animation/test/document-timeline/test_document-timeline.html

@@ -128,8 +128,9 @@ async_test(function(t) {
       getComputedStyle(hiddenIFrame).display;
 
       window.requestAnimationFrame(t.step_func(function() {
-        assert_true(
-          hiddenIFrame.contentDocument.timeline.currentTime >= previousValue,
+        assert_greater_than_equal(
+          hiddenIFrame.contentDocument.timeline.currentTime,
+          previousValue,
           'document.timeline.currentTime does not go backwards after'
           + ' re-setting display:none');
         t.done();

dom/base/Document.cpp

@@ -13621,8 +13621,9 @@ void Document::SetNavigationTiming(nsDOMNavigationTiming* aTiming) {
   // If there's already the DocumentTimeline instance, tell it since the
   // DocumentTimeline is based on both the navigation start time stamp and the
   // refresh driver timestamp.
-  if (mDocumentTimeline) {
-    mDocumentTimeline->UpdateLastRefreshDriverTime();
+  if (mDocumentTimeline && mTiming) {
+    mDocumentTimeline->MaybeUpdateLastRefreshDriverTime(
+        mTiming->GetNavigationStartTimeStamp());
   }
 }
 

layout/style/test/animation_utils.js

@@ -601,8 +601,9 @@ const ExpectComparisonTo = {
         break;
 
       case RunningOn.TodoMainThread:
-        todo(
-          compositorStr === "",
+        todo_is(
+          compositorStr,
+          "",
           desc + ": should NOT be animating on compositor"
         );
         actualStr = compositorStr === "" ? computedStr : compositorStr;