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

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 2

[Boris Chiou [:boris]]

(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. :)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

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 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 1: Drop nsStyleContext argument from KeyframeUtils::GetAnimationPropertiesFromKeyframes.

It's not used at all.

Review commit: https://reviewboard.mozilla.org/r/112308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112308/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

Servo_GetComputedKeyframeValues is almost the same as what we do in
KeyframeUtils::GetComputedKeyframeValues() for Gecko. Unlike the function
for Gecko, this function does not allocate each ComputedKeyframeValues element,
the allocation has been done before calling Servo_GetComputedKeyframeValues.

Review commit: https://reviewboard.mozilla.org/r/112310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112310/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

The reason why we pass 'const ServoComputedStyleValues*' to
GetComputedKeyframeValues instead of a const reference is that we'd like
to check this kind of arguments with MOZ_ASSERT() in various template
functions that will be implemented in subsequent patches. I.e. we will check
MOZ_ASSERT() against both of 'nsStyleContext*' and
'const ServoComputedStyleValues*'.

Review commit: https://reviewboard.mozilla.org/r/112312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112312/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 4: Templatize BuildProperties().

We'd like to use template to manage functions that have almost the same functionality
both on stylo and gecko instead of branching in each functions because it will be
easier to maintain stylo and gecko for a while.

Review commit: https://reviewboard.mozilla.org/r/112314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112314/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 5: Templatize UpdateProperties().

We had to implement UpdateProperties(nsStyleContext*) explicitly
since there are some calls of UpdateProperties() with RefPtr<nsStyleContext>.
Without this compiler tries to use template function instead.

Review commit: https://reviewboard.mozilla.org/r/112316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112316/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 6: Templatize SetKeyframes().

We have SetKeyframes(nsStyleContext*) for the same reason
of UpdateProperties(nsStyleContext*).

Review commit: https://reviewboard.mozilla.org/r/112318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112318/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 7: In case of stylo, use servo computed values instead of nsStyleContext to update properties.

We convert nsStyleContext* to servo's computed values in UpdateProperties()
since it is called from various places (e.g. SetTarget()) with nsStyleContext*.

Review commit: https://reviewboard.mozilla.org/r/112320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112320/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1338927 - Part 8: Drop Servo_AnimationValues_Populate.

Review commit: https://reviewboard.mozilla.org/r/112322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112322/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8837042 [details]
Bug 1338927 - Part 1:  Drop nsStyleContext argument from KeyframeUtils::GetAnimationPropertiesFromKeyframes.

https://reviewboard.mozilla.org/r/112308/#review113944

Comment 14

[Brian Birtles (:birtles)]

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?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

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 16

[Brian Birtles (:birtles)]

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.

Comment 17

[Brian Birtles (:birtles)]

