Commit 7c837e6e authored by Kevin Ellis's avatar Kevin Ellis Committed by Commit Bot

Fix timing of when pending flags are reset.

Previously, the pending pause and play flags were reset in
Animation::Update. The flags should be reset when the animation
is ready (received start notification or stopped a running animation).
This change will affect when Animation::pending() changes state from
true to false. The next step is to reset these flags inside
of play and pause tasks per spec, which requires further cleanup of the
use of PlayStateUpdateScope.

The following patch introduced a subtle bug to the timing of notifications
to devtools > Animations, whereby the devtools notification could fire a
start notification before the start time was received.

https://chromium-review.googlesource.com/c/chromium/src/+/1822321

Fixing the timing of the pending task resets forces the devtools
notification to sync with the start of the animation, which in turn
undoes the faulty perf improvement seen in 1822321.


Bug: 960944
Change-Id: Icbcf546614570e1936bec3db94facb1b5bfc4e46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1873905Reviewed-by: default avatarMajid Valipour <majidvp@chromium.org>
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715417}
parent de207f4e
......@@ -300,6 +300,9 @@ void Animation::setCurrentTime(double new_current_time,
SetCompositorPending(/*effect_changed=*/false);
animation_play_state_ = CalculateAnimationPlayState();
internal_play_state_ = CalculatePlayState();
// Notify of potential state change.
NotifyProbe();
}
// https://drafts.csswg.org/web-animations/#setting-the-current-time-of-an-animation
......@@ -495,6 +498,8 @@ bool Animation::PreCommit(
if (!should_start) {
current_time_pending_ = false;
pending_pause_ = false;
pending_play_ = false;
}
if (should_start) {
......@@ -642,6 +647,9 @@ void Animation::NotifyStartTime(double ready_time) {
// TODO(crbug.com/960944): deprecate use of these flags.
internal_play_state_ = CalculatePlayState();
animation_play_state_ = CalculateAnimationPlayState();
// Notify of change from pending to running state.
NotifyProbe();
}
bool Animation::Affects(const Element& element,
......@@ -739,6 +747,7 @@ void Animation::SetStartTimeInternal(base::Optional<double> new_start_time) {
// Force a new, limited, current time.
hold_time_ = base::nullopt;
paused_ = false;
pending_pause_ = false;
double current_time = CalculateCurrentTime();
if (playback_rate_ > 0 && current_time > EffectEnd()) {
current_time = EffectEnd();
......@@ -933,6 +942,8 @@ void Animation::UnpauseInternal() {
if (!paused_)
return;
paused_ = false;
pending_pause_ = false;
pending_play_ = true;
SetCurrentTimeInternal(CurrentTimeInternal(), kTimingUpdateOnDemand);
}
......@@ -1028,7 +1039,6 @@ void Animation::PlayInternal(AutoRewind auto_rewind,
internal_play_state_ = kUnset;
SetOutdated();
SetCompositorPending(/*effect_changed=*/false);
NotifyProbe();
// 9. Run the procedure to update an animation’s finished state for animation
// with the did seek flag set to false, and the synchronously notify flag
......@@ -1039,6 +1049,9 @@ void Animation::PlayInternal(AutoRewind auto_rewind,
// TODO(crbug.com/960944): Deprecate.
animation_play_state_ = CalculateAnimationPlayState();
internal_play_state_ = CalculatePlayState();
// Notify change to pending play or finished state.
NotifyProbe();
}
// https://drafts.csswg.org/web-animations/#reversing-an-animation-section
......@@ -1115,7 +1128,11 @@ void Animation::finish(ExceptionState& exception_state) {
SetOutdated();
UpdateFinishedState(UpdateType::kDiscontinuous, NotificationType::kSync);
animation_play_state_ = internal_play_state_ = kFinished;
animation_play_state_ = kFinished;
internal_play_state_ = kFinished;
// Notify of change to finished state.
NotifyProbe();
}
void Animation::UpdateFinishedState(UpdateType update_type,
......@@ -1176,7 +1193,6 @@ void Animation::UpdateFinishedState(UpdateType update_type,
finished_promise_->Reset();
}
}
NotifyProbe();
}
void Animation::ScheduleAsyncFinish() {
......@@ -1234,6 +1250,8 @@ void Animation::CommitFinishNotification() {
}
void Animation::CommitAllUpdatesForTesting() {
pending_play_ = false;
pending_pause_ = false;
if (pending_finish_notification_)
CommitFinishNotification();
}
......@@ -1562,14 +1580,9 @@ bool Animation::Update(TimingUpdateReason reason) {
return false;
if (reason == kTimingUpdateForAnimationFrame) {
// Pending tasks are committed when the animation is 'ready'. This can be
// at the time of promise resolution or after a frame tick. Whereas the
// spec calls for creating scheduled tasks to commit pending changes, in the
// Blink implementation, this is an acknowledgment that the changes have
// taken affect.
// TODO(crbug.com/960944): Consider restructuring implementation to more
// closely align with the recommended algorithm in the spec.
ResetPendingTasks();
// TODO(crbug.com/960944): Align timing of committing the pending playback
// rate with the spec.
ApplyPendingPlaybackRate();
}
PlayStateUpdateScope update_scope(*this, reason, kDoNotSetCompositorPending);
......@@ -1714,11 +1727,13 @@ void Animation::cancel() {
// Apply changes synchronously.
SetCompositorPending(/*effect_changed=*/false);
NotifyProbe();
SetOutdated();
// Force dispatch of canceled event.
ForceServiceOnNextFrame();
// Notify of change to canceled state.
NotifyProbe();
}
void Animation::BeginUpdatingState() {
......@@ -1862,6 +1877,8 @@ Animation::PlayStateUpdateScope::~PlayStateUpdateScope() {
break;
}
animation_->EndUpdatingState();
// Play state may have changed.
animation_->NotifyProbe();
}
......@@ -1949,8 +1966,10 @@ void Animation::NotifyProbe() {
pending() ? kPending : CalculateAnimationPlayState();
if (old_play_state != new_play_state) {
probe::AnimationPlayStateChanged(document_, this, old_play_state,
new_play_state);
if (!pending()) {
probe::AnimationPlayStateChanged(document_, this, old_play_state,
new_play_state);
}
reported_play_state_ = new_play_state;
bool was_active = old_play_state == kPending || old_play_state == kRunning;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment