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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
2 months ago
17 days ago

People

(Reporter: hiro, Assigned: boris)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
hiro
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
hiro
: review+
manishearth
: review+
Details | Review
41 bytes, text/x-github-pull-request
Details | Review | Splinter Review
(Reporter)

Description

2 months ago
This is a sibling of bug 1340322.
(Assignee)

Updated

2 months ago
Assignee: nobody → boris.chiou
(Assignee)

Updated

2 months ago
Status: NEW → ASSIGNED
(Assignee)

Updated

2 months ago
Blocks: 1341372
(Assignee)

Comment 1

2 months ago
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.
(Assignee)

Updated

2 months ago
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 months ago
mozreview-review
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.
(Reporter)

Comment 13

2 months ago
(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 15

2 months ago
mozreview-review
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 16

2 months ago
mozreview-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 17

2 months ago
mozreview-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 18

2 months ago
mozreview-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)
(Assignee)

Comment 19

2 months ago
(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.
(Assignee)

Comment 20

2 months ago
(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 21

2 months ago
mozreview-review
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 22

2 months ago
mozreview-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 23

2 months ago
mozreview-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 24

2 months ago
mozreview-review
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 25

2 months ago
mozreview-review
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 26

2 months ago
mozreview-review
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 27

2 months ago
mozreview-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.
(Assignee)

Comment 28

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 29

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 30

2 months ago
mozreview-review-reply
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!
(Assignee)

Comment 31

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 32

2 months ago
mozreview-review-reply
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.
(Assignee)

Updated

2 months ago
Attachment #8845887 - Flags: review?(bbirtles)
(Assignee)

Updated

2 months ago
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 34

2 months ago
mozreview-review
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)
(Assignee)

Comment 35

2 months ago
(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.
(Assignee)

Comment 36

2 months ago
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 hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 39

2 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8845883 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8845886 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8845887 - Attachment is obsolete: true

Comment 47

2 months ago
mozreview-review
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?
(Assignee)

Comment 48

2 months ago
mozreview-review-reply
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?
(Assignee)

Comment 50

2 months ago
(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 51

2 months ago
mozreview-review
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+
(Assignee)

Comment 52

a month ago
(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 53

a month ago
mozreview-review
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 54

a month ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 62

a month ago
mozreview-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.
(Assignee)

Comment 63

a month ago
mozreview-review-reply
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 :)
(Reporter)

Comment 64

a month ago
(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?
(Reporter)

Comment 65

a month ago
(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.
(Assignee)

Comment 66

a month ago
(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.
(Reporter)

Comment 67

a month ago
(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.
(Assignee)

Comment 68

a month ago
(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 69

28 days ago
mozreview-review
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+
(Assignee)

Comment 70

23 days ago
(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.
(Assignee)

Comment 71

23 days ago
(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.
(Assignee)

Comment 72

23 days ago
(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.
(Reporter)

Comment 73

23 days ago
(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).
(Assignee)

Comment 74

23 days ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 84

23 days ago
mozreview-review
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.
(Assignee)

Updated

23 days ago
Attachment #8855678 - Flags: review?(manishearth)
(Reporter)

Comment 85

23 days ago
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.
(Reporter)

Comment 86

23 days ago
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.
(Reporter)

Comment 87

23 days ago
mozreview-review
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+
(Assignee)

Comment 88

23 days ago
mozreview-review-reply
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.
(Reporter)

Comment 89

23 days ago
mozreview-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

::: 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+
(Assignee)

Comment 90

23 days ago
mozreview-review-reply
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.
(Reporter)

Comment 91

23 days ago
(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.
(Assignee)

Comment 92

23 days ago
(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 93

22 days ago
mozreview-review
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+
(Assignee)

Comment 94

22 days ago
mozreview-review-reply
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.
(Reporter)

Comment 95

22 days ago
(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.
(Assignee)

Comment 96

22 days ago
(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
(Reporter)

Comment 97

22 days ago
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.
(Reporter)

Comment 98

22 days ago
Oh, wrong. animated_properties.mako.rs is aniamated_properties mod.
(Assignee)

Comment 99

22 days ago
(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
(Assignee)

Comment 100

22 days ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 111

20 days ago
mozreview-review
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+
Blocks: 1243581

Comment 112

20 days ago
mozreview-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?
(Assignee)

Comment 113

20 days ago
mozreview-review-reply
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 114

20 days ago
mozreview-review
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+
(Assignee)

Comment 115

20 days ago
Thanks, all. Let's merge this bug.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 125

18 days ago
Created attachment 8857434 [details] [review]
Servo PR, #16381
Comment hidden (obsolete)

Comment 127

18 days ago
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

Comment 128

17 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a8cea03e8a6
https://hg.mozilla.org/mozilla-central/rev/09e14d740ef9
https://hg.mozilla.org/mozilla-central/rev/66b6fcacc6b4
https://hg.mozilla.org/mozilla-central/rev/1f3ed947f5e0
https://hg.mozilla.org/mozilla-central/rev/13a48156ccc1
https://hg.mozilla.org/mozilla-central/rev/bc04bd1c40ab
https://hg.mozilla.org/mozilla-central/rev/5505fd73da2e
https://hg.mozilla.org/mozilla-central/rev/3f67f638d08c
https://hg.mozilla.org/mozilla-central/rev/946b403ea75b
Status: ASSIGNED → RESOLVED
Last Resolved: 17 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.