1338087 - stylo: Stop storing StyleAnimationValue on main thread

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Boris Chiou [:boris]]

After landing Bug 1337313 and Bug 1335942, it's time to stop storing StyleAnimationValue for stylo animations, which means we should always use RawServoAnimationValue for animations.

However, ComputeDistance and Accumulation/Addition still need StyleAnimationValue. The former is used by spacing, which can be disabled for stylo, and the latter is not supported yet (Bug 1329878 will fix it!).

Note: This may cause some mochitests failed. (But we will know what is need to be fixed ASAP.)

Comment 1

[Brian Birtles (:birtles)]

Sorry, I haven't been following this closely but what are we doing for compositor animations? Don't we need StyleAnimationValue objects there still?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

No at least for main thread side, once bug 1337313 landed.  To run animations on the compositor, we still need to update cascading result.  Apart from the cascading issue for the animations on the compositor we can run animations on the compositor now!
On the compositor we still use StyleAnimationValue for interpolation but it's generated from Animatable object on the compositor. Boris did the job!

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1338087 - Part 1: Don't apply paced spacing on stylo.

To avoid using StyleAnimationValue on stylo, we should skip
ApplySpacing, which means We always fall back to distribute spacing on stylo.

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

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1338087 - Part 2: Drop the computation of StyleAnimationValue on stylo.

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

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1338087 - Part 3: Append PropertyStyleAnimationValuePair from Servo side.

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

Comment 6

[Boris Chiou [:boris]]

Actually, I would like to precalculate the Context outside the loop, instead of in Servo_AnimationValues_Populate(), but I think hiro will do that in bug 1338927.

Comment 7

[Boris Chiou [:boris]]

Comment on attachment 8836562 [details]
Bug 1338087 - Part 3: Append PropertyStyleAnimationValuePair from Servo side.

https://reviewboard.mozilla.org/r/111880/#review113188

