Closed Bug 1343753 Opened 7 years ago Closed 7 years ago

stylo: Update CSS transitions with servo's computed values instead of nsStyleContext

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: boris)

References

Details

Attachments

(10 files, 3 obsolete files)

Bug 1343753 - Part 1: Make GetTransitionKeyframes as a local static function.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1343753 - Part 2: Update GetTransitionKeyframes for stylo.
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
Details
Bug 1343753 - Part 3: Use AnimationValue in ElementPropertyTransition and CSSTransition.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1343753 - Part 4: Introduce AnimationValue::IsInterpolableWith.
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
Details
Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
hiro
: review+
Details
Bug 1343753 - Part 7: Use template for UpdateTransitions and ConsiderInitiatingTransition.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1343753 - Part 8: Rename and add new methods in nsTransitionManager.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1343753 - Part 9: Early return for non-animatable properties for transitions.
59 bytes, text/x-review-board-request
hiro
: review+
manishearth
: review+
Details
Servo PR, #16381
41 bytes, text/x-github-pull-request
Details | Review 
This is a sibling of bug 1340322.
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
Blocks: 1341372
I am working on this bug, and I think this is necessary for CSS Transition, so I would like to set the priority as P1.
Priority: -- → P1
Comment on attachment 8845884 [details]
Bug 1343753 - Part 7: Use template for UpdateTransitions and ConsiderInitiatingTransition.

https://reviewboard.mozilla.org/r/118964/#review121306

::: layout/style/nsTransitionManager.cpp:575
(Diff revision 1)
>    RefPtr<nsStyleContext> afterChangeStyle;
>    if (collection) {
>      MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
>                 "ServoStyleSets should not use nsTransitionManager "
>                 "for transitions");
> +    // FIXME: Bug 1341372, we need to resolve the after-change style for stylo.

