Closed Bug 1443427 Opened 6 years ago Closed 6 years ago

Do not flush throttled animations for Animation::FlushStyle()

Categories

(Core :: DOM: Animation, enhancement, P3)

Product:
Core
Component:
DOM: Animation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1443427 - Don't flush throttled animations in Animation::FlushStyle().
59 bytes, text/x-review-board-request
birtles
: review+
Details 
I tried to fix the case in Animation::FlushStyle() in bug 1442817, but it turned out that we need to fix bug 1443423 for this.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8957109 [details]
Bug 1443427 - Don't flush throttled animations in Animation::FlushStyle().

https://reviewboard.mozilla.org/r/225646/#review232218

::: commit-message-c36e4:3
(Diff revision 1)
> +Animation::FlushStyle() gets called only for CSS animations/transitions'
> +playState changes or ready Promise for CSS animations.  In either case
> +if this animation is throttled or this animation is affected by throttled
> +animations, we well end up flushing the throttled animations in sequential
> +tasks after normal styling process.

This doesn't seem right.

Like bug 1442817, isn't what we really want to say that we only want to flush style so that we have the up-to-date animation state and that throttled animations don't affect animation state so we don't need to flush them?

I mean, it has nothing to do with sequential tasks, right?

Or have I completely misunderstood this bug?
(In reply to Brian Birtles (:birtles) from comment #4)
> Comment on attachment 8957109 [details]
> Bug 1443427 - Don't flush throttled animations in Animation::FlushStyle().
> 
> https://reviewboard.mozilla.org/r/225646/#review232218
> 
> ::: commit-message-c36e4:3
> (Diff revision 1)
> > +Animation::FlushStyle() gets called only for CSS animations/transitions'
> > +playState changes or ready Promise for CSS animations.  In either case
> > +if this animation is throttled or this animation is affected by throttled
> > +animations, we well end up flushing the throttled animations in sequential
> > +tasks after normal styling process.
> 
> This doesn't seem right.
> 
> Like bug 1442817, isn't what we really want to say that we only want to
> flush style so that we have the up-to-date animation state and that
> throttled animations don't affect animation state so we don't need to flush
> them?
> 
> I mean, it has nothing to do with sequential tasks, right?

Oh right.  That's right.  I was confused about PlayFromStyle() and PauseFromStyle().  All of functions here are XXFromJS!
Comment on attachment 8957108 [details]
Bug 1443427 - Run test cases that use SVG element at the end of test_restyles.html.

https://reviewboard.mozilla.org/r/225644/#review232228

::: commit-message-be056:3
(Diff revision 1)
> +Those tests causes a problem that transitions are not able to be triggered
> +reliably by getAnimations() on old style system (bug 1443725) for some reasons.
> +So we have to move them at the end of the file to avoid affecting other test
> +cases.

The tests being moved don't appear to use transitions or getAnimations. Is it that these tests affect others that do?

This comment could be more clear.
Comment on attachment 8957109 [details]
Bug 1443427 - Don't flush throttled animations in Animation::FlushStyle().

https://reviewboard.mozilla.org/r/225646/#review232230
Attachment #8957109 - Flags: review?(bbirtles)
Comment on attachment 8957108 [details]
Bug 1443427 - Run test cases that use SVG element at the end of test_restyles.html.

https://reviewboard.mozilla.org/r/225644/#review232630
Attachment #8957108 - Flags: review?(bbirtles)
Hiro, were you planning on revisiting this?

It seems like the comments / commit messages just need to be fixed.
Flags: needinfo?(hikezoe)
Priority: -- → P3
Attachment #8957108 - Attachment is obsolete: true
The first patch, attachment 8957108 [details], has been obsoleted since we already dropped the old style system.
Flags: needinfo?(hikezoe)
Comment on attachment 8957109 [details]
Bug 1443427 - Don't flush throttled animations in Animation::FlushStyle().

https://reviewboard.mozilla.org/r/225646/#review241254

::: dom/animation/Animation.h:439
(Diff revision 2)
>    void UpdateEffect();
> +  /**
> +   * Flush all pending styles other than throttled animation styles (e.g.
> +   * animations running on the compositor).
> +   */
>    void FlushStyle() const;

Would it make sense to rename this to FlushUnanimatedStyle?

::: dom/animation/test/mozilla/file_restyles.html:1821
(Diff revision 2)
> +
> +      await animation.ready;
> +      ok(SpecialPowers.wrap(animation).isRunningOnCompositor);
> +
> +      var markers = observeAnimSyncStyling(() => {
> +        sibling.style.opcity = '0.5';

s/opcity/opacity/

::: dom/animation/test/mozilla/file_restyles.html:1871
(Diff revision 2)
> +
> +      await transition.ready;
> +      ok(SpecialPowers.wrap(transition).isRunningOnCompositor);
> +
> +      var markers = observeAnimSyncStyling(() => {
> +        sibling.style.opcity = '0.5';

s/opcity/opacity/
Attachment #8957109 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b714d7b30417
Don't flush throttled animations in Animation::FlushStyle(). r=birtles
https://hg.mozilla.org/mozilla-central/rev/b714d7b30417
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.