1439428 - Call UpdateCascadeResults directly in the case where the sequential task is CascadeResults

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

Calling RequestRestyle for the case where the sequential task is CascadeResults [1] is weird.  Normally RequestRestyle is a result of UpdateCascadeResults.  We should call UpdateCascadeResults() directly there.

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

[1] https://hg.mozilla.org/mozilla-central/file/3d23b4bd60d9/layout/style/ServoBindings.cpp#l673

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1439428 - Call UpdateCascadeResults() directly instead of RestyleRestyle() at the end of Gecko_UpdateAnimations().

Calling RequestRestyle() for update cascade results is weird since in general
RequestRestyle() is a result of updating cascade results (e.g. when an
!important style is changed).  In the case where we already know that we need
to update cascade results we can do it right after all the other processes that
may need to update cascade results has done so that we don't need to worry about
the cases additional cascade results update happens.

Review commit: https://reviewboard.mozilla.org/r/221476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221476/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

This is a prerequisite for bug 1439269, without this change bug 1439269 will be bit confusing since we will call RequestRestyle() for both CascadeResults and DisplayChangedFromNone.

Comment 3

[Brian Birtles (:birtles)]

Comment on attachment 8952226 [details]
Bug 1439428 - Call UpdateCascadeResults() directly instead of RestyleRestyle() at the end of Gecko_UpdateAnimations().

https://reviewboard.mozilla.org/r/221476/#review227788

::: commit-message-0158d:3
(Diff revision 1)
> +Calling RequestRestyle() for update cascade results is weird since in general
> +RequestRestyle() is a result of updating cascade results (e.g. when an
> +!important style is changed).  In the case where we already know that we need
> +to update cascade results we can do it right after all the other processes that
> +may need to update cascade results has done so that we don't need to worry about
> +the cases additional cascade results update happens.

Can you comment on why calling MaybeUpdateCascadeResults is not sufficient?

::: dom/animation/EffectCompositor.h:228
(Diff revision 1)
> +  // This can be expensive so we should only call it if styles that apply
> +  // above the animation level of the cascade might have changed. For all
> +  // other cases we should call MaybeUpdateCascadeResults.

Assuming this change is correct, we probably want to add a big NOTE: or something here. Part of the advantage of making this method private is only internal callers would need to worry about whether they should call MaybeUpdateCascadeResults or UpdateCascadeResults.

::: layout/style/ServoBindings.cpp:675
(Diff revision 1)
> -    // rules. We post a restyle here so that we can update the cascade
> -    // results in the pre-traversal of the next restyle.
> +    // CSS animations/transitions might have been destroyed above updating
> +    // processes, so we have to check there remains still animations here.

Nit: "CSS animations/transitions might have been destroyed as part of the above steps so before updating cascade results, we checking if there are still any animations to update."

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, travelling 20-26 Feb) from comment #3)
> Comment on attachment 8952226 [details]
> Bug 1439428 - Call UpdateCascadeResults() directly instead of
> RestyleRestyle() at the end of Gecko_UpdateAnimations().
> 
> https://reviewboard.mozilla.org/r/221476/#review227788
> 
> ::: commit-message-0158d:3
> (Diff revision 1)
> > +Calling RequestRestyle() for update cascade results is weird since in general
> > +RequestRestyle() is a result of updating cascade results (e.g. when an
> > +!important style is changed).  In the case where we already know that we need
> > +to update cascade results we can do it right after all the other processes that
> > +may need to update cascade results has done so that we don't need to worry about
> > +the cases additional cascade results update happens.
> 
> Can you comment on why calling MaybeUpdateCascadeResults is not sufficient?

I did actually try MaybeUpdateCascadeResults first[1], but it causes a failure in test_restyles.html.  I haven't checked why it failed.  Another reason is that if we use MaybeUpdateCascadeResults here we should call MarkCascadeNeedsUpdate() before it something like this;

  if (aTask & UpdateAnimationsTasks::CascadeResults) {
    MarkCascadeNeedsUpdate();
    MaybeUpdateCascadeResults();
  }

it's odd.  Currently we don't need MarkCascadeNeedsUpdate() since UpdateEffectProperties() already calls MarkCascadeNeedsUpdate() and CascadeResults always happens with UpdateAnimationsTasks::EffectProperties (for now), but I didn't want to depend it.

That's said, the test failure might have been fixed by other patches, I will re-try it.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce2ed65b725ae7da87a37293434531845e238f34

Comment 5

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> I did actually try MaybeUpdateCascadeResults first[1], but it causes a
> failure in test_restyles.html.  I haven't checked why it failed.  Another
> reason is that if we use MaybeUpdateCascadeResults here we should call
> MarkCascadeNeedsUpdate() before it something like this;
> 
>   if (aTask & UpdateAnimationsTasks::CascadeResults) {
>     MarkCascadeNeedsUpdate();
>     MaybeUpdateCascadeResults();
>   }

Yeah, we shouldn't do that.

I think what you're saying is, if we get the CascadeResults task that means we detected on the Servo side that the cascade has been updated but we couldn't call MarkCascadeNeedsUpdate because we were on the Servo side.