Sorry, the bug number is not correct. "Remove animation styles when generating transitions" is the step 2 of M2, so I need to file another bug for this.
(In reply to Boris Chiou [:boris] from comment #12)
> ::: layout/style/nsTransitionManager.cpp:575
> (Diff revision 1)
> >    RefPtr<nsStyleContext> afterChangeStyle;
> >    if (collection) {
> >      MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
> >                 "ServoStyleSets should not use nsTransitionManager "
> >                 "for transitions");
> > +    // FIXME: Bug 1341372, we need to resolve the after-change style for stylo.
> 
> Sorry, the bug number is not correct. "Remove animation styles when
> generating transitions" is the step 2 of M2, so I need to file another bug
> for this.

Yeah, I am guessing we will get the after-change style before creating a SequentialTask to trigger transitions.
Hi Boris, what is the scope of this bug? What are the pieces not covered by this bug?

At a first glance, we still need to trigger an animation-only restyle, correctly calculate the after-change style, store the transition endpoints, create transitions in a SequentialTask etc.

A little bit more context would help. Thanks.
Comment on attachment 8845878 [details]
Bug 1343753 - Part 1: Make GetTransitionKeyframes as a local static function.

https://reviewboard.mozilla.org/r/118952/#review121332
Attachment #8845878 - Flags: review?(bbirtles) → review+
Comment on attachment 8845879 [details]
Bug 1343753 - Part 2: Update GetTransitionKeyframes for stylo.

https://reviewboard.mozilla.org/r/118954/#review121334

::: layout/style/nsTransitionManager.cpp:737
(Diff revision 1)
> +    pv.mServoDeclarationBlock =
> +      Servo_AnimationValue_Uncompute(aValue.mServo).Consume();

(I'm trusting Manish to check if this is sensible or not since I'm not familiar with the lifetime/ownership arrangement here.)

::: servo/ports/geckolib/glue.rs:276
(Diff revision 1)
> +    let value = AnimationValue::as_arc(&value);
> +    let mut block = PropertyDeclarationBlock::new();
> +    block.push(value.uncompute(), Importance::Normal);
> +    Arc::new(RwLock::new(block)).into_strong()

(Again, I'm assuming Manish will review this part.)
Attachment #8845879 - Flags: review?(bbirtles) → review+
Comment on attachment 8845880 [details]
Bug 1343753 - Part 3: Use AnimationValue in ElementPropertyTransition and CSSTransition.

https://reviewboard.mozilla.org/r/118956/#review121336

::: layout/style/StyleAnimationValue.h:579
(Diff revision 1)
>    inline bool operator==(const AnimationValue& aOther) const;
> +  inline bool operator!=(const AnimationValue& aOther) const;

(We don't really need inline for any of these. In many cases, the function definition isn't available so the compiler won't be able to inline it. And when the function definition is available, the compiler will most likely inline it anyway, regardless of whether or not the keyword is present.)

::: layout/style/StyleAnimationValueInlines.h:17
(Diff revision 1)
>      // mGecko and mServo are mutual exclusive, one of them must be null
>      if (mServo) {

(This really should be in the header file and we should be asserting this. This whole AnimationValue struct is quite messy it seems.)

::: layout/style/StyleAnimationValueInlines.h:27
(Diff revision 1)
>  }
>  
> +bool
> +AnimationValue::operator!=(const AnimationValue& aOther) const
> +{
> +  return !(*this == aOther);

('return !operator==(aOther);' would be simpler still.)

::: layout/style/nsTransitionManager.cpp:132
(Diff revision 1)
> +                                          replacedTo.mServo,
> +                                          valuePosition).Consume();
> +      if (startValue.mServo) {
> +        mKeyframes[0].mPropertyValues[0].mServoDeclarationBlock =
> +          Servo_AnimationValue_Uncompute(startValue.mServo).Consume();
> +

Nit: I don't think we need this blank line here?
Attachment #8845880 - Flags: review?(bbirtles) → review+
Comment on attachment 8845881 [details]
Bug 1343753 - Part 4: Introduce AnimationValue::IsInterpolableWith.

https://reviewboard.mozilla.org/r/118958/#review121338

::: layout/style/StyleAnimationValue.h:595
(Diff revision 1)
> +  inline bool IsInterpolatable(nsCSSPropertyID aProperty,
> +                               const AnimationValue& aToValue) const;

Again, no need for inline here.

::: layout/style/StyleAnimationValueInlines.h:65
(Diff revision 1)
>  }
>  
> +bool
> +AnimationValue::IsInterpolatable(nsCSSPropertyID aProperty,
> +                                 const AnimationValue& aToValue) const
> +{

I think this needs to assert something like:

  !mServo != !mGecko && (i.e. either one is set)
  !!mServo == !!aToValue.mServo &&
  !!mGecko == !!aToValue.mGecko
  (i.e. the same one is set)
(In reply to Brian Birtles (:birtles) from comment #14)
> Hi Boris, what is the scope of this bug? What are the pieces not covered by
> this bug?
> 
> At a first glance, we still need to trigger an animation-only restyle,
> correctly calculate the after-change style, store the transition endpoints,
> create transitions in a SequentialTask etc.
> 
> A little bit more context would help. Thanks.

I didn't cover the "trigger" part into this bug because I thought this bug only wants to use ComputedValues in UpdateTransitions(), so leave the "trigger" part into bug 1341372. However, in order to test these patches, we still need to trigger those things you mentioned. (I did test these patches by calling StyleContextChanged in PostTraversal with oldStyleContext and newStyleContext before hiro's work on two pass restyle.) If we think it's better to implement them here, I can also include them. Thanks.
(In reply to Boris Chiou [:boris] from comment #19)
> (In reply to Brian Birtles (:birtles) from comment #14)
> > Hi Boris, what is the scope of this bug? What are the pieces not covered by
> > this bug?
> > 
> > At a first glance, we still need to trigger an animation-only restyle,
> > correctly calculate the after-change style, store the transition endpoints,
> > create transitions in a SequentialTask etc.
> > 
> > A little bit more context would help. Thanks.
> 
> I didn't cover the "trigger" part into this bug because I thought this bug
> only wants to use ComputedValues in UpdateTransitions(), so leave the
> "trigger" part into bug 1341372. However, in order to test these patches, we
> still need to trigger those things you mentioned. (I did test these patches
> by calling StyleContextChanged in PostTraversal with oldStyleContext and
> newStyleContext before hiro's work on two pass restyle.) If we think it's
> better to implement them here, I can also include them. Thanks.

And I'm start to work on those you mentioned. Thanks for the reminder.
Comment on attachment 8845881 [details]
Bug 1343753 - Part 4: Introduce AnimationValue::IsInterpolableWith.

https://reviewboard.mozilla.org/r/118958/#review121352

::: layout/style/StyleAnimationValue.h:595
(Diff revision 1)
>    // Uncompute this AnimationValue and then serialize it.
>    inline void SerializeSpecifiedValue(nsCSSPropertyID aProperty,
>                                        nsAString& aString) const;
> +
> +  // Check if |*this| and |aToValue| can be interpolated.
> +  inline bool IsInterpolatable(nsCSSPropertyID aProperty,

Let's call this IsInterpolableWith

::: layout/style/StyleAnimationValueInlines.h:65
(Diff revision 1)
>  }
>  
> +bool
> +AnimationValue::IsInterpolatable(nsCSSPropertyID aProperty,
> +                                 const AnimationValue& aToValue) const
> +{

Actually I think we should either do this as:

  MOZ_ASSERT(!IsNull() && !aOther.IsNull(),
             "Interpolation end points should be non-null");
  MOZ_ASSERT(!mGecko == !aOther.mGecko &&
             !mServo == !aOther.mServo,
             "Animation values should have the same style engine");

or make it handle null values (return false), as in:

  if (IsNull() || aOther.IsNull()) {
    return false;
  }

  MOZ_ASSERT(!mGecko == !aOther.mGecko &&
             !mServo == !aOther.mServo,
             "Animation values should have the same style engine");

::: layout/style/nsTransitionManager.cpp:836
(Diff revision 1)
>    bool haveChange = startValue != endValue;
>  
>    bool shouldAnimate =
>      haveValues &&
>      haveChange &&
> -    // Check that we can interpolate between these values
> +    // Check that we can interpolate between these values.

I don't think we need this comment any more. The function name is self-explanatory.
Attachment #8845881 - Flags: review?(bbirtles) → review+
Comment on attachment 8845882 [details]
Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.

https://reviewboard.mozilla.org/r/118960/#review121358

::: commit-message-50a37:4
(Diff revision 1)
> +Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.
> +
> +In order to use ServoComputedValues, we need to introduce a new FFI,
> +Servo_AnimationValue_Extract, which extract an AnimationValue from the

Nit: s/extract/extracts/

::: commit-message-50a37:8
(Diff revision 1)
> +ServoComputedValues, because I will use template for nsStyleContext and
> +ServoComputedStyleValues in UpdateTransitions() and

(Wow, we have ServoComputedValues *and* ServoComputedStyleValues--and I reviewed that too! We should have named ServoComputedStyleValues something else like ServoComputedStylePair, ServoComputedStyleContext, ServoCurrentAndParentStyle, or something.)
Attachment #8845882 - Flags: review?(bbirtles) → review+
Comment on attachment 8845883 [details]
Bug 1343753 - Part 6: Use NonOwningAnimationTarget in UpdateTransitions.

https://reviewboard.mozilla.org/r/118962/#review121368

::: dom/animation/EffectSet.h:60
(Diff revision 1)
>    static EffectSet* GetEffectSet(const dom::Element* aElement,
>                                   CSSPseudoElementType aPseudoType);
> +  static EffectSet* GetEffectSet(const NonOwningAnimationTarget& aTarget);

Why don't we just change the first version of GetEffectSet to take a NonOwningAnimationTarget rather than having two versions?

Also, are we always setting the mElement member to the parent element for pseudo elements, or does it sometimes represent the generated content element? If we're using NonOwningAnimationTarget then I'd like to be sure that it is always the parent element for pseudo elements.
Comment on attachment 8845884 [details]
Bug 1343753 - Part 7: Use template for UpdateTransitions and ConsiderInitiatingTransition.

https://reviewboard.mozilla.org/r/118964/#review121372

::: layout/style/nsTransitionManager.cpp:575
(Diff revision 1)
>    RefPtr<nsStyleContext> afterChangeStyle;
>    if (collection) {
>      MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
>                 "ServoStyleSets should not use nsTransitionManager "
>                 "for transitions");
> +    // FIXME: Bug 1341372, we need to resolve the after-change style for stylo.

(Opening an issue for updating this bug number so we don't forget to do it.)

::: layout/style/nsTransitionManager.cpp:592
(Diff revision 1)
> +    if (mPresContext->StyleSet()->IsServo()) {
> +      ServoComputedStyleValues oldStyle = {
> +        aOldStyleContext->StyleSource().AsServoComputedValues(),
> +        nullptr
> +      };
> +      ServoComputedStyleValues newStyle = {
> +        afterChangeStyle->StyleSource().AsServoComputedValues(),
> +        afterChangeStyle->GetParent()
> +          ? afterChangeStyle->GetParent()->StyleSource().AsServoComputedValues()
> +          : nullptr
> +      };

I'm curious about this code. I'm not really familiar with the setup in Stylo so can you explain why this is correct -- why we need the parent style for the new style but not the old?

::: layout/style/nsTransitionManager.cpp:619
(Diff revision 1)
> +    // FIXME: Bug 1341372, this function may call UpdateCascadeResult, which is
> +    // still use newStyleContext to get the rule node, we need to fix it.

I don't understand this comment.
Comment on attachment 8845885 [details]
Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.

https://reviewboard.mozilla.org/r/118966/#review121376

::: layout/base/RestyleManager.h:266
(Diff revision 1)
>  
> +  // Are we currently in the middle of a call to ProcessRestyles?
> +  // This flag is used both as a debugging aid to assert that we are not
> +  // performing nested calls to ProcessPendingRestyles, as well as to ignore
> +  // redundant calls to IncrementAnimationGeneration.
> +  bool mIsProcessingRestyles;

This is only used by the GeckoRestyleManager, right? So it should stay there and we should just use IsGecko() / AsGecko() to access it?
Comment on attachment 8845886 [details]
Bug 1343753 - Part 9: Remove assertions from ConsiderInitiatingTransition.

https://reviewboard.mozilla.org/r/118968/#review121374

It seems odd to remove this in a separate patch. Shouldn't it go with the patch that invalidated this assertion?
Attachment #8845886 - Flags: review?(bbirtles) → review+
Comment on attachment 8845887 [details]
Bug 1343753 - Part 10: Make StyleDisplay as the function argument of StyleContextChanged.

https://reviewboard.mozilla.org/r/118970/#review121366

What are the different ways of getting StyleDisplay? It appears we don't use this refactoring in this patch series so it's not clear to me why we need it or why it's better than, say, adding a check in side StyleContextChanged.
Comment on attachment 8845883 [details]
Bug 1343753 - Part 6: Use NonOwningAnimationTarget in UpdateTransitions.

https://reviewboard.mozilla.org/r/118962/#review121368

> Why don't we just change the first version of GetEffectSet to take a NonOwningAnimationTarget rather than having two versions?
> 
> Also, are we always setting the mElement member to the parent element for pseudo elements, or does it sometimes represent the generated content element? If we're using NonOwningAnimationTarget then I'd like to be sure that it is always the parent element for pseudo elements.

I noticed that for css transition, the element represent the genereated content element (accroding to the assertion and comment in StyleContextChanged) on gecko. Therefore, I should drop this patch becuase it is not always the parent element for pseudo elements. Thanks.
Comment on attachment 8845884 [details]
Bug 1343753 - Part 7: Use template for UpdateTransitions and ConsiderInitiatingTransition.

https://reviewboard.mozilla.org/r/118964/#review121372

> I'm curious about this code. I'm not really familiar with the setup in Stylo so can you explain why this is correct -- why we need the parent style for the new style but not the old?

We need the new parent comptued values while setting keyframes in ConsiderInitiatingTransition(); however, no one needs the old parent computed value, so this is the reason I make it nullptr.

However, I just notice that maybe we shouldn't reuse StyleContextChanged for stylo. I notice that we create the new StyleContext until ServoRestyleManager::ProcessPostTraversal(), so we only have the new ServoComputedValues in SequentialTask, which means there is no way to reuse StyleContext to get nsStyleContext::HasPseudoElementData(). I might need to write another version of StyleContextChanged for stylo.

> I don't understand this comment.

MaybeUpdateAnimationRule() will call MaybeUpdateCascadeResult(). If we need to update cascade result, UpdateCascadeRestyle() will call EffectCompositor::GetOverriddenProperties(), which uses nsStyleContext to get the rule node, and travere it to find all the !important rules. I fixed this in our OMTA bug, but that is not merged yet, so here we may get an assertion while trying to use nsStyleContext to get the nsRuleNode.
Comment on attachment 8845885 [details]
Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.

https://reviewboard.mozilla.org/r/118966/#review121376

> This is only used by the GeckoRestyleManager, right? So it should stay there and we should just use IsGecko() / AsGecko() to access it?

Sure. I will move it!
Comment on attachment 8845886 [details]
Bug 1343753 - Part 9: Remove assertions from ConsiderInitiatingTransition.

https://reviewboard.mozilla.org/r/118968/#review121374

> It seems odd to remove this in a separate patch. Shouldn't it go with the patch that invalidated this assertion?

OK, I think this should belong to part 7. Thanks.
Comment on attachment 8845887 [details]
Bug 1343753 - Part 10: Make StyleDisplay as the function argument of StyleContextChanged.

https://reviewboard.mozilla.org/r/118970/#review121366

Yes. I should drop this now and put it into the patch which we trigger transitions. Thanks.
Attachment #8845887 - Flags: review?(bbirtles)
Attachment #8845883 - Flags: review?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #29)
> Comment on attachment 8845884 [details]
> Bug 1343753 - Part 7: Use template for UpdateTransitions and
> ConsiderInitiatingTransition.
> 
> https://reviewboard.mozilla.org/r/118964/#review121372
> 
> > I'm curious about this code. I'm not really familiar with the setup in Stylo so can you explain why this is correct -- why we need the parent style for the new style but not the old?
> 
> We need the new parent comptued values while setting keyframes in
> ConsiderInitiatingTransition(); however, no one needs the old parent
> computed value, so this is the reason I make it nullptr.
> 
> However, I just notice that maybe we shouldn't reuse StyleContextChanged for
> stylo. I notice that we create the new StyleContext until
> ServoRestyleManager::ProcessPostTraversal(), so we only have the new
> ServoComputedValues in SequentialTask, which means there is no way to reuse
> StyleContext to get nsStyleContext::HasPseudoElementData(). I might need to
> write another version of StyleContextChanged for stylo.

It sounds like we should work on getting the animation-restyle done first since, without that, it will be hard to test any of this work.

I agree that we probably don't want to use StyleContextChanged. Instead, I think we plan to record the transition start and end points (and any other relevant parameters) during the parallel traversal and then generate the transitions later in a SequentialTask.
Comment on attachment 8845885 [details]
Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.

https://reviewboard.mozilla.org/r/118966/#review121434

Cancelling request on this patch for now since, as per comment 30, we will leave mIsProcessingRestyles in GeckoRestyleManager. I will review it after that change.
Attachment #8845885 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #33)
> It sounds like we should work on getting the animation-restyle done first
> since, without that, it will be hard to test any of this work.
> 
> I agree that we probably don't want to use StyleContextChanged. Instead, I
> think we plan to record the transition start and end points (and any other
> relevant parameters) during the parallel traversal and then generate the
> transitions later in a SequentialTask.

Agree. Let's hold these patches until getting animation-restyle done first (bug 1344966). Thanks for coordinating the steps and priorities.
Comment on attachment 8845884 [details]
Bug 1343753 - Part 7: Use template for UpdateTransitions and ConsiderInitiatingTransition.

clean review request because we may not reuse StyleContextChanged for stylo.
Attachment #8845884 - Flags: review?(bbirtles)
Comment on attachment 8845881 [details]
Bug 1343753 - Part 4: Introduce AnimationValue::IsInterpolableWith.

https://reviewboard.mozilla.org/r/118958/#review121338

> Again, no need for inline here.

Unfortunnately, the detinition of IsInterpolableWith is also in a header file (StyleAnimationValueInlines.h), so we will get link errors without inline keyword (i.e. duplicated definitions). Therefore, I have to keep this inline keyword. I believe AnimationValue will be conciser after we obsolete StyleAnimationValue. Sorry for this messy struct.
Attachment #8845883 - Attachment is obsolete: true
Attachment #8845886 - Attachment is obsolete: true
Attachment #8845887 - Attachment is obsolete: true
Comment on attachment 8845885 [details]
Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.

https://reviewboard.mozilla.org/r/118966/#review121768

::: layout/base/RestyleManagerInlines.h:82
(Diff revision 2)
> +void
> +RestyleManager::IncrementAnimationGeneration()
> +{
> +  // We update the animation generation at start of each call to
> +  // ProcessPendingRestyles so we should ignore any subsequent (redundant)
> +  // calls that occur while we are still processing restyles.
> +  if ((IsGecko() && !AsGecko()->IsProcessingRestyles()) ||
> +      (IsServo() && !mInStyleRefresh)) {
> +    ++mAnimationGeneration;
> +  }
> +}

Why do we put this in RestlyeManagerInlines.h and not just RestyleManager.h?
Comment on attachment 8845885 [details]
Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.

https://reviewboard.mozilla.org/r/118966/#review121768

> Why do we put this in RestlyeManagerInlines.h and not just RestyleManager.h?

`AsGecko()->XXXX` need to know the declartaions of GeckoRestyleManager; however, in RestyleManger.h, we only can use the forward declaraion for GeckoRestyleManager. (We cannot include "GeckoRetyleManger.h" in RestyleManager.h, or there will be many compilation errors.) I think this is why we put all the definitions into RestyleManagerInlines.h, instead of RestyleManager.h directly.
(In reply to Boris Chiou [:boris] from comment #48)
> Comment on attachment 8845885 [details]
> Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.
> 
> https://reviewboard.mozilla.org/r/118966/#review121768
> 
> > Why do we put this in RestlyeManagerInlines.h and not just RestyleManager.h?
> 
> `AsGecko()->XXXX` need to know the declartaions of GeckoRestyleManager;
> however, in RestyleManger.h, we only can use the forward declaraion for
> GeckoRestyleManager. (We cannot include "GeckoRetyleManger.h" in
> RestyleManager.h, or there will be many compilation errors.) I think this is
> why we put all the definitions into RestyleManagerInlines.h, instead of
> RestyleManager.h directly.

Oh I see. It's because we want to make this inline. Otherwise we could just declare it in RestyleManager.h and then define it in RestyleManager.cpp.

I don't think this is so performance critical that we need to make it inline, right? It makes sense for the forwarding methods since we want to make that a zero-cost abstraction, but is it necessary here?
(In reply to Brian Birtles (:birtles) from comment #49)
> Oh I see. It's because we want to make this inline. Otherwise we could just
> declare it in RestyleManager.h and then define it in RestyleManager.cpp.
> 
> I don't think this is so performance critical that we need to make it
> inline, right? It makes sense for the forwarding methods since we want to
> make that a zero-cost abstraction, but is it necessary here?

IncrementAnimationGeneration() is only called by EffectCompositor::RequestRestyle, and only for layer sync, so I think it's fine to move it to cpp file.
Comment on attachment 8845885 [details]
Bug 1343753 - Part 6: Move mAnimationGeneration into RestyleManager.

https://reviewboard.mozilla.org/r/118966/#review122744

::: layout/base/RestyleManagerInlines.h:82
(Diff revision 2)
> +void
> +RestyleManager::IncrementAnimationGeneration()
> +{
> +  // We update the animation generation at start of each call to
> +  // ProcessPendingRestyles so we should ignore any subsequent (redundant)
> +  // calls that occur while we are still processing restyles.
> +  if ((IsGecko() && !AsGecko()->IsProcessingRestyles()) ||
> +      (IsServo() && !mInStyleRefresh)) {
> +    ++mAnimationGeneration;
> +  }
> +}

As discussed in comment 50, let's put this in RestyleManager.{h,cpp}
Attachment #8845885 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #51)
> ::: layout/base/RestyleManagerInlines.h:82
> (Diff revision 2)
> > +void
> > +RestyleManager::IncrementAnimationGeneration()
> > +{
> > +  // We update the animation generation at start of each call to
> > +  // ProcessPendingRestyles so we should ignore any subsequent (redundant)
> > +  // calls that occur while we are still processing restyles.
> > +  if ((IsGecko() && !AsGecko()->IsProcessingRestyles()) ||
> > +      (IsServo() && !mInStyleRefresh)) {
> > +    ++mAnimationGeneration;
> > +  }
> > +}
> 
> As discussed in comment 50, let's put this in RestyleManager.{h,cpp}

Thanks, Brian. I keep part 7 un-reviewed now because we may need to revise it after making sure the design of Bug 1341372.
Comment on attachment 8845879 [details]
Bug 1343753 - Part 2: Update GetTransitionKeyframes for stylo.

https://reviewboard.mozilla.org/r/118954/#review123570
Attachment #8845879 - Flags: review?(manishearth) → review+
Comment on attachment 8845881 [details]
Bug 1343753 - Part 4: Introduce AnimationValue::IsInterpolableWith.

https://reviewboard.mozilla.org/r/118958/#review123612
Attachment #8845881 - Flags: review?(manishearth) → review+
Comment on attachment 8845882 [details]
Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.

https://reviewboard.mozilla.org/r/118960/#review127144

::: servo/components/style/properties/gecko.mako.rs:199
(Diff revision 3)
> +    pub fn extract_animation_value(&self, property: PropertyId) -> Option<AnimationValue> {
> +        match property {
> +            % for prop in data.longhands:
> +                % if prop.animatable:

I did write a similar function for bug 1311257.  In my case I used TransitionProperty instead of PropertyDeclarationId. (And I added the function in AnimationValue, named from_computed_values)

I think this extract_animation_value is called only with with animatable properties because we will not create a SequentialTask of updateing CSS transitions for non-animatable properties.
Comment on attachment 8845882 [details]
Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.

https://reviewboard.mozilla.org/r/118960/#review127144

> I did write a similar function for bug 1311257.  In my case I used TransitionProperty instead of PropertyDeclarationId. (And I added the function in AnimationValue, named from_computed_values)
> 
> I think this extract_animation_value is called only with with animatable properties because we will not create a SequentialTask of updateing CSS transitions for non-animatable properties.

OK, I see. I can reuse your function. It sounds great :)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #62)
> Comment on attachment 8845882 [details]
> Bug 1343753 - Part 5: Support ServoComputedValues in
> ExtractNonDiscreteComputedValue.
> 
> https://reviewboard.mozilla.org/r/118960/#review127144
> 
> ::: servo/components/style/properties/gecko.mako.rs:199
> (Diff revision 3)
> > +    pub fn extract_animation_value(&self, property: PropertyId) -> Option<AnimationValue> {
> > +        match property {
> > +            % for prop in data.longhands:
> > +                % if prop.animatable:
> 
> I did write a similar function for bug 1311257.  In my case I used
> TransitionProperty instead of PropertyDeclarationId. (And I added the
> function in AnimationValue, named from_computed_values)
> 
> I think this extract_animation_value is called only with with animatable
> properties because we will not create a SequentialTask of updateing CSS
> transitions for non-animatable properties.

Ah, I was wrong.  In the case where transition property is 'all', we might call the function.  Where do we filter non-animatable property out?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #64)

> Ah, I was wrong.  In the case where transition property is 'all', we might
> call the function.  Where do we filter non-animatable property out?

We will need to check whether |aProperty| is animatable for *stylo* in ConsiderInitiatingTransition.
Actually stylo does not support discrete type animation yet.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #65)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #64)
> 
> > Ah, I was wrong.  In the case where transition property is 'all', we might
> > call the function.  Where do we filter non-animatable property out?
> 
> We will need to check whether |aProperty| is animatable for *stylo* in
> ConsiderInitiatingTransition.
> Actually stylo does not support discrete type animation yet.

Yes, ExtractNonDiscreteComputedValue() returns false if |aProperty| is not animatable, and then ConsiderInitiatingTransition early returns.
(In reply to Boris Chiou [:boris] from comment #66)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #65)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #64)
> > 
> > > Ah, I was wrong.  In the case where transition property is 'all', we might
> > > call the function.  Where do we filter non-animatable property out?
> > 
> > We will need to check whether |aProperty| is animatable for *stylo* in
> > ConsiderInitiatingTransition.
> > Actually stylo does not support discrete type animation yet.
> 
> Yes, ExtractNonDiscreteComputedValue() returns false if |aProperty| is not
> animatable, and then ConsiderInitiatingTransition early returns.

I meant 'if (nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_None)' check in ConsiderInitiatingTransition(). The check does not match servo's animatable properties at all.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #67)
> I meant 'if (nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_None)'
> check in ConsiderInitiatingTransition(). The check does not match servo's
> animatable properties at all.

Thanks. Noted.
Comment on attachment 8845882 [details]
Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.

https://reviewboard.mozilla.org/r/118960/#review127432
Attachment #8845882 - Flags: review?(manishearth) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #65)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #64)
> 
> > Ah, I was wrong.  In the case where transition property is 'all', we might
> > call the function.  Where do we filter non-animatable property out?
> 
> We will need to check whether |aProperty| is animatable for *stylo* in
> ConsiderInitiatingTransition.
> Actually stylo does not support discrete type animation yet.

Looks like your patch for discrete animation is merged, so this part seems ok now.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #64)
> Ah, I was wrong.  In the case where transition property is 'all', we might
> call the function.  Where do we filter non-animatable property out?

I just double-checked it again, and I think I can reuse your function because we call ExtractNonDiscreteComputedValue() only for longhand properties. Shorthand and "All" are converted into longhand before calling it. Therefore, I will revise my patch to use Servo_ComputedValues_ExtractAnimationValue.
(In reply to Boris Chiou [:boris] from comment #71)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #64)
> > Ah, I was wrong.  In the case where transition property is 'all', we might
> > call the function.  Where do we filter non-animatable property out?
> 
> I just double-checked it again, and I think I can reuse your function
> because we call ExtractNonDiscreteComputedValue() only for longhand
> properties. Shorthand and "All" are converted into longhand before calling
> it. Therefore, I will revise my patch to use
> Servo_ComputedValues_ExtractAnimationValue.

so sad

GECKO(87627) | thread '<unnamed>' panicked at 'Unsupported Servo transition property: eCSSProperty_border_spacing', /Users/boris/projects/firefox/gecko/objdirs/obj-browser-stylo-debug/toolkit/library/x86_64-apple-darwin/debug/build/style-9ccdc1cef0ff3fb2/out/properties.rs:62445

I should revise your function to return Option to avoid the unsupported properties.
(In reply to Boris Chiou [:boris] from comment #72)
> (In reply to Boris Chiou [:boris] from comment #71)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #64)
> > > Ah, I was wrong.  In the case where transition property is 'all', we might
> > > call the function.  Where do we filter non-animatable property out?
> > 
> > I just double-checked it again, and I think I can reuse your function
> > because we call ExtractNonDiscreteComputedValue() only for longhand
> > properties. Shorthand and "All" are converted into longhand before calling
> > it. Therefore, I will revise my patch to use
> > Servo_ComputedValues_ExtractAnimationValue.
> 
> so sad
> 
> GECKO(87627) | thread '<unnamed>' panicked at 'Unsupported Servo transition
> property: eCSSProperty_border_spacing',
> /Users/boris/projects/firefox/gecko/objdirs/obj-browser-stylo-debug/toolkit/
> library/x86_64-apple-darwin/debug/build/style-9ccdc1cef0ff3fb2/out/
> properties.rs:62445
> 
> I should revise your function to return Option to avoid the unsupported
> properties.

That's what I wanted to say in comment 67, we need to check servo's property.animatable flag where we check (nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_None).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #73)
> That's what I wanted to say in comment 67, we need to check servo's
> property.animatable flag where we check
> (nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_None).

OK. I see. I will upload one more patch to revise this line directly. Thanks.
Comment on attachment 8855678 [details]
Bug 1343753 - Part 9: Early return for non-animatable properties for transitions.

https://reviewboard.mozilla.org/r/127526/#review130222

::: servo/components/style/properties/helpers/animated_properties.mako.rs:112
(Diff revision 1)
> +
> +    /// Returns true if this nsCSSPropertyID is one of the animatable properties.
> +    pub fn is_animatable(property: nsCSSPropertyID) -> bool {
> +        match property {
> +            % for prop in data.longhands:
> +                % if prop.animatable:

we need to update this after rebasing.
Attachment #8855678 - Flags: review?(manishearth)
Boris, I think we can check whether the target property can be interpolted or not in needs_update_transitions() in bug 1341372. 
I commented it in bug 1341372 comment 11, so some patches here will be revised.
Ah, OK. I missed the case that a running transition property is changed to non-animatable one.  So even if we check the property is animatable in needs_update_transitions(), we need to check it in C++ too. Sorry for confusing.
Comment on attachment 8845882 [details]
Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.

https://reviewboard.mozilla.org/r/118960/#review130266

::: layout/style/nsTransitionManager.cpp:451
(Diff revision 4)
> +  if (nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_Discrete &&
> +      aProperty != eCSSProperty_visibility) {
> +    return false;
> +  }

Instead we need to check servo's animation_type == "discrete" here.
Attachment #8845882 - Flags: review?(hikezoe) → review+
Comment on attachment 8845882 [details]
Bug 1343753 - Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue.

https://reviewboard.mozilla.org/r/118960/#review130266

> Instead we need to check servo's animation_type == "discrete" here.

OK. I forget to add an FFI to check if it is a discrete type. Will update this after your patches merged into m-c.
Comment on attachment 8855678 [details]
Bug 1343753 - Part 9: Early return for non-animatable properties for transitions.

https://reviewboard.mozilla.org/r/127526/#review130276

::: servo/components/style/properties/helpers/animated_properties.mako.rs:109
(Diff revision 1)
> +    pub fn is_animatable(property: nsCSSPropertyID) -> bool {
> +        match property {
> +            % for prop in data.longhands:
> +                % if prop.animatable:
> +                    ${helpers.to_nscsspropertyid(prop.ident)} => true,
> +                % endif
> +            % endfor
> +            _ => false
> +        }
> +    }

We have already From<nsCSSPropertyID> for TransitionProperty. So we can add a member method of TransitionProperty like this:  

pub fn is_animatable(&self) -> bool;

?

If we do add this is_animatable(nsCSSPropertyID), we should add #[cfg(feature = "gecko")].
Attachment #8855678 - Flags: review?(hikezoe) → review+
Comment on attachment 8855678 [details]
Bug 1343753 - Part 9: Early return for non-animatable properties for transitions.

https://reviewboard.mozilla.org/r/127526/#review130276

> We have already From<nsCSSPropertyID> for TransitionProperty. So we can add a member method of TransitionProperty like this:  
> 
> pub fn is_animatable(&self) -> bool;
> 
> ?
> 
> If we do add this is_animatable(nsCSSPropertyID), we should add #[cfg(feature = "gecko")].

Good idea. Thanks! After rebasing your discrete patches, I will revise the condition as `prop.animation_type == "normal"`. And add a similar function to check if a property is discrete in part 5. e.g.

```
#[cfg(feature = "gecko")]
pub fn is_discrete(&self) -> bool;
```

Thanks for review.
(In reply to Boris Chiou [:boris] from comment #90)
> 
> ```
> #[cfg(feature = "gecko")]
> pub fn is_discrete(&self) -> bool;
> ```

I think is_discrete() is usable for Servo.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #91)
> (In reply to Boris Chiou [:boris] from comment #90)
> > 
> > ```
> > #[cfg(feature = "gecko")]
> > pub fn is_discrete(&self) -> bool;
> > ```
> 
> I think is_discrete() is usable for Servo.

OK, let's remove #[cfg(feature = "gecko")] from is_discrete().
Comment on attachment 8855678 [details]
Bug 1343753 - Part 9: Early return for non-animatable properties for transitions.

https://reviewboard.mozilla.org/r/127526/#review130470
Attachment #8855678 - Flags: review?(manishearth) → review+
Comment on attachment 8855678 [details]
Bug 1343753 - Part 9: Early return for non-animatable properties for transitions.

https://reviewboard.mozilla.org/r/127526/#review130276

> Good idea. Thanks! After rebasing your discrete patches, I will revise the condition as `prop.animation_type == "normal"`. And add a similar function to check if a property is discrete in part 5. e.g.
> 
> ```
> #[cfg(feature = "gecko")]
> pub fn is_discrete(&self) -> bool;
> ```
> 
> Thanks for review.

Hiro, I think this is not what we want. If we convert nsCSSPropertyID into TransitionProperty, this conversion implicitly applies animatable properties because TransitionProperty only supports animatable properties now. I would like to filter out those non-animatable properties *without panic*, so I shouldn't convert nsCSSPropertyID into TransitionPropety. Therefore, I think we should just keep the orignal function sigunature: use nsCSSPropertyID as the parameter and make is_animatable() as a associated function of TransitionProperty. However, is_discrete() is called only if the property is animatable, so it is ok to make it as a method of TransitionProperty. i.e.
```
impl TransitionProperty {
  #[cfg(feature = "gecko")]
  pub fn is_animatable(property: nsCSSPropertyID) -> bool;

  pub fn is_discrete(&self) -> bool;
}
```

Thanks.
(In reply to Boris Chiou [:boris] from comment #94)
> Comment on attachment 8855678 [details]
> Bug 1343753 - Part 9: Early return for non-animatable properties for
> transitions.
> 
> https://reviewboard.mozilla.org/r/127526/#review130276
> 
> > Good idea. Thanks! After rebasing your discrete patches, I will revise the condition as `prop.animation_type == "normal"`. And add a similar function to check if a property is discrete in part 5. e.g.
> > 
> > ```
> > #[cfg(feature = "gecko")]
> > pub fn is_discrete(&self) -> bool;
> > ```
> > 
> > Thanks for review.
> 
> Hiro, I think this is not what we want. If we convert nsCSSPropertyID into
> TransitionProperty, this conversion implicitly applies animatable properties
> because TransitionProperty only supports animatable properties now. I would
> like to filter out those non-animatable properties *without panic*, so I
> shouldn't convert nsCSSPropertyID into TransitionPropety. Therefore, I think
> we should just keep the orignal function sigunature: use nsCSSPropertyID as
> the parameter and make is_animatable() as a associated function of
> TransitionProperty. However, is_discrete() is called only if the property is
> animatable, so it is ok to make it as a method of TransitionProperty. i.e.
> ```
> impl TransitionProperty {
>   #[cfg(feature = "gecko")]
>   pub fn is_animatable(property: nsCSSPropertyID) -> bool;
> 
>   pub fn is_discrete(&self) -> bool;
> }

Ah, indeed. Then TransitionProperty::is_animatable(nsCSSPropertyID) is somewhat misleading.
We should implement it in Servo_Property_IsAnimatable() directly.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #95)
> Ah, indeed. Then TransitionProperty::is_animatable(nsCSSPropertyID) is
> somewhat misleading.
> We should implement it in Servo_Property_IsAnimatable() directly.

If we want to implement it in Servo_Property_IsAnimatable() directly, we cannot use `prop.animatable` to check if it is an animatable proeprty because I think that is only used in xxxx.mako.rs files. An alternative way is to convert the nsCSSPropertyID into PropertyID [1], and then write something like:

impl PropertyID
{
  #[cfg(feature = "gecko")]
  pub fn is_animatable(&self) -> bool {
    ... // gen code by prop.animatable = true.
  }
}

I think this is much better.

[1] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/servo/components/style/properties/properties.mako.rs#797-824
I think the function can be a public function in properties mod (implementation can be written in aniamted_properties.mako.rs, I guess), nscsspropertyid_is_animatable() or something.
Oh, wrong. animated_properties.mako.rs is aniamated_properties mod.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> Oh, wrong. animated_properties.mako.rs is aniamated_properties mod.

OK. Let make is as a mod public function
(In reply to Boris Chiou [:boris] from comment #99)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> > Oh, wrong. animated_properties.mako.rs is aniamated_properties mod.
> 
> OK. Let make is as a mod public function

emilio, do you have any concern to make it a public function in animated_properties.mako.rs?
Flags: needinfo?(emilio+bugs)
Not really, sounds fine to me :)
Flags: needinfo?(emilio+bugs)
Comment on attachment 8845884 [details]
Bug 1343753 - Part 7: Use template for UpdateTransitions and ConsiderInitiatingTransition.

https://reviewboard.mozilla.org/r/118964/#review130712
Attachment #8845884 - Flags: review?(bbirtles) → review+
Comment on attachment 8855677 [details]
Bug 1343753 - Part 8: Rename and add new methods in nsTransitionManager.

https://reviewboard.mozilla.org/r/127524/#review130718

This looks fine but I'd like to understand the answers to the following two questions first.

::: layout/style/ServoBindings.cpp:482
(Diff revision 2)
> +    if (tasks & UpdateAnimationsTasks::CSSTransitions) {
> +      MOZ_ASSERT(aOldComputedValues);
> +      const ServoComputedValuesWithParent oldServoValues =
> +        { aOldComputedValues, nullptr };
> +      presContext->TransitionManager()->
> +        UpdateTransitions(const_cast<dom::Element*>(aElement), pseudoType,
> +                          oldServoValues, servoValues);
> +    }

I wasn't sure about the order these tasks should appear. Is there any reason to do CSS transitions last? If not, maybe it makes more sense to do it before the task where we update effect properties?

::: layout/style/nsTransitionManager.h:423
(Diff revision 2)
>    // Update the transitions. It'd start new, replace, or stop current
>    // transitions if need. aDisp and aElement shouldn't be nullptr.

Can we update this comment at the same time.

// Update transitions. This will start new transitions,
// replace existing transitions, and stop existing transitions
// as needed. aDisp and aElement must be non-null.
// ...

::: servo/components/style/gecko/wrapper.rs:599
(Diff revision 2)
>              Gecko_UpdateAnimations(self.0, atom_ptr,
> +                                   None,
>                                     computed_values_opt,
>                                     parent_values_opt,
>                                     tasks.bits());

So, as I understand it, if |tasks| includes the CSS transitions tasks, then the old computed style should not be None. What ensures that is true? I guess that happens in another bug?
Comment on attachment 8855677 [details]
Bug 1343753 - Part 8: Rename and add new methods in nsTransitionManager.

https://reviewboard.mozilla.org/r/127524/#review130718

I'm sorry to split my patch series into two bugs. The main purpose in this bug is to make sure UpdateTransitions() works for both animations and transitions, so Servo side doen't do anything in this bug.

> I wasn't sure about the order these tasks should appear. Is there any reason to do CSS transitions last? If not, maybe it makes more sense to do it before the task where we update effect properties?

No reason. I can move it before update effect properties.

> So, as I understand it, if |tasks| includes the CSS transitions tasks, then the old computed style should not be None. What ensures that is true? I guess that happens in another bug?

Yes. That is in another bug. In Bug 1343753, the part 5 makes sure this thing. Sorry for split this into two bugs and patches. Right now tasks.bits() doesn't include CSS transitions, so pass None is ok for now. I sent a review request to you for Bug 1343753 part 5 last Friday, but that patch needs update to fix an intermitt failure, so I canceled the review this morning. Sorry for that.
Comment on attachment 8855677 [details]
Bug 1343753 - Part 8: Rename and add new methods in nsTransitionManager.

https://reviewboard.mozilla.org/r/127524/#review130734
Attachment #8855677 - Flags: review?(bbirtles) → review+
Thanks, all. Let's merge this bug.
Attached file Servo PR, #16381 — 

    
    

    
      
  

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a8cea03e8a6
Part 1: Make GetTransitionKeyframes as a local static function. r=birtles
https://hg.mozilla.org/integration/autoland/rev/09e14d740ef9
Part 2: Update GetTransitionKeyframes for stylo. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/66b6fcacc6b4
Part 3: Use AnimationValue in ElementPropertyTransition and CSSTransition. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1f3ed947f5e0
Part 4: Introduce AnimationValue::IsInterpolableWith. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/13a48156ccc1
Part 5: Support ServoComputedValues in ExtractNonDiscreteComputedValue. r=birtles,hiro,manishearth
https://hg.mozilla.org/integration/autoland/rev/bc04bd1c40ab
Part 6: Move mAnimationGeneration into RestyleManager. r=birtles
https://hg.mozilla.org/integration/autoland/rev/5505fd73da2e
Part 7: Use template for UpdateTransitions and ConsiderInitiatingTransition. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3f67f638d08c
Part 8: Rename and add new methods in nsTransitionManager. r=birtles
https://hg.mozilla.org/integration/autoland/rev/946b403ea75b
Part 9: Early return for non-animatable properties for transitions. r=hiro,manishearth

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: