Remove AnimationPlayer::mIsPaused and represent pause state by a null start time

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
No description provided.
Assignee

Comment 1

5 years ago
This patch reworks AnimationPlayer to represent the paused state by a null start
time. This brings it into line with recent changes in the Web Animations spec
and removes the need for the mIsPaused member variable.

The idea is that in order for a player to play, it needs a start time and
an active timeline. The processing is roughly:
* If it is seeked to a particular time (through setting the currentTime or
  calling pause() or finish()) but has no start time, it is paused.
* Otherwise, if it has no active timeline or no start time, it is idle.

By removing the mIsPaused flag the number of possible permutations of states is
reduced so the model is easier to reason about (see:
http://lists.w3.org/Archives/Public/public-fx/2014OctDec/0026.html).

This patch replaces the mIsPaused flag with checks for mStartTime.IsNull()
according to the rules outlined above.
Attachment #8528112 - Flags: review?(dzbarsky)
Assignee

Updated

5 years ago
Summary: Remove AnimationPlayer::mIsPaused and represent pause state but a null start time → Remove AnimationPlayer::mIsPaused and represent pause state by a null start time
Assignee

Updated

5 years ago
Blocks: 1104427
Assignee

Updated

5 years ago
Attachment #8528112 - Flags: review?(dzbarsky) → review?(dholbert)
Comment on attachment 8528112 [details] [diff] [review]
Rework pausing in AnimationPlayer to remove mIsPaused

>+++ b/dom/animation/AnimationPlayer.cpp
> AnimationPlayer::DoPlay()
> {
[...]
>+  Nullable<TimeDuration> currentTime = GetCurrentTime();
>+  if (currentTime.IsNull()) {
>+    mHoldTime.SetValue(TimeDuration(0));
>+  }
>+
>+  // If the hold time is null, we are already playing normally
>+  if (mHoldTime.IsNull()) {
>     return;

I think this should be "else if" -- because if we enter the first branch, we know for a fact that mHoldTime is *not* null (it's TimeDuration(0)), so there's no point in checking if mHoldTime.IsNull().

(It's possible there might be some additional logic-cleanup we could do here, too, since GetCurrentTime() may internally invoke mTimeline->GetCurrentTime(), and then we also invoke that directly, lower down here in DoPlay. If we can consolidate that (maybe by expanding this GetCurrentTime() call), that might be nice; maybe that doesn't belong here though.)

> void
> AnimationPlayer::DoPause()
> {
>-  if (mIsPaused) {
>+  if (IsPaused()) {
>     return;
>   }
>-  mIsPaused = true;
[...]
>   // Bug 927349 - check for null result here and go to pending state
>   mHoldTime = GetCurrentTime();

(Per IRL discussion, the IsPaused()-and-then-GetCurrentTime() seems a bit redundant here, since IsPaused() *calls* GetCurrentTime().  Maybe this could really just be a mStartTime.IsNull() check? Anyway, since this early-return is eventually going away later in your patch-queue, I guess this distinction doesn't really matter much.)

r=me, with the "else if" added (& maybe a followup filed for GetCurrentTime() / mTimeline->GetCurrentTime() redundancy-cleanup/consolidation, as you see fit)
Attachment #8528112 - Flags: review?(dholbert) → review+
Assignee

Updated

5 years ago
Blocks: 1107351
https://hg.mozilla.org/mozilla-central/rev/98822efd91ae
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.