Closed Bug 1420928 Opened 7 years ago Closed 6 years ago

stylo: animation-name and transition-property probably shouldn't be early properties.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: emilio, Assigned: hiro)

References

Details

Attachments

(10 files, 7 obsolete files)

Bug 1420928 - Pass a const reference of nsStyleDisplay instead of a raw pointer to DoUpdateTransitions.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - BuildKeyframes takes animation name and timing function instead of StyleAnimation.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - Add getters for each animation property in nsStyleDisplay.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - Add getters for each transition property in nsStyleDisplay.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - Add a function that calculates combined duration with a given position in the transition-property list.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - Reuse computed values for animation properties repeatedly if the computed values' length is less than animation-name length.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - Reuse computed values for transition properties repeatedly if the computed values' length is less than transition-property length.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - Drop StyleTransition::GetCombinedDuration.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1420928 - Templatize functions that ensures nsStyleAutoArray length.
59 bytes, text/x-review-board-request
boris
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review 
These were added as such in bug 1346655, but the spec quote mentioned in that bug only mentions the used value.

As such, it looks to me that we shouldn't touch the computed value at all, and that that clamping should be done at used-value time.
Hiro, any chance you can take a look at this? This will allow to optimize more easily the parsing of the transition / animation longhands when there are custom properties in there.
Flags: needinfo?(hikezoe)
Oh right, indeed.  I can take this. Though it will take some time that I recall it. :)
Assignee: nobody → hikezoe
Flags: needinfo?(hikezoe)
Priority: -- → P3
See Also: → 1426246
I made a mistake when shrinking nsStyleAutoArray in the previous try.  A revised try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a7eac65be58780f9008555b5d44900b870402c8
Status: NEW → ASSIGNED
I did intentionally leave gecko part (nsRuleNode::ComputeDisplayData) as it is since it will be dropped soon.
Comment on attachment 8939654 [details]
Bug 1420928 - Pass a const reference of nsStyleDisplay instead of a raw pointer to DoUpdateTransitions.

https://reviewboard.mozilla.org/r/209952/#review215598

