Closed Bug 1337313 Opened 7 years ago Closed 7 years ago

stylo: Use RawServoAnimationValue for KeyframeEffectReadOnly::GetProperties()

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla54

Tracking Flags:

Tracking

Status

firefox54

---

fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1337313 - Part 1: Use AnimationValue in CreatePropertyValue. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	hiro : review+	Details
Bug 1337313 - Part 2: Add AnimationValue::SerializeSpecifiedValue. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	hiro : review+	Details
Bug 1337313 - [Servo] Add Servo_AnimationValue_Serialize. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	manishearth : review+	Details
Bug 1337313 - Part 3: Rename Servo_AnimationValues_XXX with Servo_AnimationValue_XXX. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	hiro : review+	Details
Servo PR, #15493 7 years ago Boris Chiou [:boris] 41 bytes, text/x-github-pull-request		Details \| Review

Boris Chiou [:boris]

Assignee

Description

•

7 years ago

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.

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Assignee: nobody → boris.chiou

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Status: NEW → ASSIGNED

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Comment 3

•

7 years ago

mozreview-review

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+

Hiroyuki Ikezoe (:hiro)

Comment 4

•

7 years ago

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

Boris Chiou [:boris]

Assignee

Comment 5

•

7 years ago

mozreview-review-reply

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.

Hiroyuki Ikezoe (:hiro)

Comment 6

•

7 years ago

With these patches can we drop the call of ComputeValuesFromStyleContext() in StyleAnimationValue::ComputeValues()?

Boris Chiou [:boris]

Assignee

Comment 7

•

7 years ago

(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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

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

Boris Chiou [:boris]

Assignee

Comment 10

•

7 years ago

mozreview-review-reply

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.

Hiroyuki Ikezoe (:hiro)

Comment 11

•

7 years ago

mozreview-review

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+

Hiroyuki Ikezoe (:hiro)

Comment 12

•

7 years ago

mozreview-review-reply

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

Boris Chiou [:boris]

Assignee

Comment 13

•

7 years ago

mozreview-review-reply

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

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Blocks: 1338087

Manish Goregaokar [:manishearth]

Comment 14

•

7 years ago

mozreview-review

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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

Comment on attachment 8835271 [details]
Bug 1337313 - Part 1: Use AnimationValue in CreatePropertyValue.

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

Comment hidden (mozreview-request)

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

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

Comment hidden (mozreview-request)

Comment on attachment 8835326 [details]
Bug 1337313 - [Servo] Add Servo_AnimationValue_Serialize.

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

Hiroyuki Ikezoe (:hiro)

Comment 19

•

7 years ago

mozreview-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+

Boris Chiou [:boris]

Assignee

Comment 20

•

7 years ago

gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0241c7941f5e

Boris Chiou [:boris]

Assignee

Comment 21

•

7 years ago

Attached file Servo PR, #15493 — Details

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Attachment #8835326 - Attachment is obsolete: true

Pulsebot

Comment 22

•

7 years ago

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

Carsten Book [:Tomcat]

Comment 23

•

7 years ago

bugherder

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: 7 years ago

status-firefox54: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla54

You need to log in before you can comment on or make changes to this bug.