1388566 - stylo: Don't flush throttled animations if the root element has normal dirty descendants bit in PreTraverse()

Status: ASSIGNED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Currently we flush throttled animations if we have normal dirty descendants bit in PreTraverse() [1][2], but it's apparently wrong. Those cases should be handled in SequentialTask for CascadeResults.

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

[1] https://hg.mozilla.org/mozilla-central/file/1d042bcb2632/dom/animation/EffectCompositor.cpp#l984
[2] https://hg.mozilla.org/mozilla-central/file/1d042bcb2632/dom/animation/EffectCompositor.cpp#l1137

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Why is it wrong, could you elaborate?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

To avoid confusion, 'flush throttled animations' in comment 0 does not mean 'flushThrotteledAnimations' for event handling. 

From [1] in comment 1.

  bool flushThrottledRestyles =
    (aRoot && aRoot->HasDirtyDescendantsForServo()) ||
    aRestyleType == AnimationRestyleType::Full;

'AnimationRestyleType::Full' means the 'flushThrottledAnimations' for event handling (honestely I want to rename it to flushTransformAnimations). So we are trying to flush all animations' (including animations running on the compositor) style if the root element has dirty descendants bit.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I did give up writing a test case for this. The only code patch what we can test this bug's case is StyleNewSubtree() in nsCSSFrameConstructor::CreateGeneratedContentItem(). Other call sites do clear servo data before calling StyleNewSubtree() or for an anonymous content, so it can not be used in automation tests.  As for CreateGeneratedContentItem() case, we don't properly update content property (bug 1398653).

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388566 - We don't need to check dirty descendant bit whether we need to flush throttled animations in PreTraverseInSubtree().

When styles which affect animations on running on the compositor,
we create a UpdateAnimationsTasks::CascadeResults and ends up calling
RequestRestyle, so we don't need to care about the case in
PreTraverseInSubtree().

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

This patch if a patch split from bug 1379534.  I will mark 1379534 as invalid.

(In reply to Brian Birtles (:birtles, away until 19 Sep) from comment #5)
> Comment on attachment 8884712 [details]
> Bug 1379534 - We don't need to check dirty descendant bit whether we need to
> flush styles in PreTraverseInSubtree().
> 
> https://reviewboard.mozilla.org/r/155582/#review160624
> 
> ::: commit-message-b594f:3
> (Diff revision 1)
> > +When style attribute is changed for an element which has animations running on
> > +the compositor, we create a SequentialTask for UpdateEffectProperties() and
> > +UpdateEffectProperties() ends up calling RequestRestyle for layer, so we don't
> > +need to care about the case in PreTraverseInSubtree().
> 
> Is this code path only called when the style attribute is changed? If so, we
> should say so. If not, I don't really understand why that is the only case
> that matters here.

Fixed.

> ::: dom/animation/EffectCompositor.cpp:965
> (Diff revision 1)
> > -  // We need to force flush all throttled animations if we also have
> > -  // non-animation restyles (since we'll want the up-to-date animation style
> > +  // We need to force flush all throttled animations if we are currently
> > +  // flushing all throttled animation restyles for event handling.
> 
> This comment doesn't make sense any more. And why is it ok to drop the
> mention of transitions? Is this no longer needed for transitions?

We still need to flush throttled animations for hit-testing.  Also, I don't actually understand the original comment for transitions, why we need to flush throttled animations if the target element has normal dirty bit to trigger transitions.  The only case I can imagine is there is a throttled font-size animation due to step functions and a with animation specified 'em'.  Even this case, I think the width transition is triggered with the correct before change style.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> I did give up writing a test case for this. The only code patch what we can
> test this bug's case is StyleNewSubtree() in
> nsCSSFrameConstructor::CreateGeneratedContentItem().

The case in CreateGeneratedContentItem(), the element that PreTraverseInSubtree takes is a newly created a pseudo element, so the element does not have the dirty bit. That means we don't need to check the dirty bit in PreTraverseInSubtree at all.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388566 - Drop unused Element argument from ServoStyleSet::PreTraverse().

It has not been used since bug 1388031.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8906835 [details]
Bug 1388566 - We don't need to check dirty descendant bit whether we need to flush throttled animations in PreTraverseInSubtree().

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

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8906835 [details]
Bug 1388566 - We don't need to check dirty descendant bit whether we need to flush throttled animations in PreTraverseInSubtree().

https://reviewboard.mozilla.org/r/178562/#review191296

::: commit-message-9880d:3
(Diff revision 2)
> +When styles which affect animations on running on the compositor,
> +we create a UpdateAnimationsTasks::CascadeResults and ends up calling
> +RequestRestyle, so we don't need to care about the case in

Where does that happen? I only see that happening when important_rules_changed is true.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8906835 [details]
> Bug 1388566 - We don't need to check dirty descendant bit whether we need to
> flush throttled animations in PreTraverseInSubtree().
> 
> https://reviewboard.mozilla.org/r/178562/#review191296
> 
> ::: commit-message-9880d:3
> (Diff revision 2)
> > +When styles which affect animations on running on the compositor,
> > +we create a UpdateAnimationsTasks::CascadeResults and ends up calling
> > +RequestRestyle, so we don't need to care about the case in
> 
> Where does that happen? I only see that happening when
> important_rules_changed is true.

OK, the commit message is somewhat misleading.  It just explains the single case (the important rules change). When animation properties changed for CSS animations running on the compositor, we do update them in a sequential task for CSSAnimations (and it leads to request restyle layer). Likewise for CSS transitions.  As far as I can tell, that's all.

Anyway, an important thing is that PreTraverseInSubtree() is called from ServoStyleSet::StyleNewSubtree() and when we call StyleNewSubtree(), the servo style data is cleared or StyleNewSubtree is called for a newly created element, so the check for the dirty bit is not useful at all there.
So even if there are some cases that I've been missing, the dirty bit check is useless.
Should I revise the commit message as this?

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yes please. Please explain in the commit message why we can be sure that the transition style of throttled animations will be up-to-date at the point when we generate new transitions.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8915014 [details]
Bug 1388566 - Drop unused Element argument from ServoStyleSet::PreTraverse().

https://reviewboard.mozilla.org/r/186280/#review191308

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #11)
> Yes please. Please explain in the commit message why we can be sure that the
> transition style of throttled animations will be up-to-date at the point
> when we generate new transitions.

To be clear, I am going to explain that the dirty bit checks is not worth doing there. Honestly the comment for transition doesn't make sense to me, since if we really need the dirty bit check there, we have to check all of animation elements there instead of the root one, also the check is done only if the root is not null, so if the PreTraverse() is called from ServoStyleSet::StyleDocument (it's normal restyling in a tick), we don't check the dirty bit.

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Even if the original code is buggy, I want to understand how this is supposed to work (or why it happens to work) before we go and touch this code. We've had plenty of regressions in this area and I'd like to avoid more!

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8906835 [details]
Bug 1388566 - We don't need to check dirty descendant bit whether we need to flush throttled animations in PreTraverseInSubtree().

OK, I will ask Boris about what the comment supposed once he comes back to work, I don't really understand about the comment.

Comment 16

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57