Closed Bug 1338087 Opened 7 years ago Closed 7 years ago

stylo: Stop storing StyleAnimationValue on main thread

Categories

(Core :: DOM: Animation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1338087 - Part 1: Don't apply paced spacing on stylo.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1338087 - Part 2: Drop the computation of StyleAnimationValue on stylo.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1338087 - Part 3: Remove asserts-if on crashtest 1244595-1.html.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Servo PR, #15524
41 bytes, text/x-github-pull-request
Details | Review 
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.)
Depends on: 1332633, 1329878
Sorry, I haven't been following this closely but what are we doing for compositor animations? Don't we need StyleAnimationValue objects there still?
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!
Status: NEW → ASSIGNED
Summary: stylo: Stop storing StyleAnimationValue → stylo: Stop storing StyleAnimationValue in main thread
Summary: stylo: Stop storing StyleAnimationValue in main thread → stylo: Stop storing StyleAnimationValue on main thread
Blocks: 1338927
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 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 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 on attachment 8836562 [details]
Bug 1338087 - Part 3: Append PropertyStyleAnimationValuePair from Servo side.

https://reviewboard.mozilla.org/r/111880/#review113192
Attachment #8836562 - Flags: review?(manishearth) → review+
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.
Attachment #8836560 - Flags: review?(hikezoe) → review+
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.
Attachment #8836561 - Flags: review?(hikezoe) → review+
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.
Flags: needinfo?(boris.chiou)
(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.
Flags: needinfo?(boris.chiou)
Attachment #8836562 - Attachment is obsolete: true
Attachment #8836562 - Flags: review?(hikezoe)
Attachment #8836571 - Flags: review?(manishearth)
Comment on attachment 8836571 [details]
Bug 1338087 - [Servo] Drop Servo_RestyleWithAddedDeclaration.

https://reviewboard.mozilla.org/r/111972/#review113202
Attachment #8836571 - Flags: review?(manishearth) → review+
Attachment #8836588 - Flags: review?(cam)
Comment on attachment 8836588 [details]
Bug 1338087 - Part 3: Remove asserts-if on crashtest 1244595-1.html.

https://reviewboard.mozilla.org/r/111984/#review113212
Attachment #8836588 - Flags: review?(cam) → review+
Attached file Servo PR, #15524 — 

    
    

    
      
  

    
  

        Attachment #8836571 -
        Attachment is obsolete: true

    
    

    
  
wait for Bug 1338926 because servo vcs sync is broken.

    
    

    
  
Pushed by bchiou@mozilla.com:
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/29f7ce98c99f
https://hg.mozilla.org/mozilla-central/rev/985981c12364
https://hg.mozilla.org/mozilla-central/rev/881d38183d2f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox54:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1324696
Depends on: 1353202

    
  
Blocks: 1409278

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: