The default bug view has changed. See this FAQ.

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

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
ASSIGNED
21 days ago
5 days ago

People

(Reporter: hiro, Assigned: boris)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

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

Attachments

(7 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+
boris
: review?
manishearth
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
(Reporter)

Description

21 days ago
This is a sibling of bug 1340322.
(Assignee)

Updated

21 days ago
Assignee: nobody → boris.chiou
(Assignee)

Updated

16 days ago
Status: NEW → ASSIGNED
(Assignee)

Updated

14 days ago
Blocks: 1341372
(Assignee)

Comment 1

14 days 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

14 days 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

11 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/#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

11 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 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/#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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days ago
Attachment #8845887 - Flags: review?(bbirtles)
(Assignee)

Updated

10 days 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

10 days 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

10 days 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

10 days 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

10 days 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

10 days ago
Attachment #8845883 - Attachment is obsolete: true
(Assignee)

Updated

10 days ago
Attachment #8845886 - Attachment is obsolete: true
(Assignee)

Updated

10 days ago
Attachment #8845887 - Attachment is obsolete: true

Comment 47

9 days 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

9 days 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

8 days 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

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

6 days 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

5 days 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

5 days 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+
You need to log in before you can comment on or make changes to this bug.