Closed Bug 1337313 Opened 5 years ago Closed 5 years ago

stylo: Use RawServoAnimationValue for KeyframeEffectReadOnly::GetProperties()

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(4 files, 1 obsolete file)

According to Bug 1335942 Comment 18, we still need to use RawServoAnimationValue for KeyframeEffectReadOnly::GetProperties(), so we don't need to store StyleAnimationValue on stylo anymore.
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
See Also: → 1337735
Comment on attachment 8835271 [details]
Bug 1337313 - Part 1: Use AnimationValue in CreatePropertyValue.

https://reviewboard.mozilla.org/r/110972/#review112234
Attachment #8835271 - Flags: review?(hikezoe) → review+
Comment on attachment 8835272 [details]
Bug 1337313 - Part 2: Add AnimationValue::SerializeSpecifiedValue.

https://reviewboard.mozilla.org/r/110974/#review112236

::: layout/style/StyleAnimationValue.cpp:5239
(Diff revision 1)
> +  if (mServo) {
> +    nsTArray<const RawServoAnimationValue*> valueArray { mServo };
> +    RefPtr<RawServoDeclarationBlock> declaration =
> +      Servo_AnimationValues_Uncompute(&valueArray).Consume();
> +    MOZ_ASSERT(declaration, "failed to uncompute RawServoAnimationValue");
> +    Servo_DeclarationBlock_SerializeOneValue(declaration, aProperty, &aString);

This looks to me that it's a bit wasted. I wonder we can add an FFI to convert single AnimationValue to String.
Comment on attachment 8835272 [details]
Bug 1337313 - Part 2: Add AnimationValue::SerializeSpecifiedValue.

https://reviewboard.mozilla.org/r/110974/#review112236

> This looks to me that it's a bit wasted. I wonder we can add an FFI to convert single AnimationValue to String.

Yes, we can. Let me revise it! Thanks for suggestion.
With these patches can we drop the call of ComputeValuesFromStyleContext() in StyleAnimationValue::ComputeValues()?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> With these patches can we drop the call of ComputeValuesFromStyleContext()
> in StyleAnimationValue::ComputeValues()?

I will try to remove it. If there are no other problems, I will file a bug and attach my patches. Thanks for reminder. :)
Comment on attachment 8835272 [details]
Bug 1337313 - Part 2: Add AnimationValue::SerializeSpecifiedValue.

https://reviewboard.mozilla.org/r/110974/#review112236

> Yes, we can. Let me revise it! Thanks for suggestion.

I cannot use overloading on FFI, so use a different name for uncomputing and serializeing.
Comment on attachment 8835272 [details]
Bug 1337313 - Part 2: Add AnimationValue::SerializeSpecifiedValue.

https://reviewboard.mozilla.org/r/110974/#review112282

Yay! My review part became simpler!

::: layout/style/ServoBindingList.h:130
(Diff revision 2)
>                     RawServoAnimationValueBorrowed to,
>                     double progress)
>  SERVO_BINDING_FUNC(Servo_AnimationValues_Uncompute,
>                     RawServoDeclarationBlockStrong,
>                     RawServoAnimationValueBorrowedListBorrowed value)
> +SERVO_BINDING_FUNC(Servo_AnimationValues_Uncompute_and_Serialize, void,

I think Servo_AnimationValue_Serialize() would be sufficient, but I'd like to hear opitions from Manish. Anyway we should use AnimationValue instead of AnimationValues.

::: layout/style/ServoBindingList.h:134
(Diff revision 2)
>                     RawServoAnimationValueBorrowedListBorrowed value)
> +SERVO_BINDING_FUNC(Servo_AnimationValues_Uncompute_and_Serialize, void,
> +                   RawServoAnimationValueBorrowed value,
> +                   nsCSSPropertyID property,
> +                   nsAString* buffer)
>  SERVO_BINDING_FUNC(Servo_AnimationValues_GetOpacity, float,

Oh, I just realized we used AnimationValues here and below as well...
Attachment #8835272 - Flags: review?(hikezoe) → review+
Comment on attachment 8835272 [details]
Bug 1337313 - Part 2: Add AnimationValue::SerializeSpecifiedValue.

https://reviewboard.mozilla.org/r/110974/#review112282

> I think Servo_AnimationValue_Serialize() would be sufficient, but I'd like to hear opitions from Manish. Anyway we should use AnimationValue instead of AnimationValues.

|opinions|.
Comment on attachment 8835272 [details]
Bug 1337313 - Part 2: Add AnimationValue::SerializeSpecifiedValue.

https://reviewboard.mozilla.org/r/110974/#review112282

> Oh, I just realized we used AnimationValues here and below as well...

OK, I can rename these function as:
Servo_AnimationValue_Serialize (wait for Manish's opinions)
Servo_AnimationValue_GetOpacity
Servo_AninationValue_GetTransform
Blocks: 1338087
Comment on attachment 8835326 [details]
Bug 1337313 - [Servo] Add Servo_AnimationValue_Serialize.

https://reviewboard.mozilla.org/r/111014/#review112672
Attachment #8835326 - Flags: review?(manishearth) → review+
Comment on attachment 8835859 [details]
Bug 1337313 - Part 3: Rename Servo_AnimationValues_XXX with Servo_AnimationValue_XXX.

https://reviewboard.mozilla.org/r/111424/#review112726

Nice!
Attachment #8835859 - Flags: review?(hikezoe) → review+
Attachment #8835326 - Attachment is obsolete: true
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74aa5f52c7c5
Part 1: Use AnimationValue in CreatePropertyValue. r=hiro
https://hg.mozilla.org/integration/autoland/rev/4184cf052f15
Part 2: Add AnimationValue::SerializeSpecifiedValue. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b83e2b2524c9
Part 3: Rename Servo_AnimationValues_XXX with Servo_AnimationValue_XXX. r=hiro
https://hg.mozilla.org/mozilla-central/rev/74aa5f52c7c5
https://hg.mozilla.org/mozilla-central/rev/4184cf052f15
https://hg.mozilla.org/mozilla-central/rev/b83e2b2524c9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.