1207951 - When Animation::ComposeStyle updates the compose time, it should not call UpdateTiming

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This call seems astray:

  https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/dom/animation/Animation.cpp#647

It was added way back here:

  https://hg.mozilla.org/mozilla-central/rev/6df80592242c

in bug 1109390.

I can't understand, however, why we call UpdateSourceContent (now UpdateEffect) when we update the time, but UpdateTiming afterwards. Furthermore, it seems like we call UpdateTiming *before* restoring the hold time.

Basically, this code makes no sense and I can't find any explanation in the bug of why it is doing this way.

I think we should replace the call to UpdateTiming with one to UpdateEffect and move it outside the scoped block (i.e. after the dtor for the AutoRestore object has run).

Comment 1

[Brian Birtles (:birtles)]

Attachment: Fix buggy logic in ComposeStyle

Comment 2

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f69ebe1cb3c1

Comment 3

[Brian Birtles (:birtles)]

We should also get rid of the redundant call to PlayState()

Comment 4

[Brian Birtles (:birtles)]

Attachment: Fix buggy logic in ComposeStyle

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8665833 [details] [diff] [review]
Fix buggy logic in ComposeStyle

Review of attachment 8665833 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/Animation.cpp
@@ -620,5 @@
>    {
>      AutoRestore<Nullable<TimeDuration>> restoreHoldTime(mHoldTime);
> -    bool updatedHoldTime = false;
> -
> -    AnimationPlayState playState = PlayState();

Can we turn on -Wshadow for dom/animation/?

@@ +649,2 @@
>  
> +  mFinishedAtLastComposeStyle = (playState == AnimationPlayState::Finished);

Is there anything in the temporarily-change-hold-time block that could cause the play state to change, such that it would make sense to assert just before this line that playState == PlayState()?

Comment 6

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #5)
> ::: dom/animation/Animation.cpp
> @@ -620,5 @@
> >    {
> >      AutoRestore<Nullable<TimeDuration>> restoreHoldTime(mHoldTime);
> > -    bool updatedHoldTime = false;
> > -
> > -    AnimationPlayState playState = PlayState();
> 
> Can we turn on -Wshadow for dom/animation/?

I filed bug 1207951 for this.

> @@ +649,2 @@
> >  
> > +  mFinishedAtLastComposeStyle = (playState == AnimationPlayState::Finished);
> 
> Is there anything in the temporarily-change-hold-time block that could cause
> the play state to change, such that it would make sense to assert just
> before this line that playState == PlayState()?

I've added that assertion now.

Comment 7

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ca1f983de00

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/005e98e2c5c9

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/005e98e2c5c9