Closed Bug 1442861 Opened 6 years ago Closed 6 years ago

Clear mNeedStyleFlush flag after ProcessPendingRestyles() in PresShell::DoFlushPendingNotifications()

Categories

(Core :: DOM: Animation, enhancement)

Product:
Core
Component:
DOM: Animation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file)

Bug 1442861 - Clear mNeedStyleFlush flag after ProcessPendingRestyles().
59 bytes, text/x-review-board-request
emilio
: review+
Details 
We clear mNeedStyleFlush at the top of PresShell::DoFlushPendingNotifications(), but we might set the flag again to update animations in sequential task or to flush throttled animations.

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

I don't know the reason the flag is originally cleared at the top of DoFlushPendingNotifications() instead of after ProcessPendingRestyles(), but I guess there is a reason for that, so I did put another "mNeedStyleFlush = false" in the above try instead of moving original one.
I thought the failure reason is that the transform animation in the test case wasn't sent to the compositor.  But actually it was.  It means that there is/are (a) case(s) nsIPresShell::NeedFlush returns false even if there is an animation on the compositor.
I guess the reason is that normal styling happened in the failure case thus mNeedThrottledAnimationFlush was cleared.  

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

In the above try, I used requestIdleCallback instead of requestAnimationFrame and used opacity animation instead of transform to avoid unthrottling it periodically.
Oh no.  I did skip test_getUnanimatedComputedStyle in the try, so it's the culprit.
Apart from this bug, test_domwindowutils.html which is the test file that I added a new test case here should be re-written with async/await.  I've filed bug 1443029 for that.
Hmm still failed on WebRender.  To make the test pass on all platforms is bit hard.

Given that the failure happens at the place we check DOMWindowUtils.needsFlush() returns true if there is a throtteled animation before we flush styles/layout [1].  So it doesn't a big problem for this fix.  This bug fixes the case that DOMWindowUtils.needsFlush() returns true *after* flushing styles/layout.  So I will drop the check here.  I think we need bug 1419226 or bug 1437036 or bug 1419851 to make the check before flusing layout work fine.

A new try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1848136fde2b70b3276ecc94b149842730319af

[1] https://hg.mozilla.org/try/rev/de27f3fc6f20e1744b46ec07030fda4a9c760df2#l1.61
Keywords: leave-open
Comment on attachment 8955963 [details]
Bug 1442861 - Clear mNeedStyleFlush flag after ProcessPendingRestyles().

https://reviewboard.mozilla.org/r/224912/#review230888

Please fix that, then r=me

::: dom/base/test/file_domwindowutils_animation.html:202
(Diff revision 1)
> +      ok(SpecialPowers.wrap(animation).isRunningOnCompositor,
> +         "Opacity animation should run on the compositor");
> +
> +      // FIXME! Bug 1442861: We should make sure needsFlush() returns true
> +      // before flusing layout.
> +      //ok(utils.needsFlush(SpecialPowers.Ci.nsIDOMWindowUtils.FLUSH_STYLE),

todo_is(utils.needsFlush(..), true)?

::: layout/base/PresShell.cpp:4244
(Diff revision 1)
>  
>      didStyleFlush = true;
> +    // We have to clear mNeedStyleFlush here again since we might have set the
> +    // flag during ProcessPendingRestyles (to update animations in sequential
> +    // task) or during flushing throttled animations.
> +    mNeedStyleFlush = false;

So I don't think it is sound to clear it here, given the `~nsAutoScriptBlocker` can run script (for XBL stuff, maybe others) and request more restyles. I think it should be cleared under the scope in which we call ProcessPendingRestyles, that is, right after the call.
Attachment #8955963 - Flags: review?(emilio) → review+
Comment on attachment 8955963 [details]
Bug 1442861 - Clear mNeedStyleFlush flag after ProcessPendingRestyles().

https://reviewboard.mozilla.org/r/224912/#review230892

::: layout/base/PresShell.cpp:4244
(Diff revision 1)
>  
>      didStyleFlush = true;
> +    // We have to clear mNeedStyleFlush here again since we might have set the
> +    // flag during ProcessPendingRestyles (to update animations in sequential
> +    // task) or during flushing throttled animations.
> +    mNeedStyleFlush = false;

Also please mention in the comment that this is not a correctness issue in any way. if only it'd be an optimization.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Comment on attachment 8955963 [details]
> Bug 1442861 - Clear mNeedStyleFlush flag after ProcessPendingRestyles().
> 
> https://reviewboard.mozilla.org/r/224912/#review230888
> 
> Please fix that, then r=me
> 
> ::: dom/base/test/file_domwindowutils_animation.html:202
> (Diff revision 1)
> > +      ok(SpecialPowers.wrap(animation).isRunningOnCompositor,
> > +         "Opacity animation should run on the compositor");
> > +
> > +      // FIXME! Bug 1442861: We should make sure needsFlush() returns true
> > +      // before flusing layout.
> > +      //ok(utils.needsFlush(SpecialPowers.Ci.nsIDOMWindowUtils.FLUSH_STYLE),
> 
> todo_is(utils.needsFlush(..), true)?

Unfortunately we can't use todo_is it since the value is sometime true, sometimes isn't.

> ::: layout/base/PresShell.cpp:4244
> (Diff revision 1)
> >  
> >      didStyleFlush = true;
> > +    // We have to clear mNeedStyleFlush here again since we might have set the
> > +    // flag during ProcessPendingRestyles (to update animations in sequential
> > +    // task) or during flushing throttled animations.
> > +    mNeedStyleFlush = false;
> 
> So I don't think it is sound to clear it here, given the
> `~nsAutoScriptBlocker` can run script (for XBL stuff, maybe others) and
> request more restyles. I think it should be cleared under the scope in which
> we call ProcessPendingRestyles, that is, right after the call.

Oh right. Indeed.  I will revise the comment.  Thanks!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8e9cae3c0b9
Clear mNeedStyleFlush flag after ProcessPendingRestyles(). r=emilio
Why did I set leave-open here?
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: