Closed Bug 1338927 Opened 5 years ago Closed 5 years ago

stylo: Generate nsTArray<ComputedKeyframeValues> from servo's computed styles

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(8 files)

We are currently using nsStyleContext in KeyframeUtils::GetComputedKeyframeValues().  But once we fix bug 1338087,  we can do the same thing with servo's computed styles.  Also in case of CSS animations we should build keyframes and AnimationProperty(s) without nsStyleContext.
To avoid duplicate work what Boris is going to do in bug 1338087 I am putting a try link what I am doing here in this bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9c5397a47fe511fb534b1090705b73b2c6ca18a

I'd want to still add another variant of KeyframeEffectReadOnly::SetKeyframes() that requires servo's computed values instead of nsStyleContext. I've not yet decided we should use template or an abstract struct that wraps nsStyleContext and the servo's computed values.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> To avoid duplicate work what Boris is going to do in bug 1338087 I am
> putting a try link what I am doing here in this bug.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c9c5397a47fe511fb534b1090705b73b2c6ca18a
> 
> I'd want to still add another variant of
> KeyframeEffectReadOnly::SetKeyframes() that requires servo's computed values
> instead of nsStyleContext. I've not yet decided we should use template or an
> abstract struct that wraps nsStyleContext and the servo's computed values.

Great. In bug 1338087, I just remove the ComputeValues (on servo-backend branch) in GetComputedKeyframeValues(), and try to append elements of PropertyStyleAnimationValue from Servo_AnimationValues_Populate. Your solution is better I think. I will update my patch for reviewing soon. Thanks for the try info. :)
I've decided to use template to avoid maintain various branches in some functions. I believe it will reduce our future works both on Gecko and Stylo even if it will somewhat increase our code and binary size.
Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

https://reviewboard.mozilla.org/r/112310/#review113620

::: servo/ports/geckolib/glue.rs:389
(Diff revision 1)
> +                    // We have to initialize StyleAnimationValue with eUnit_Null,
> +                    // otherwise the mGecko keeps uninitialized state and then
> +                    // StyleAnimationValue::operator== fails.
> +                    animation_values[i].mValue.mGecko.mUnit = StyleAnimationValue_Unit::eUnit_Null;

I will drop this tweak because Shing is working on modification of AnimationValue::operator== in bug 1337229.  With the the modification we don't need to worry about StyleAnimationValue::operator== here.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8837042 [details]
Bug 1338927 - Part 1:  Drop nsStyleContext argument from KeyframeUtils::GetAnimationPropertiesFromKeyframes.

https://reviewboard.mozilla.org/r/112308/#review113944
Attachment #8837042 - Flags: review?(bbirtles) → review+
Comment on attachment 8837044 [details]
Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

https://reviewboard.mozilla.org/r/112312/#review113950

Can't we just use mozilla::IsPointer<T>::value as needed in subsequent patches?
Could you please explain it in detail?  You mean we can replace MOZ_ASSERT() with MOZ_ASSERT_IF(IsPointer<>,) something like this:

MOZ_ASSERT_IF(mozilla::IsPointer<StyleType>::value, aStyleType);

?

I did not know IsPointer<>, but yes we can do this.

But there is still a place that needs to be passed const ServoComputedStylesValues*, it's SetKeyframesInternal(). It calls UpdateProperties() only if we have nsStyleContext* or the servo's computed values.

Or, totally different things?
Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

https://reviewboard.mozilla.org/r/112310/#review113948

::: dom/animation/KeyframeUtils.cpp:22
(Diff revision 1)
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/dom/KeyframeEffectBinding.h"
>  #include "mozilla/dom/KeyframeEffectReadOnly.h" // For PropertyValuesPair etc.
>  #include "jsapi.h" // For ForOfIterator etc.
>  #include "nsClassHashtable.h"
> +#include "nsContentUtils.h" // For GetContextForContent.

Nit: No need to trailing .

(we only have above because it is part of 'etc.')

::: dom/animation/KeyframeUtils.cpp:604
(Diff revision 1)
> +  nsPresContext* presContext = nsContentUtils::GetContextForContent(aElement);
> +  MOZ_ASSERT(presContext);

I think this is probably right, but why are we sure presContext will always be non-null? Couldn't this potentially be called for an element without a pres context? (via JS using GetProperties()?)

