Closed Bug 1341518 Opened 7 years ago Closed 7 years ago

Drop the call of SetNeedStyleFlush in UpdateAnimations()

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file)

Now I am convinced this is not useful any more.

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

The next tick needed for animation events is ensured by DocumentTimeline instead of this.
If there were test cases that rely on this call, it's not related to events, should not rely on this, we should do RequestRestyle or something instead.
Blocks: 1340938
No longer blocks: 134093
The try looks good. That means we don't need the call at all, or we have no such test case there. But if there were such cases in the real world, we should fix the cases by other ways.
Sorry, this is going to take a few days before I get to this. I need to check what we should be doing here. Even if no test fails, we might be doing more work this way or simply getting lucky.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> The next tick needed for animation events is ensured by DocumentTimeline
> instead of this.
> If there were test cases that rely on this call, it's not related to events,
> should not rely on this, we should do RequestRestyle or something instead.

I don't see how this is related to whether a SetNeedStyleFlush call is needed.  SetNeedStyleFlush needs to be called whenever nsIDocument::FlushPendingNotifications needs to call into the pres shell to cause a style flush.  If the events need to be dispatched prior to a flush (rather than waiting until the next tick), as the comment suggests, I don't see why the call can be removed.
However, the comment seems to no longer be true following https://hg.mozilla.org/mozilla-central/rev/67d385efae3f5d2ff718fa04b37e2b0face67013 , so I suspect that this patch is OK, and has been since that one.
Wow, great.  I did not know that DispatchEvents() was called inside FlushPendingNotifications().
Comment on attachment 8839809 [details]
Bug 1341518 - We don't need to call SetNeedStyleFlush() explicitly in UpdateAnimations.

https://reviewboard.mozilla.org/r/114376/#review116196

Sorry, this cause is bug 1134163. I don't realize this implementation, but I should have investigated more.

This patch look good to me. Thank you so much.
Attachment #8839809 - Flags: review?(mantaroh) → review+
Comment on attachment 8839809 [details]
Bug 1341518 - We don't need to call SetNeedStyleFlush() explicitly in UpdateAnimations.

https://reviewboard.mozilla.org/r/114376/#review117018

(For my own reference, when reviewing this I was wondering, "How can we be sure we'll get another tick in order to dispatch any queued events?"

For cancel events we have special handling in Animation::CancelNoUpdate() introduced in bug 1302648 for this. For start and iteration events presumably we are still registered with the timeline because the animation has yet to finish (and hence Animation::NeedsTicks() returns true). For finished animations (i.e. animationend events), we queue and dispatch events in the same tick (and we get one tick after the animation finishes because special handling in DocumentTimeline::WillRefresh ensures we still call Tick() in that case even though NeedsTicks() returns false). We only needed to add special handling for the cancel case because otherwise we would remove ourselves from the timeline at the moment the animation was cancelled and never get a subsequent tick.)
Attachment #8839809 - Flags: review?(bbirtles) → review+
Thank you all involved!

(In reply to Brian Birtles (:birtles) from comment #8)
> Comment on attachment 8839809 [details]
> Bug 1341518 - We don't need to call SetNeedStyleFlush() explicitly in
> UpdateAnimations.
> 
> https://reviewboard.mozilla.org/r/114376/#review117018
> 
> (For my own reference, when reviewing this I was wondering, "How can we be
> sure we'll get another tick in order to dispatch any queued events?"
> 
> For cancel events we have special handling in Animation::CancelNoUpdate()
> introduced in bug 1302648 for this. For start and iteration events
> presumably we are still registered with the timeline because the animation
> has yet to finish (and hence Animation::NeedsTicks() returns true). For
> finished animations (i.e. animationend events), we queue and dispatch events
> in the same tick (and we get one tick after the animation finishes because
> special handling in DocumentTimeline::WillRefresh ensures we still call
> Tick() in that case even though NeedsTicks() returns false).

I make a unnecessary addition to this. In this case PostUpdate() is called for painting the finished result from the Tick(), I think.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/586854d5c3d4
We don't need to call SetNeedStyleFlush() explicitly in UpdateAnimations. r=birtles,mantaroh
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/586854d5c3d4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.