::: servo/ports/geckolib/glue.rs:301
(Diff revision 1)
>      let mut iter = guard.declarations
>                      .iter()
>                      .filter_map(|&(ref decl, imp)| {
>                          if imp == Importance::Normal {
> -                            AnimationValue::from_declaration(decl, &context, &init)
> +                            match AnimationValue::from_declaration(decl, &context, &init) {
> +                                Some(v) => Some((decl.id().to_nscsspropertyid(), v)),

I just noticed that we can use TransitionProperty to convert it into a nsCSSPropertyID. However, I need others' opinions.

Comment 8

[Boris Chiou [:boris]]

Comment on attachment 8836561 [details]
Bug 1338087 - Part 2: Drop the computation of StyleAnimationValue on stylo.

https://reviewboard.mozilla.org/r/111878/#review113190

::: layout/style/StyleAnimationValue.cpp
(Diff revision 1)
> -  // FIXME: Servo bindings don't yet represent const-ness so we just
> -  // cast it away for now.
> -  auto declarations = const_cast<RawServoDeclarationBlock*>(&aDeclarations);
> -  RefPtr<ServoComputedValues> computedValues =
> -    aStyleContext->PresContext()->StyleSet()->AsServo()->
> -      RestyleWithAddedDeclaration(declarations, previousStyle).Consume();

Is it possible to use this in the future? if not, I can also remove ServoStyleSet::RestyleWithAddedDeclaration(...) and its FFI.

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8836562 [details]
Bug 1338087 - Part 3: Append PropertyStyleAnimationValuePair from Servo side.

https://reviewboard.mozilla.org/r/111880/#review113192

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8836560 [details]
Bug 1338087 - Part 1: Don't apply paced spacing on stylo.

https://reviewboard.mozilla.org/r/111876/#review113194

::: dom/animation/KeyframeEffectReadOnly.cpp:902
(Diff revision 1)
>                                               aStyleContext);
>  
> -  if (mEffectOptions.mSpacingMode == SpacingMode::paced) {
> +  // FIXME: Bug 1332633: we have to implement ComputeDistance for
> +  //        RawServoAnimationValue.
> +  if (mEffectOptions.mSpacingMode == SpacingMode::paced &&
> +      aStyleContext->PresContext()->StyleSet()->IsGecko()) {

I am wondering we should unify this kind of check in a later bug.  In other parts we are currently using mDocument->IsStyledByServo().  But that's OK for now.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8836561 [details]
Bug 1338087 - Part 2: Drop the computation of StyleAnimationValue on stylo.

https://reviewboard.mozilla.org/r/111878/#review113198

I think we can also drop the forward declaration of RawServoDeclarationBlock in StyleAnimationValue.h.  Also ServoBindings.h?  I am not really sure.

::: dom/animation/KeyframeUtils.cpp:638
(Diff revision 1)
> +                                               CSSEnabledState::eForAllContent) {
> +            if (nsCSSProps::kAnimTypeTable[*p] == eStyleAnimType_None) {
> +              // Skip non-animatable component longhands.
> -          continue;
> +              continue;
> -        }
> +            }
> +            PropertyStyleAnimationValuePair* valuePair = values.AppendElement();
> +            valuePair->mProperty = *p;
> +          }
> +        } else {
> +          PropertyStyleAnimationValuePair* valuePair = values.AppendElement();
> +          valuePair->mProperty = pair.mProperty;
> +        }
> +

I haven't checked the later patch yet, this looks correct to me.

::: layout/style/StyleAnimationValue.cpp
(Diff revision 1)
> -  // FIXME: Servo bindings don't yet represent const-ness so we just
> -  // cast it away for now.
> -  auto declarations = const_cast<RawServoDeclarationBlock*>(&aDeclarations);
> -  RefPtr<ServoComputedValues> computedValues =
> -    aStyleContext->PresContext()->StyleSet()->AsServo()->
> -      RestyleWithAddedDeclaration(declarations, previousStyle).Consume();

I don't think the function will be used in the future. Let's drop this here.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Boris, is there any problems without part 3?  If there is no problem, I'd like you to land these patches without part 3 here, and drop Servo_AnimationValues_Populate in bug 1338927.

Comment 13

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Boris, is there any problems without part 3?  If there is no problem, I'd
> like you to land these patches without part 3 here, and drop
> Servo_AnimationValues_Populate in bug 1338927.

OK, I can drop part 3.

Comment 14

[Boris Chiou [:boris]]

Comment on attachment 8836561 [details]
Bug 1338087 - Part 2: Drop the computation of StyleAnimationValue on stylo.

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

Comment 15

[Boris Chiou [:boris]]

Attachment: Bug 1338087 - [Servo] Drop Servo_RestyleWithAddedDeclaration.

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

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8836571 [details]
Bug 1338087 - [Servo] Drop Servo_RestyleWithAddedDeclaration.

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

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8836571 [details]
Bug 1338087 - [Servo] Drop Servo_RestyleWithAddedDeclaration.

https://reviewboard.mozilla.org/r/111972/#review113202

Comment 18

[Boris Chiou [:boris]]

Attachment: Bug 1338087 - Part 3: Remove asserts-if on crashtest 1244595-1.html.

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

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8836588 [details]
Bug 1338087 - Part 3: Remove asserts-if on crashtest 1244595-1.html.

https://reviewboard.mozilla.org/r/111984/#review113212

Comment 20

[Boris Chiou [:boris]]

Attachment: Servo PR, #15524

Comment 21

[Boris Chiou [:boris]]

Comment on attachment 8836588 [details]
Bug 1338087 - Part 3: Remove asserts-if on crashtest 1244595-1.html.

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

Comment 22

[Boris Chiou [:boris]]

wait for Bug 1338926 because servo vcs sync is broken.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29f7ce98c99f
Part 1: Don't apply paced spacing on stylo. r=hiro
https://hg.mozilla.org/integration/autoland/rev/985981c12364
Part 2: Drop the computation of StyleAnimationValue on stylo. r=hiro
https://hg.mozilla.org/integration/autoland/rev/881d38183d2f
Part 3: Remove asserts-if on crashtest 1244595-1.html. r=heycam

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29f7ce98c99f
https://hg.mozilla.org/mozilla-central/rev/985981c12364
https://hg.mozilla.org/mozilla-central/rev/881d38183d2f