In that case we just need to highlight the point in the moved comment that says that we shouldn't normally call UpdateCascadeResults unless we know for sure we've detected a change in the cascade (e.g. just add a "NOTE:" before that paragraph).

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, travelling 20-26 Feb) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > I did actually try MaybeUpdateCascadeResults first[1], but it causes a
> > failure in test_restyles.html.  I haven't checked why it failed.  Another
> > reason is that if we use MaybeUpdateCascadeResults here we should call
> > MarkCascadeNeedsUpdate() before it something like this;
> > 
> >   if (aTask & UpdateAnimationsTasks::CascadeResults) {
> >     MarkCascadeNeedsUpdate();
> >     MaybeUpdateCascadeResults();
> >   }
> 
> Yeah, we shouldn't do that.
> 
> I think what you're saying is, if we get the CascadeResults task that means
> we detected on the Servo side that the cascade has been updated but we
> couldn't call MarkCascadeNeedsUpdate because we were on the Servo side.
> 
> In that case we just need to highlight the point in the moved comment that
> says that we shouldn't normally call UpdateCascadeResults unless we know for
> sure we've detected a change in the cascade (e.g. just add a "NOTE:" before
> that paragraph).

Thanks.  I will amend the comment.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1439428 - Make EffectCompositor::UpdateCascadeResults public.

We will call the function directly in a sequential task in the case where we
already detected that !important styles for animations has been changed in
parallel traversal.

Review commit: https://reviewboard.mozilla.org/r/222140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222140/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952226 [details]
Bug 1439428 - Call UpdateCascadeResults() directly instead of RestyleRestyle() at the end of Gecko_UpdateAnimations().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221476/diff/1-2/

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8952899 [details]
Bug 1439428 - Make EffectCompositor::UpdateCascadeResults public.

https://reviewboard.mozilla.org/r/222140/#review228190

::: dom/animation/EffectCompositor.h:228
(Diff revision 1)
> +  // NOTE: In Servo backend, we can detect cascade level changes during parallel
> +  // traversal, but we don't call MarkCascadeNeedsUpdate() during the traversal,
> +  // instead we call this function directly in a sequential task on the
> +  // main-thead.  This case is only one exception we don't use
> +  // MaybeUpdateCascadeResults().
> +  //
> +  // This can be expensive so we should only call it if styles that apply
> +  // above the animation level of the cascade might have changed. For all
> +  // other cases we should call MaybeUpdateCascadeResults.

Sorry, I wasn't clear. I didn't mean to add an extra paragraph, just the "NOTE:" word, that's all.

But since we're adding an extra paragraph let's replace these two paragraphs with:

"NOTE: This can be expensive so we should only call it if styles that apply above the animation level of the cascade might have changed. For all other cases we should call MaybeUpdateCascadeResults.

This is typically reserved for internal callers but is public here since when we detect changes to the cascade on the Servo side we can't call MarkCascadeNeedsUpdate during the traversal so instead we call this as part of a follow-up sequential task."

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8952226 [details]
Bug 1439428 - Call UpdateCascadeResults() directly instead of RestyleRestyle() at the end of Gecko_UpdateAnimations().

https://reviewboard.mozilla.org/r/221476/#review228192

::: layout/style/ServoBindings.cpp:676
(Diff revision 2)
>    }
>  
>    if (aTasks & UpdateAnimationsTasks::CascadeResults) {
> -    // This task will be scheduled if we detected any changes to !important
> -    // rules. We post a restyle here so that we can update the cascade
> -    // results in the pre-traversal of the next restyle.
> +    EffectSet* effectSet = EffectSet::GetEffectSet(aElement, pseudoType);
> +    // CSS animations/transitions might have been destroyed as part of the above
> +    // steps so before upting cascade results, we check if there are still any

s/upting/updating/

::: layout/style/ServoBindings.cpp:679
(Diff revision 2)
> +      // We don't use MaybeUpdateCascadeResults() here since it needs a preceded
> +      // call of MarkCascadeUpdated() but we don't do it in parallel traversal
> +      // to avoid mutate the state.

I'd probably just drop this comment or else write:

"We call UpdateCascadeResults directly (instead of MaybeUpdateCascadeResults) since we know for sure that the cascade has changed, but we were unable to call MarkCascadeUpdated when we noticed it since we avoid mutating state as part of the Servo parallel traversal."

Comment 11

[Brian Birtles (:birtles)]

Thanks for taking care of this!

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952899 [details]
Bug 1439428 - Make EffectCompositor::UpdateCascadeResults public.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222140/diff/1-2/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952226 [details]
Bug 1439428 - Call UpdateCascadeResults() directly instead of RestyleRestyle() at the end of Gecko_UpdateAnimations().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221476/diff/2-3/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

browser_webconsole_check_stubs_console_api.js failures on the try is bug 1432232 comment 6.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952899 [details]
Bug 1439428 - Make EffectCompositor::UpdateCascadeResults public.

https://reviewboard.mozilla.org/r/222140/#review228456

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952226 [details]
Bug 1439428 - Call UpdateCascadeResults() directly instead of RestyleRestyle() at the end of Gecko_UpdateAnimations().

https://reviewboard.mozilla.org/r/221476/#review228458

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea14b0cfc082
Make EffectCompositor::UpdateCascadeResults public. r=birtles
https://hg.mozilla.org/integration/autoland/rev/242aa23ce731
Call UpdateCascadeResults() directly instead of RestyleRestyle() at the end of Gecko_UpdateAnimations(). r=birtles

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ea14b0cfc082
https://hg.mozilla.org/mozilla-central/rev/242aa23ce731