::: layout/style/nsTransitionManager.cpp:650
(Diff revision 1)
> -  const nsStyleDisplay *disp =
> +  const nsStyleDisplay* disp =
>        aNewStyle->ComputedData()->GetStyleDisplay();
> -  return DoUpdateTransitions(disp,
> +  return DoUpdateTransitions(*disp,

Should we add a MOZ_ASSERT here that disp is non-null? (To replace the assertion we dropped.)
Attachment #8939654 - Flags: review?(bbirtles) → review+
Comment on attachment 8939654 [details]
Bug 1420928 - Pass a const reference of nsStyleDisplay instead of a raw pointer to DoUpdateTransitions.

https://reviewboard.mozilla.org/r/209952/#review215602
Attachment #8939654 - Flags: review+
Comment on attachment 8939654 [details]
Bug 1420928 - Pass a const reference of nsStyleDisplay instead of a raw pointer to DoUpdateTransitions.

https://reviewboard.mozilla.org/r/209952/#review215598

> Should we add a MOZ_ASSERT here that disp is non-null? (To replace the assertion we dropped.)

They cannot return null btw, I guess we should just rename them.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #38)
> Comment on attachment 8939654 [details]
> Bug 1420928 - Pass a const reference of nsStyleDisplay instead of a raw
> pointer to DoUpdateTransitions.
> 
> https://reviewboard.mozilla.org/r/209952/#review215598
> 
> > Should we add a MOZ_ASSERT here that disp is non-null? (To replace the assertion we dropped.)
> 
> They cannot return null btw, I guess we should just rename them.

In that case, don't worry about the assert. I thought that was the case but wasn't sure, given the Get* naming.
Comment on attachment 8939654 [details]
Bug 1420928 - Pass a const reference of nsStyleDisplay instead of a raw pointer to DoUpdateTransitions.

https://reviewboard.mozilla.org/r/209952/#review215608
Attachment #8939654 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939655 [details]
Bug 1420928 - BuildKeyframes takes animation name and timing function instead of StyleAnimation.

https://reviewboard.mozilla.org/r/209954/#review215610
Attachment #8939655 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939656 [details]
Bug 1420928 - Add getters for each animation property in nsStyleDisplay.

https://reviewboard.mozilla.org/r/209956/#review215614

::: layout/style/nsStyleStruct.h:2646
(Diff revision 2)
> +  }
> +  mozilla::dom::FillMode GetAnimationFillMode(uint32_t aIndex) const
> +  {
> +    return mAnimations[aIndex % mAnimationFillModeCount].GetFillMode();
> +  }
> +  uint8_t GetAnimationPlayState(uint32_t aIndex) const

Hope that one day we could use mozilla::dom::AnimationPlayState (enum class), instead of NS_STYLE_ANIMATION_PLAY_STATE_XXXX.
Attachment #8939656 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939657 [details]
Bug 1420928 - Add getters for each transition property in nsStyleDisplay.

https://reviewboard.mozilla.org/r/209958/#review215618
Attachment #8939657 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939658 [details]
Bug 1420928 - Add a function that calculates combined duration with a given position in the transition-property list.

https://reviewboard.mozilla.org/r/209960/#review215620

::: layout/style/nsStyleStruct.h:2631
(Diff revision 2)
>    {
>      return mTransitions[aIndex % mTransitionTimingFunctionCount].GetTimingFunction();
>    }
> +  float GetTransitionCombinedDuration(uint32_t aIndex) const
> +  {
> +    // http://dev.w3.org/csswg/css-transitions/#combined-duration

nit: https://drafts.csswg.org/css-transitions/#transition-combined-duration
Attachment #8939658 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939659 [details]
Bug 1420928 - Make transition_combined_duration_at consider duration and delay value are used by repeating the list of values if the given index is greater than the list length.

https://reviewboard.mozilla.org/r/209962/#review215624
Attachment #8939659 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939660 [details]
Bug 1420928 - Call transition_combined_duration_at to get combined duration instead of calculating directly.

https://reviewboard.mozilla.org/r/209964/#review215626
Attachment #8939660 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939661 [details]
Bug 1420928 - Check each animation sub property count in animations_equals.

https://reviewboard.mozilla.org/r/209966/#review215676
Attachment #8939661 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939662 [details]
Bug 1420928 - Reuse computed values for animation properties repeatedly if the computed values' length is less than animation-name length.

https://reviewboard.mozilla.org/r/209968/#review215682
Attachment #8939662 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939663 [details]
Bug 1420928 - Reuse computed values for transition properties repeatedly if the computed values' length is less than transition-property length.

https://reviewboard.mozilla.org/r/209970/#review215684
Attachment #8939663 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939664 [details]
Bug 1420928 - Don't fill out deficient animation/transition property values in the style struct.

https://reviewboard.mozilla.org/r/209972/#review215690
Attachment #8939664 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939665 [details]
Bug 1420928 - Drop animation-name and transition-property from early properties.

https://reviewboard.mozilla.org/r/209974/#review215692
Attachment #8939665 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939666 [details]
Bug 1420928 - Drop StyleTransition::GetCombinedDuration.

https://reviewboard.mozilla.org/r/209976/#review215694
Attachment #8939666 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939667 [details]
Bug 1420928 - Templatize functions that ensures nsStyleAutoArray length.

https://reviewboard.mozilla.org/r/209978/#review215710

Nice.
Attachment #8939667 - Flags: review?(boris.chiou) → review+
Comment on attachment 8939668 [details]
Bug 1420928 - Shrink array for StyleAnimation and StyleTransition if possible.

https://reviewboard.mozilla.org/r/209980/#review215722

This looks good to me, but I have a minor question below.

::: servo/components/style/properties/gecko.mako.rs
(Diff revision 2)
> -        self.gecko.m${type.capitalize()}s.ensure_len(input_len);
>  
>          self.gecko.m${type.capitalize()}${gecko_ffi_name}Count = input_len as u32;
> +
> +        let max = self.max_${type}_property_count();
> +        self.gecko.m${type.capitalize()}s.set_len(max);
> +
>          for (gecko, servo) in self.gecko.m${type.capitalize()}s.iter_mut().take(input_len as usize).zip(v) {
>              gecko.m${gecko_ffi_name} = servo.seconds() * 1000.;
>          }

Actually, I'm not sure if this is ok to set the length according to the maximum among all animation/transition counts. I think this works, but we may change the length of `self.gecko.m${type.capitalize()}s` many times because we use the maximum amoung all the counts, especially when we update the shorthand value of animation/transition. (Is it possible?)
e.g.
At the initial setup or the shorthand setup, these functions are called sequentially to intialize the all the transition properties.
```
set_transition_property(...)
set_transition_delay(...)
set_transition_duration(...)
set_transition_timine_function(...)
```
When calling `set_transition_property(...)`, we update `self.gecko.mTransitionPropertyCount`, but other _counts_ are still out-of-updated, so the length of `self.gecko.mTransitions` is set accroding to the new `mTransitionPropertyCount` and old (`mTransitionDelayCount`, `mTransitionDurationCount`, and `mTransitionTimingFunctionCount`). This may not be a problem because we eventually set the length of `self.gecko.mTransitions` to a suitable length. I just wonder if this is acceptable to us? Thanks.
Attachment #8939668 - Flags: review?(boris.chiou) → review+
(In reply to Boris Chiou [:boris] from comment #54)
> Comment on attachment 8939668 [details]
> Bug 1420928 - Shrink array for StyleAnimation and StyleTransition if
> possible.
> 
> https://reviewboard.mozilla.org/r/209980/#review215722
> 
> This looks good to me, but I have a minor question below.
> 
> ::: servo/components/style/properties/gecko.mako.rs
> (Diff revision 2)
> > -        self.gecko.m${type.capitalize()}s.ensure_len(input_len);
> >  
> >          self.gecko.m${type.capitalize()}${gecko_ffi_name}Count = input_len as u32;
> > +
> > +        let max = self.max_${type}_property_count();
> > +        self.gecko.m${type.capitalize()}s.set_len(max);
> > +
> >          for (gecko, servo) in self.gecko.m${type.capitalize()}s.iter_mut().take(input_len as usize).zip(v) {
> >              gecko.m${gecko_ffi_name} = servo.seconds() * 1000.;
> >          }
> 
> Actually, I'm not sure if this is ok to set the length according to the
> maximum among all animation/transition counts. I think this works, but we
> may change the length of `self.gecko.m${type.capitalize()}s` many times
> because we use the maximum amoung all the counts, especially when we update
> the shorthand value of animation/transition. (Is it possible?)
> e.g.
> At the initial setup or the shorthand setup, these functions are called
> sequentially to intialize the all the transition properties.
> ```
> set_transition_property(...)
> set_transition_delay(...)
> set_transition_duration(...)
> set_transition_timine_function(...)
> ```
> When calling `set_transition_property(...)`, we update
> `self.gecko.mTransitionPropertyCount`, but other _counts_ are still
> out-of-updated, so the length of `self.gecko.mTransitions` is set accroding
> to the new `mTransitionPropertyCount` and old (`mTransitionDelayCount`,
> `mTransitionDurationCount`, and `mTransitionTimingFunctionCount`). This may
> not be a problem because we eventually set the length of
> `self.gecko.mTransitions` to a suitable length. I just wonder if this is
> acceptable to us? Thanks.

That's a good question.  The initial length of these properties is 1, and in most cases the length of transition properties is 1, so it's not be a big deal I think.  But the question noticed me that nsStyleAutoArray::SetLengthNonZero() does not early return in the case where the length is not changed.  I will add an additional patch to avoid calling the FFI if the length is not changed.

Thanks!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> (In reply to Boris Chiou [:boris] from comment #54)
> > Comment on attachment 8939668 [details]
> > Bug 1420928 - Shrink array for StyleAnimation and StyleTransition if
> > possible.
> > 
> > https://reviewboard.mozilla.org/r/209980/#review215722
> > 
> > This looks good to me, but I have a minor question below.
> > 
> > ::: servo/components/style/properties/gecko.mako.rs
> > (Diff revision 2)
> > > -        self.gecko.m${type.capitalize()}s.ensure_len(input_len);
> > >  
> > >          self.gecko.m${type.capitalize()}${gecko_ffi_name}Count = input_len as u32;
> > > +
> > > +        let max = self.max_${type}_property_count();
> > > +        self.gecko.m${type.capitalize()}s.set_len(max);
> > > +
> > >          for (gecko, servo) in self.gecko.m${type.capitalize()}s.iter_mut().take(input_len as usize).zip(v) {
> > >              gecko.m${gecko_ffi_name} = servo.seconds() * 1000.;
> > >          }
> > 
> > Actually, I'm not sure if this is ok to set the length according to the
> > maximum among all animation/transition counts. I think this works, but we
> > may change the length of `self.gecko.m${type.capitalize()}s` many times
> > because we use the maximum amoung all the counts, especially when we update
> > the shorthand value of animation/transition. (Is it possible?)
> > e.g.
> > At the initial setup or the shorthand setup, these functions are called
> > sequentially to intialize the all the transition properties.
> > ```
> > set_transition_property(...)
> > set_transition_delay(...)
> > set_transition_duration(...)
> > set_transition_timine_function(...)
> > ```
> > When calling `set_transition_property(...)`, we update
> > `self.gecko.mTransitionPropertyCount`, but other _counts_ are still
> > out-of-updated, so the length of `self.gecko.mTransitions` is set accroding
> > to the new `mTransitionPropertyCount` and old (`mTransitionDelayCount`,
> > `mTransitionDurationCount`, and `mTransitionTimingFunctionCount`). This may
> > not be a problem because we eventually set the length of
> > `self.gecko.mTransitions` to a suitable length. I just wonder if this is
> > acceptable to us? Thanks.
> 
> That's a good question.  The initial length of these properties is 1, and in
> most cases the length of transition properties is 1, so it's not be a big
> deal I think.  But the question noticed me that
> nsStyleAutoArray::SetLengthNonZero() does not early return in the case where
> the length is not changed.  I will add an additional patch to avoid calling
> the FFI if the length is not changed.
> 
> Thanks!

Doesn't that patch also have the risk of re-allocating a bunch in the worst case? Not sure it's worth to shrink it in the unlikely case the length gets reduced.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #68)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> > (In reply to Boris Chiou [:boris] from comment #54)
> > > Comment on attachment 8939668 [details]
> > > Bug 1420928 - Shrink array for StyleAnimation and StyleTransition if
> > > possible.
> > > 
> > > https://reviewboard.mozilla.org/r/209980/#review215722
> > > 
> > > This looks good to me, but I have a minor question below.
> > > 
> > > ::: servo/components/style/properties/gecko.mako.rs
> > > (Diff revision 2)
> > > > -        self.gecko.m${type.capitalize()}s.ensure_len(input_len);
> > > >  
> > > >          self.gecko.m${type.capitalize()}${gecko_ffi_name}Count = input_len as u32;
> > > > +
> > > > +        let max = self.max_${type}_property_count();
> > > > +        self.gecko.m${type.capitalize()}s.set_len(max);
> > > > +
> > > >          for (gecko, servo) in self.gecko.m${type.capitalize()}s.iter_mut().take(input_len as usize).zip(v) {
> > > >              gecko.m${gecko_ffi_name} = servo.seconds() * 1000.;
> > > >          }
> > > 
> > > Actually, I'm not sure if this is ok to set the length according to the
> > > maximum among all animation/transition counts. I think this works, but we
> > > may change the length of `self.gecko.m${type.capitalize()}s` many times
> > > because we use the maximum amoung all the counts, especially when we update
> > > the shorthand value of animation/transition. (Is it possible?)
> > > e.g.
> > > At the initial setup or the shorthand setup, these functions are called
> > > sequentially to intialize the all the transition properties.
> > > ```
> > > set_transition_property(...)
> > > set_transition_delay(...)
> > > set_transition_duration(...)
> > > set_transition_timine_function(...)
> > > ```
> > > When calling `set_transition_property(...)`, we update
> > > `self.gecko.mTransitionPropertyCount`, but other _counts_ are still
> > > out-of-updated, so the length of `self.gecko.mTransitions` is set accroding
> > > to the new `mTransitionPropertyCount` and old (`mTransitionDelayCount`,
> > > `mTransitionDurationCount`, and `mTransitionTimingFunctionCount`). This may
> > > not be a problem because we eventually set the length of
> > > `self.gecko.mTransitions` to a suitable length. I just wonder if this is
> > > acceptable to us? Thanks.
> > 
> > That's a good question.  The initial length of these properties is 1, and in
> > most cases the length of transition properties is 1, so it's not be a big
> > deal I think.  But the question noticed me that
> > nsStyleAutoArray::SetLengthNonZero() does not early return in the case where
> > the length is not changed.  I will add an additional patch to avoid calling
> > the FFI if the length is not changed.
> > 
> > Thanks!
> 
> Doesn't that patch also have the risk of re-allocating a bunch in the worst
> case? Not sure it's worth to shrink it in the unlikely case the length gets
> reduced.

Hmm, a worst case I can think of (and it's common to some extent) is that creating/removing (two or more) short transitions repeatedly.  It is really big deals?  I don't think it's a big deal but given that both of you guys are concerned it, I will drop the changes in this bug.  Thanks!
Attachment #8939765 - Flags: review?(boris.chiou)
Attachment #8939668 - Attachment is obsolete: true
Attachment #8939765 - Attachment is obsolete: true
Attached file Servo PR — 

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8939659 -
        Attachment is obsolete: true

    
  

        Attachment #8939660 -
        Attachment is obsolete: true

    
  

        Attachment #8939661 -
        Attachment is obsolete: true

    
  

        Attachment #8939664 -
        Attachment is obsolete: true

    
  

        Attachment #8939665 -
        Attachment is obsolete: true

    
    

    
  
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93d2585579e7
Pass a const reference of nsStyleDisplay instead of a raw pointer to DoUpdateTransitions. r=birtles,boris
https://hg.mozilla.org/integration/autoland/rev/7d0c3d617c4d
BuildKeyframes takes animation name and timing function instead of StyleAnimation. r=boris
https://hg.mozilla.org/integration/autoland/rev/60d947dd9da1
Add getters for each animation property in nsStyleDisplay. r=boris
https://hg.mozilla.org/integration/autoland/rev/b870588dc2a8
Add getters for each transition property in nsStyleDisplay. r=boris
https://hg.mozilla.org/integration/autoland/rev/fa35bc2879ad
Add a function that calculates combined duration with a given position in the transition-property list. r=boris
https://hg.mozilla.org/integration/autoland/rev/2004d0192d0c
Reuse computed values for animation properties repeatedly if the computed values' length is less than animation-name length. r=boris
https://hg.mozilla.org/integration/autoland/rev/2cf3e262e0d4
Reuse computed values for transition properties repeatedly if the computed values' length is less than transition-property length. r=boris
https://hg.mozilla.org/integration/autoland/rev/f61cba8c9843
Drop StyleTransition::GetCombinedDuration. r=boris
https://hg.mozilla.org/integration/autoland/rev/96c9efcb4a31
Templatize functions that ensures nsStyleAutoArray length. r=boris

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: