Closed Bug 1081007 Opened 10 years ago Closed 10 years ago

Fix the arrangement of calls between Play/PlayFromJS/PlayFromStyle in AnimationPlayer

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(1 file)

See bug 1070745 comment 63. As discussed with Daniel we want something like:

    AnimationPlayer:
      PlayFromJS() { DoPlay(); }
      Play()  { DoPlay(); }
      #DoPlay() { Actually play }
     
    CSSAnimationPlayer:
      OVERRIDE PlayFromJS { flush; call AnimationPlayer::PlayFromJS(); }
      OVERRIDE Play(): { Set sticky bit; call AnimationPlayer::Play(); }
      inherit DoPlay
      PlayFromStyle() { Sticky bit check etc,; DoPlay() so we don't set the sticky bit; } 

This is based on Daniel's pastebin but I've incorporated the changes from bug 1078122 since I assume it will land after that.
(In reply to Brian Birtles (:birtles) from comment #0)
> See bug 1070745 comment 63. As discussed with Daniel we want something like:
> 
>     AnimationPlayer:
>       PlayFromJS() { DoPlay(); }

I think that was supposed to be:

        PlayFromJS() { Play(); }
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.
Attachment #8505974 - Flags: review?(dholbert)
Comment on attachment 8505974 [details] [diff] [review]
Fix relationship between Play/PlayFromJS/PlayFromStyle etc

>+++ b/dom/animation/AnimationPlayer.cpp
[...]
>+void
> AnimationPlayer::FlushStyle() const
> {
>   nsIDocument* doc = GetRenderedDocument();
>   if (doc) {
>     doc->FlushPendingNotifications(Flush_Style);
>   }
> }
> 

Question: Does FlushStyle still need to live on AnimationPlayer, or can it move to CSSAnimationPlayer now? (I'm not entirely sure, but I'm wondering if the only remaining callers are all in CSSAnimationPlayer at this point.)

>diff --git a/dom/animation/AnimationPlayer.h b/dom/animation/AnimationPlayer.h
>   // AnimationPlayer methods
[...]
>+  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.
[...]
>+  virtual void PlayFromJS() { Play(); }
>+  void PauseFromJS() { Pause(); }

This comment isn't accurate anymore: "when called from script we perform extra steps".  As shown by the PlayFromJS/PauseFromJS implementations here, we perform no extra steps, at least not in this class.

Maybe replace "we perform" with "we (or our subclass) may perform"?

Also, in the cause of PauseFromJS, it looks like there really are no extra steps (not even in a subclass, because it's non-virtual). I think it's there just for symmetry, right?  Maybe add a comment to reflect that -- e.g.
  // Not strictly needed; just exists for symmetry with PlayFromJS():

r=me with comment tweaks along those lines (and with FlushStyle moved if possible/appropriate).  Thanks!
Attachment #8505974 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8505974 [details] [diff] [review]
...
> Question: Does FlushStyle still need to live on AnimationPlayer, or can it
> move to CSSAnimationPlayer now? (I'm not entirely sure, but I'm wondering if
> the only remaining callers are all in CSSAnimationPlayer at this point.)

I just realised that CSSTransitionPlayer also uses it so we need to leave it on AnimationPlayer.

> >diff --git a/dom/animation/AnimationPlayer.h b/dom/animation/AnimationPlayer.h
> >   // AnimationPlayer methods
> [...]
> >+  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.
> [...]
> >+  virtual void PlayFromJS() { Play(); }
> >+  void PauseFromJS() { Pause(); }
> 
> This comment isn't accurate anymore: "when called from script we perform
> extra steps".  As shown by the PlayFromJS/PauseFromJS implementations here,
> we perform no extra steps, at least not in this class.
> 
> Maybe replace "we perform" with "we (or our subclass) may perform"?

Fixed.

> Also, in the cause of PauseFromJS, it looks like there really are no extra
> steps (not even in a subclass, because it's non-virtual). I think it's there
> just for symmetry, right?  Maybe add a comment to reflect that -- e.g.
>   // Not strictly needed; just exists for symmetry with PlayFromJS():

Right, we should just remove PauseFromJS in WebIDL but we'll probably need to make 'pause()' flush in future so I'll just add the comment now (rather than getting a DOM peer to review this little change to the WebIDL only to then change it back again).
(Is this bug landable?)
Flags: needinfo?(birtles)
It's waiting on bug 1073336 which is taking longer than expected. I'm working on the last parts of it now.
Flags: needinfo?(birtles)
Cool! no rush, just stumbled across this in a tab that I'd left open, and wanted to make sure it hadn't been forgotten. :)

--> Adding explicit dependency on bug 1073336.
Depends on: 1073336
https://hg.mozilla.org/mozilla-central/rev/1be1a3bc5cb6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.