::: dom/animation/KeyframeUtils.cpp:610
(Diff revision 1)
> +  const size_t len = aKeyframes.Length();
> +  for (size_t i = 0; i < len; i++) {
> +    Unused << result.AppendElement();
> +  }

Can't we just do:

  result.AppendElements(aKeyframes.Length());

Also, I don't think we have a MOZ_MUST_USE annotation on the infallible version of AppendElements that nsTArray exposes.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> But there is still a place that needs to be passed const
> ServoComputedStylesValues*, it's SetKeyframesInternal(). It calls
> UpdateProperties() only if we have nsStyleContext* or the servo's computed
> values.

Ok, I was just reading the commit message which said we were only doing this for use with MOZ_ASSERT. I haven't got SetKeyframesInternal yet so I'll have to see if it makes sense to use Maybe<> there or to just use a pointer.
(In reply to Brian Birtles (:birtles) from comment #16)
> ::: dom/animation/KeyframeUtils.cpp:604
> (Diff revision 1)
> > +  nsPresContext* presContext = nsContentUtils::GetContextForContent(aElement);
> > +  MOZ_ASSERT(presContext);
> 
> I think this is probably right, but why are we sure presContext will always
> be non-null? Couldn't this potentially be called for an element without a
> pres context? (via JS using GetProperties()?)

Ah, indeed. It seems to me that this call will fail in such case. Interesting, in Gecko KeyframeEffectReadOnly::GetTargetStyleContext() will fail in such case, that means when we call this function we can get a valid pres shell there at this moment.

In a later bug, I will fix to get a pres shell from OwnerDoc() if we fail to get the pres shell from composed doc both on Gecko and Stylo.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> In a later bug, I will fix to get a pres shell from OwnerDoc() if we fail to
> get the pres shell from composed doc both on Gecko and Stylo.

Oh, I think the case I'm thinking of that might not work either. I was just thinking we should return an empty array in that case.
(In reply to Brian Birtles (:birtles) from comment #19)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> > In a later bug, I will fix to get a pres shell from OwnerDoc() if we fail to
> > get the pres shell from composed doc both on Gecko and Stylo.
> 
> Oh, I think the case I'm thinking of that might not work either. I was just
> thinking we should return an empty array in that case.

Just to be clear, even in case of stylo, the pres shell is never null in this function, because for script animation we get servo's computed values from nsStyleContext.  Also I am going to use this function for CSS animations, but in that case we don't call UpdateAnimations at all I think.  (Do we update CSS animations if target element has no associated document?)

What I will do in the later bug is to make GetProperties() returning plausible results, I am not sure it's worth doing though.
Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

https://reviewboard.mozilla.org/r/112310/#review113968

Looks good to me but I wonder how we handle invalid values in light of this.

(Storing invalid values was mostly important for pacing to ensure that we get the right pacing even if one of the values wasn't understood by the implementation. Now that we are going to drop pacing, I wonder if we need to store invalid values or if we should change the spec.)

::: dom/animation/KeyframeUtils.cpp:609
(Diff revision 1)
> +  nsPresContext* presContext = nsContentUtils::GetContextForContent(aElement);
> +  MOZ_ASSERT(presContext);
> +
> +  nsTArray<ComputedKeyframeValues> result(aKeyframes.Length());
> +
> +  // Allocate each nsTArray<PropertyStyleAnimationValuePair> here.

(I guess technically this should be 'Initialize' or 'Construct'? We already allocated the memory for the outer array in the ctor above and now we're just initializing each the pre-allocated memory with the default ctor for nsTArray which doesn't allocate any additional memory.)

::: servo/ports/geckolib/glue.rs:349
(Diff revision 1)
> +        inherited_style: parent_style.unwrap_or(&init),
> +        style: (**style).clone(),
> +        font_metrics_provider: None,
> +    };
> +
> +    unsafe { computed_keyframes.set_len(keyframes.len() as u32); }

(I'm curious why we need to do this. Don't our FFI bindings preserve the length of the array? I thought we already resized it and initialized it on the Gecko side?)

::: servo/ports/geckolib/glue.rs:356
(Diff revision 1)
> +        let iter = keyframe.mPropertyValues.iter()
> +                                           .filter(|&property| !property.mServoDeclarationBlock.mRawPtr.is_null());

We should probably add a comment here mentioning that mServoDeclarationBlock will be null when we have an invalid property.

... and don't we need the StyleAnimationValue in that case to represent the invalid value?

In KeyframeEffectReadOnly::GetKeyframes we serialize the StyleAnimationValue when we have an invalid value even if the backend is Servo.
Attachment #8837043 - Flags: review?(bbirtles)
I've filed an issue on the spec for *not* storing invalid values: https://github.com/w3c/web-animations/issues/181
(In reply to Brian Birtles (:birtles) from comment #21)
> ::: servo/ports/geckolib/glue.rs:356
> (Diff revision 1)
> > +        let iter = keyframe.mPropertyValues.iter()
> > +                                           .filter(|&property| !property.mServoDeclarationBlock.mRawPtr.is_null());
> 
> We should probably add a comment here mentioning that mServoDeclarationBlock
> will be null when we have an invalid property.
> 
> ... and don't we need the StyleAnimationValue in that case to represent the
> invalid value?
> 
> In KeyframeEffectReadOnly::GetKeyframes we serialize the StyleAnimationValue
> when we have an invalid value even if the backend is Servo.

Uho!  I did not know that.  But I did now confirm that we store the invalid values in servo's declaration block.
Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

https://reviewboard.mozilla.org/r/112310/#review113996

Changing this to r+ since it looks like we will be able to drop storing invalid keyframe values and then my only blocking concern with this patch will be resolved.
Attachment #8837043 - Flags: review+
Filed bug 1339693 for dropping invalid values.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> Uho!  I did not know that.  But I did now confirm that we store the invalid
> values in servo's declaration block.

Oh, really. I'm pretty sure we didn't use to do that. Can you make it *not* store them?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> But there is still a place that needs to be passed const
> ServoComputedStylesValues*, it's SetKeyframesInternal(). It calls
> UpdateProperties() only if we have nsStyleContext* or the servo's computed
> values.

We could just do the same thing there, I guess

if (!IsPointerType<StyleType>::value || aStyle) {
  ...
}

That seems better to me but do you foresee other problems?
(In reply to Brian Birtles (:birtles) from comment #26)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > Uho!  I did not know that.  But I did now confirm that we store the invalid
> > values in servo's declaration block.
> 
> Oh, really. I'm pretty sure we didn't use to do that. Can you make it *not*
> store them?

I think it's parser side, we can do it in bug 1339693.
Comment on attachment 8837044 [details]
Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

https://reviewboard.mozilla.org/r/112312/#review114006

Clearing this review request for now since it seems like we could use a const ref for ServoComputedStyleValues and that that would be (a) closer to what we want to use long-term once StyleAnimationValue goes away, (b) more robust (i.e. more static checking, less dynamic checking).

Let me know if this proves problematic however.
Attachment #8837044 - Flags: review?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> (In reply to Brian Birtles (:birtles) from comment #26)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > > Uho!  I did not know that.  But I did now confirm that we store the invalid
> > > values in servo's declaration block.
> > 
> > Oh, really. I'm pretty sure we didn't use to do that. Can you make it *not*
> > store them?
> 
> I think it's parser side, we can do it in bug 1339693.

Ok, great. Thanks.
Comment on attachment 8837045 [details]
Bug 1338927 - Part 4: Templatize BuildProperties().

https://reviewboard.mozilla.org/r/112314/#review114004

::: dom/animation/KeyframeEffectReadOnly.cpp:858
(Diff revision 1)
> +template<typename StyleType>
>  nsTArray<AnimationProperty>
> -KeyframeEffectReadOnly::BuildProperties(nsStyleContext* aStyleContext)
> +KeyframeEffectReadOnly::BuildProperties(StyleType aStyle)
>  {
> -  MOZ_ASSERT(aStyleContext);
> +  static_assert(IsSame<StyleType, nsStyleContext*>::value ||
> +                IsSame<StyleType, const ServoComputedStyleValues*>::value,
> +                "StyleType should be nsStyleContext* or "
> +                "const ServoStyleComputedValues*");

(I was wondering if we could use MaybeOneOf<> for these wrapper functions so they don't need to be templatized and we don't need to do these checks.

e.g. T1 = nsStyleContext* (only set when non-null)
     T2 = ServoComputedStyleValues

However, having looked into it, I think it would end up being a bit messier, although it would save a little in code size.)

::: dom/animation/KeyframeUtils.cpp:592
(Diff revision 1)
>  KeyframeUtils::ApplyDistributeSpacing(nsTArray<Keyframe>& aKeyframes)
>  {
>    nsTArray<ComputedKeyframeValues> emptyArray;
>    ApplySpacing(aKeyframes, SpacingMode::distribute, eCSSProperty_UNKNOWN,
> -               emptyArray, nullptr);
> +                           // We need static_cast here since there is another
> +                           // variant of ApplySpacing() that requeires

s/requeires/requires/

but actually I think this should be 'takes'

e.g.

... there is another variant of ApplySpacing() that takes a pointer to ServoComputedStyleValues as the last argument.

That said, if we switch to using a const ref, I guess that goes away. And it also goes away once we drop spacing!
Attachment #8837045 - Flags: review?(bbirtles) → review+
Comment on attachment 8837046 [details]
Bug 1338927 - Part 5: Templatize UpdateProperties().

https://reviewboard.mozilla.org/r/112316/#review114010

This looks good to me but I'd like to see what you decide about the naming / whether we can drop the two wrapper methods altogether.

::: dom/animation/KeyframeEffectReadOnly.h:435
(Diff revision 1)
> +  template<typename StyleType>
> +  void UpdatePropertiesInternal(StyleType aStyle);
> +

Can we just make this public, rename it to UpdateProperties, and, if necessary, use explicit instantiation for the two concrete version in the .cpp file?

I don't love the *Internal naming. If we have to have this, I think Do*, i.e. DoUpdateProperties, might be a little bit more common in our codebase.
Attachment #8837046 - Flags: review?(bbirtles)
Comment on attachment 8837047 [details]
Bug 1338927 - Part 6: Templatize SetKeyframes().

https://reviewboard.mozilla.org/r/112318/#review113988

Commit message: s/SetKeyframes/SetKeyframes/

As with part 5, if possible I'd prefer to drop the two wrapper methods. If that proves awkward, then I think DoSetKeyframes() might be a little more consistent with what we do elsewhere.
Attachment #8837047 - Flags: review?(bbirtles) → review+
Comment on attachment 8837048 [details]
Bug 1338927 - Part 7: In case of stylo, use servo computed values instead of nsStyleContext to update properties.

https://reviewboard.mozilla.org/r/112320/#review114012
Attachment #8837048 - Flags: review?(bbirtles) → review+
Comment on attachment 8837049 [details]
Bug 1338927 - Part 8: Drop Servo_AnimationValues_Populate.

https://reviewboard.mozilla.org/r/112322/#review114014
Attachment #8837049 - Flags: review?(bbirtles) → review+
Comment on attachment 8837046 [details]
Bug 1338927 - Part 5: Templatize UpdateProperties().

https://reviewboard.mozilla.org/r/112316/#review114010

> Can we just make this public, rename it to UpdateProperties, and, if necessary, use explicit instantiation for the two concrete version in the .cpp file?
> 
> I don't love the *Internal naming. If we have to have this, I think Do*, i.e. DoUpdateProperties, might be a little bit more common in our codebase.

I gave up using the template with RefPtr<nsStyleContext> for now. I have no idea other than calling .get() explicitly.
Comment on attachment 8837044 [details]
Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

https://reviewboard.mozilla.org/r/112312/#review114366

::: dom/animation/KeyframeEffectReadOnly.h:137
(Diff revision 2)
>  
> +struct ServoComputedStyleValues
> +{
> +  const ServoComputedValues* mCurrentStyle;
> +  const ServoComputedValues* mParentStyle;
> +  explicit operator bool() const { return true; }

I don't understand why we need this. It seems in parts 5 and 6 we we use IsPointer to obviate the need for this. Can't we do the same thing in part 4?
(In reply to Brian Birtles (:birtles) from comment #45)
> Comment on attachment 8837044 [details]
> Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.
> 
> https://reviewboard.mozilla.org/r/112312/#review114366
> 
> ::: dom/animation/KeyframeEffectReadOnly.h:137
> (Diff revision 2)
> >  
> > +struct ServoComputedStyleValues
> > +{
> > +  const ServoComputedValues* mCurrentStyle;
> > +  const ServoComputedValues* mParentStyle;
> > +  explicit operator bool() const { return true; }
> 
> I don't understand why we need this. It seems in parts 5 and 6 we we use
> IsPointer to obviate the need for this. Can't we do the same thing in part 4?

Unfortunately IsPointer<> did not solve it.  'if (IsPointer<StyleType>::value && aStyleType)' still needs it. As far as I can tell we need the operator if we do check the const ref in MOZ_ASSERT() or if () block. If we had IsPointerIf<> then we don't need the operator.
Oh gosh! Now I realized the 'if (IsPointer<StyleType>::value && aStyleType)' is totally wrong. I will fix it.
Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

https://reviewboard.mozilla.org/r/112310/#review114414

::: dom/animation/KeyframeUtils.h:18
(Diff revision 2)
>  
>  struct JSContext;
>  class JSObject;
>  class nsIDocument;
>  class nsStyleContext;
> +class ServoComputedValues;

This should be struct, not class.
Comment on attachment 8837044 [details]
Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

https://reviewboard.mozilla.org/r/112312/#review114416

::: dom/animation/KeyframeEffectReadOnly.h:137
(Diff revision 2)
>  
> +struct ServoComputedStyleValues
> +{
> +  const ServoComputedValues* mCurrentStyle;
> +  const ServoComputedValues* mParentStyle;
> +  explicit operator bool() const { return true; }

Alright, seems ok to me. Probably better than any more messing around with templates to avoid calling this line when IsPointer is false.

Alternatively, we could just pass a const Maybe<ServoComputedStyleValues>& around since that has the same 'operator*' and 'operator bool' behavior as a pointer. Either way seems fine to me.
Attachment #8837044 - Flags: review?(bbirtles) → review+
Comment on attachment 8837045 [details]
Bug 1338927 - Part 4: Templatize BuildProperties().

https://reviewboard.mozilla.org/r/112314/#review114418

::: dom/animation/KeyframeEffectReadOnly.cpp:288
(Diff revision 2)
> -  nsTArray<AnimationProperty> properties = BuildProperties(aStyleContext);
> +  nsTArray<AnimationProperty> properties =
> +    BuildProperties(Forward<nsStyleContext*>(aStyleContext));

This seems a bit awkward. I guess this is because if we pass an lvalue to an universal reference we end up with nsStyleContext*&.

In that case, why don't we just explicitly indicate we want an rvalue here:

  DoSetKeyframes(Move(aKeyframes), Move(aStyleContext));

That seems clearer?

Effectively, that is what Forward is doing.

If we have:

  template<class T> int func(T&&);

And we do:

  int i;
  int n = func(i);

We will call func<int&>(int&)

If we do:

  int n = func(0);

We will call f<int>(int&&)

So when we use Forward or Move, we end up calling BuildProperties<nsStyleContext*>(nsStyleContext*&&).

Alternatively, we could just say a reference to a pointer is fine and update our assertions to something like:

  static_assert(
    IsSame<RemoveReference<StyleType>::Type, nsStyleContext*>::value ||
    IsSame<StyleType, const ServoComputedStyleValues&>::value,
    "StyleType should be nsStyleContext* or "
    "const ServoComputedStyleValues&");

Or even just:

  static_assert(
    IsSame<StyleType, nsStyleContext*&>::value ||
    IsSame<StyleType, const ServoComputedStyleValues&>::value,
    "StyleType should be nsStyleContext* or "
    "const ServoComputedStyleValues&");

Maybe that pointer setup you had to begin with was better ;)
Comment on attachment 8837046 [details]
Bug 1338927 - Part 5: Templatize UpdateProperties().

https://reviewboard.mozilla.org/r/112316/#review114424

::: dom/animation/KeyframeEffectReadOnly.cpp:284
(Diff revision 2)
> +
> +void
> +KeyframeEffectReadOnly::UpdateProperties(
> +  const ServoComputedStyleValues& aServoValues)
> +{
> +  DoUpdateProperties(Forward<const ServoComputedStyleValues&>(aServoValues));

Again, based on what we do with part 4, this should probably be either:

  DoUpdateProperties(Move(aStyleContext));

or just:

  DoUpdateProperties(aStyleContext);
Attachment #8837046 - Flags: review?(bbirtles) → review+
Comment on attachment 8837047 [details]
Bug 1338927 - Part 6: Templatize SetKeyframes().

https://reviewboard.mozilla.org/r/112318/#review114420

::: dom/animation/KeyframeEffectReadOnly.cpp:194
(Diff revision 2)
>  
>  void
>  KeyframeEffectReadOnly::SetKeyframes(nsTArray<Keyframe>&& aKeyframes,
>                                       nsStyleContext* aStyleContext)
>  {
> +  DoSetKeyframes(Move(aKeyframes), Forward<nsStyleContext*>(aStyleContext));

Depending on which direction we decide to go with part 4, this should just be either:

  DoSetKeyframes(Move(aKeyframes), Move(aStyleContext));

or:

  DoSetKeyframes(Move(aKeyframes), aStyleContext);

::: dom/animation/KeyframeEffectReadOnly.cpp:202
(Diff revision 2)
> +  DoSetKeyframes(Move(aKeyframes),
> +                 Forward<const ServoComputedStyleValues&>(aServoValues));

In either case this should just be:

  DoSetKeyframes(Move(aKeyframes), aServoValues);
Comment on attachment 8837048 [details]
Bug 1338927 - Part 7: In case of stylo, use servo computed values instead of nsStyleContext to update properties.

https://reviewboard.mozilla.org/r/112320/#review114422

::: dom/animation/KeyframeEffectReadOnly.cpp:314
(Diff revision 2)
> +    aStyleContext->GetParent()
> +      ? aStyleContext->GetParent()->StyleSource().AsServoComputedValues()
> +      : nullptr;
> +
> +  const ServoComputedStyleValues servoValues = { currentStyle, parentStyle };
> +  DoUpdateProperties(Forward<const ServoComputedStyleValues&>(servoValues));

Again, this should just be DoUpdateProperties(servoValues);
(In reply to Brian Birtles (:birtles) from comment #49)

>   static_assert(
>     IsSame<StyleType, nsStyleContext*&>::value ||
>     IsSame<StyleType, const ServoComputedStyleValues&>::value,
>     "StyleType should be nsStyleContext* or "
>     "const ServoComputedStyleValues&");
> 
> Maybe that pointer setup you had to begin with was better ;)

The latter can not be used in this case since there needs a null check against |aStyleType| in the function.  I will use Move(). Thanks!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #53)
> (In reply to Brian Birtles (:birtles) from comment #49)
> 
> >   static_assert(
> >     IsSame<StyleType, nsStyleContext*&>::value ||
> >     IsSame<StyleType, const ServoComputedStyleValues&>::value,
> >     "StyleType should be nsStyleContext* or "
> >     "const ServoComputedStyleValues&");
> > 
> > Maybe that pointer setup you had to begin with was better ;)
> 
> The latter can not be used in this case since there needs a null check
> against |aStyleType| in the function.  I will use Move(). Thanks!

Sorry. Actually neither can be used.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #53)
> > (In reply to Brian Birtles (:birtles) from comment #49)
> > 
> > >   static_assert(
> > >     IsSame<StyleType, nsStyleContext*&>::value ||
> > >     IsSame<StyleType, const ServoComputedStyleValues&>::value,
> > >     "StyleType should be nsStyleContext* or "
> > >     "const ServoComputedStyleValues&");
> > > 
> > > Maybe that pointer setup you had to begin with was better ;)
> > 
> > The latter can not be used in this case since there needs a null check
> > against |aStyleType| in the function.  I will use Move(). Thanks!
> 
> Sorry. Actually neither can be used.

I don't quite follow. Move() works for me. What error are you seeing? And your original patches with a pointer to the struct worked.
I meant the static_assert().  Move() works fine. :-) Except in DoUpdateProperties().
In DoUpdateProperties you should use Forward<StyleType>(aStyle) because there aStyle is a universal reference. In the other call sites it is not.
Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

https://reviewboard.mozilla.org/r/112310/#review114620

::: servo/components/style/build_gecko.rs:615
(Diff revision 3)
>          ];
>          let servo_borrow_types = [
>              "nsCSSValue",
>              "RawGeckoAnimationValueList",
>              "RawGeckoKeyframeList",
> +            "RawGeckoComputedKeyframeValuesList",

This will change the generated bindings, please copy the new generated bindings from the objdir (`$objdir/toolkit/library/rust/$target/debug/build/style-something/out/gecko/*` to `servo/components/style/gecko_bindings`). Preferably do a regen before these patches and make a commit containing the bindings changes which you don't push up, and then do a second regen+copy within this commit.

::: servo/ports/geckolib/glue.rs:357
(Diff revision 3)
> +
> +        // mServoDeclarationBlock is null in the case where we have an invalid css property.
> +        let iter = keyframe.mPropertyValues.iter()
> +                                           .filter(|&property| !property.mServoDeclarationBlock.mRawPtr.is_null());
> +        for property in iter {
> +            let declarations = unsafe { &&*property.mServoDeclarationBlock.mRawPtr.clone() };

Is the double `&` necessary here?

::: servo/ports/geckolib/glue.rs:380
(Diff revision 3)
> +                                    }
> +                                } else {
> +                                    None
> +                                }
> +                            });
> +

Might want to call ensure_capacity on `animation_values` first?)

::: servo/ports/geckolib/glue.rs:383
(Diff revision 3)
> +                                }
> +                            });
> +
> +            for (i, anim) in anim_iter.enumerate() {
> +                if !properties_on_this_keyframe.contains(&anim.0) {
> +                    unsafe { animation_values.set_len((i + 1) as u32) };

Leave a comment that this is safe because we immediately write to the uninitialized values.
Attachment #8837043 - Flags: review?(manishearth) → review+
Thank you Manish!

(In reply to Manish Goregaokar [:manishearth] from comment #65)
> ::: servo/components/style/build_gecko.rs:615
> (Diff revision 3)
> >          ];
> >          let servo_borrow_types = [
> >              "nsCSSValue",
> >              "RawGeckoAnimationValueList",
> >              "RawGeckoKeyframeList",
> > +            "RawGeckoComputedKeyframeValuesList",
> 
> This will change the generated bindings, please copy the new generated
> bindings from the objdir
> (`$objdir/toolkit/library/rust/$target/debug/build/style-something/out/gecko/
> *` to `servo/components/style/gecko_bindings`). Preferably do a regen before
> these patches and make a commit containing the bindings changes which you
> don't push up, and then do a second regen+copy within this commit.

Thanks.  I am hoping the bindings change will not conflict bug 1337229, there is a patch for bug 1337229 in my MQ.

> ::: servo/ports/geckolib/glue.rs:357
> (Diff revision 3)
> > +
> > +        // mServoDeclarationBlock is null in the case where we have an invalid css property.
> > +        let iter = keyframe.mPropertyValues.iter()
> > +                                           .filter(|&property| !property.mServoDeclarationBlock.mRawPtr.is_null());
> > +        for property in iter {
> > +            let declarations = unsafe { &&*property.mServoDeclarationBlock.mRawPtr.clone() };
> 
> Is the double `&` necessary here?
> 
> ::: servo/ports/geckolib/glue.rs:380
> (Diff revision 3)
> > +                                    }
> > +                                } else {
> > +                                    None
> > +                                }
> > +                            });
> > +
> 
> Might want to call ensure_capacity on `animation_values` first?)

In my understandings, to use ensure_capacity here we need to use count() against anim_iter(), as a result the anim_iter() is consumed.  I am not sure it's a preferable way.
Depends on: 1337229
The bindings change conflicts bug 1337229. I am waiting for bug 1337229 first.
Blocks: 1340322
Blocks: 1334036
When this is fixed, you probably want to try reenabling the 1323114-2.html crashtest in dom/animation/test/crashtests/crashtests.list
Stripped off servo side changes.
I will send a PR for the servo side change once bug 1337229 has been landed.
Oops, I missed bz's comment. Thank you bz!, I will fix it later.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebe916f390d2
Part 1:  Drop nsStyleContext argument from KeyframeUtils::GetAnimationPropertiesFromKeyframes. r=birtles
https://hg.mozilla.org/integration/autoland/rev/bee36c82a848
Part 2: Generate ComputedKeyframeValues array from servo's computed values. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/94a1d81cae94
Part 3: Introduce ServoComputedStyleValues. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c0dae3f87e94
Part 4: Templatize BuildProperties(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/0e480e154fee
Part 5: Templatize UpdateProperties(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/3f96e96e16c8
Part 6: Templatize SetKeyframes(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/e1584055718c
Part 7: In case of stylo, use servo computed values instead of nsStyleContext to update properties. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ab163e43086d
Part 8: Drop Servo_AnimationValues_Populate. r=birtles
You need to log in before you can comment on or make changes to this bug.