(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.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 19

[Brian Birtles (:birtles)]

(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.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(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 21

[Brian Birtles (:birtles)]

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.

Comment 22

[Brian Birtles (:birtles)]

I've filed an issue on the spec for *not* storing invalid values: https://github.com/w3c/web-animations/issues/181

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(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 24

[Brian Birtles (:birtles)]

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.

Comment 25

[Brian Birtles (:birtles)]

Filed bug 1339693 for dropping invalid values.

Comment 26

[Brian Birtles (:birtles)]

(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?

Comment 27

[Brian Birtles (:birtles)]

(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?

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(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 29

[Brian Birtles (:birtles)]

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.

Comment 30

[Brian Birtles (:birtles)]

(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 31

[Brian Birtles (:birtles)]

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!

Comment 32

[Brian Birtles (:birtles)]

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.

Comment 33

[Brian Birtles (:birtles)]

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.

Comment 34

[Brian Birtles (:birtles)]

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

Comment 35

[Brian Birtles (:birtles)]

Comment on attachment 8837049 [details]
Bug 1338927 - Part 8: Drop Servo_AnimationValues_Populate.

https://reviewboard.mozilla.org/r/112322/#review114014

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837042 [details]
Bug 1338927 - Part 1:  Drop nsStyleContext argument from KeyframeUtils::GetAnimationPropertiesFromKeyframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112308/diff/1-2/

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112310/diff/1-2/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837044 [details]
Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112312/diff/1-2/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112314/diff/1-2/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837046 [details]
Bug 1338927 - Part 5: Templatize UpdateProperties().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112316/diff/1-2/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837047 [details]
Bug 1338927 - Part 6: Templatize SetKeyframes().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112318/diff/1-2/

Comment 42

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112320/diff/1-2/

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837049 [details]
Bug 1338927 - Part 8: Drop Servo_AnimationValues_Populate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112322/diff/1-2/

Comment 44

[Hiroyuki Ikezoe (:hiro)]

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 45

[Brian Birtles (:birtles)]

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?

Comment 46

[Hiroyuki Ikezoe (:hiro)]

(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 47

[Brian Birtles (:birtles)]

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 48

[Brian Birtles (:birtles)]

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.

Comment 49

[Brian Birtles (:birtles)]

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 50

[Brian Birtles (:birtles)]

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);

Comment 51

[Brian Birtles (:birtles)]

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 52

[Brian Birtles (:birtles)]

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);

Comment 53

[Hiroyuki Ikezoe (:hiro)]

(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!

Comment 54

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 55

[Brian Birtles (:birtles)]

(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.

Comment 56

[Hiroyuki Ikezoe (:hiro)]

I meant the static_assert().  Move() works fine. :-) Except in DoUpdateProperties().

Comment 57

[Brian Birtles (:birtles)]

In DoUpdateProperties you should use Forward<StyleType>(aStyle) because there aStyle is a universal reference. In the other call sites it is not.

Comment 58

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112310/diff/2-3/

Comment 59

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837044 [details]
Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112312/diff/2-3/

Comment 60

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112314/diff/2-3/

Comment 61

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837046 [details]
Bug 1338927 - Part 5: Templatize UpdateProperties().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112316/diff/2-3/

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837047 [details]
Bug 1338927 - Part 6: Templatize SetKeyframes().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112318/diff/2-3/

Comment 63

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112320/diff/2-3/

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837049 [details]
Bug 1338927 - Part 8: Drop Servo_AnimationValues_Populate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112322/diff/2-3/

Comment 65

[Manish Goregaokar [:manishearth]]

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.

Comment 66

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 67

[Hiroyuki Ikezoe (:hiro)]

The bindings change conflicts bug 1337229. I am waiting for bug 1337229 first.

Comment 68

[Boris Zbarsky [:bzbarsky]]

When this is fixed, you probably want to try reenabling the 1323114-2.html crashtest in dom/animation/test/crashtests/crashtests.list

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837042 [details]
Bug 1338927 - Part 1:  Drop nsStyleContext argument from KeyframeUtils::GetAnimationPropertiesFromKeyframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112308/diff/2-3/

Comment 70

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837043 [details]
Bug 1338927 - Part 2: Generate ComputedKeyframeValues array from servo's computed values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112310/diff/3-4/

Comment 71

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837044 [details]
Bug 1338927 - Part 3: Introduce ServoComputedStyleValues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112312/diff/3-4/

Comment 72

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112314/diff/3-4/

Comment 73

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837046 [details]
Bug 1338927 - Part 5: Templatize UpdateProperties().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112316/diff/3-4/

Comment 74

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837047 [details]
Bug 1338927 - Part 6: Templatize SetKeyframes().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112318/diff/3-4/

Comment 75

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112320/diff/3-4/

Comment 76

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837049 [details]
Bug 1338927 - Part 8: Drop Servo_AnimationValues_Populate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112322/diff/3-4/

Comment 77

[Hiroyuki Ikezoe (:hiro)]

Stripped off servo side changes.
I will send a PR for the servo side change once bug 1337229 has been landed.

Comment 78

[Hiroyuki Ikezoe (:hiro)]

Oops, I missed bz's comment. Thank you bz!, I will fix it later.

Comment 79

[Hiroyuki Ikezoe (:hiro)]

https://github.com/servo/servo/pull/15722

Comment 80

[Pulsebot]

Pushed by [e-mail]:
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

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ebe916f390d2
https://hg.mozilla.org/mozilla-central/rev/bee36c82a848
https://hg.mozilla.org/mozilla-central/rev/94a1d81cae94
https://hg.mozilla.org/mozilla-central/rev/c0dae3f87e94
https://hg.mozilla.org/mozilla-central/rev/0e480e154fee
https://hg.mozilla.org/mozilla-central/rev/3f96e96e16c8
https://hg.mozilla.org/mozilla-central/rev/e1584055718c
https://hg.mozilla.org/mozilla-central/rev/ab163e43086d