1332657 - stylo: Make transform animatable

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Boris Chiou [:boris]]

We make transform non-animatable [1] in stylo. Though we still have "lengths and percentages" problem for translate [2] in servo, we can still try to enable it, and let it work on some simple animation tests.

[1] https://dxr.mozilla.org/servo/rev/1f76aa6ef7ec7db0a0c40223874a72e8f4059deb/components/style/properties/longhand/box.mako.rs#1053
[2] https://github.com/servo/servo/issues/13267

Comment 1

[Boris Chiou [:boris]]

After applying patches of Bug 1317209, we start to get many assertions:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1272475-1.html | assertion count 18 is more than expected 3 to 10 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1272475-2.html | assertion count 22 is more than expected 4 to 10 assertions

Both 1272475-1.html and 1272475-2.html use *Transform* to test, so we got assertions because we don't support transform now. I think this bug can fix them.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I agree with Boris.
Let's see if what happens:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e36395ffa404c6f07e557d1d83e4ff75cb1b685

Note that about 1272475-1.html and 1272475-2.html I did confirm that they crash because of servo's interpolation results NaN value  there.

Comment 3

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> I agree with Boris.
> Let's see if what happens:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9e36395ffa404c6f07e557d1d83e4ff75cb1b685
> 
> Note that about 1272475-1.html and 1272475-2.html I did confirm that they
> crash because of servo's interpolation results NaN value  there.

Ha, looks like you are working on this bug. Cool!

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> I agree with Boris.
> Let's see if what happens:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9e36395ffa404c6f07e557d1d83e4ff75cb1b685

The try result looks reasonable to me.  I found some issues in conversions nsCSSValue in the patch. I will revise it soon.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Here is another try with the revised patch.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Here is another try with the revised patch.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bbfc526c10b3d31206ba3ebdaf1152b45f4e96e
With some modifications against reftest-stylo.lists and crashtest.list, now the try results are green.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Filed bug 1335998 for the interpolation problem that needs reference box.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1332657 - Part 1: Use more proper function to set nsCSSValue.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1332657 - Part 2: Implement clone_transform.

The implementation of clone_transform is an adaptation of set_transform.

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1332657 - Part 3: Make transform property animatable.

This just changes animatable to True, drops 'if product == "servo" block
, fixes indentations and moves 'use' declarations at the top of the file.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1332657 - Part 4: Adjust reftest expectations for transform animations.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1332657 - Part 5: Adjust crashtest expectations for transform animations.

Actually 1328535-1.html does not have transform animations, it has been passed
without these patches.

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

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8832757 [details]
Bug 1332657 - Part 1: Use more proper function to set nsCSSValue.

https://reviewboard.mozilla.org/r/108982/#review110208

::: layout/style/ServoBindings.cpp:1088
(Diff revision 1)
>  }
>  
>  void
>  Gecko_CSSValue_SetPercentage(nsCSSValueBorrowedMut aCSSValue, float aPercent)
>  {
> -  aCSSValue->SetFloatValue(aPercent, eCSSUnit_Number);
> +  aCSSValue->SetPercentValue(aPercent);

This is eCSSUnit_Percent, not eCSSUnit_Number, fwiw. Though that is probably more correct, the previous code was probably wrong.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8832757 [details]
Bug 1332657 - Part 1: Use more proper function to set nsCSSValue.

https://reviewboard.mozilla.org/r/108982/#review110220

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8832758 [details]
Bug 1332657 - Part 2: Implement clone_transform.

https://reviewboard.mozilla.org/r/108984/#review110226

::: layout/style/ServoBindings.h:302
(Diff revision 1)
>  nsStyleQuoteValues* Gecko_NewStyleQuoteValues(uint32_t len);
>  NS_DECL_THREADSAFE_FFI_REFCOUNTING(nsStyleQuoteValues, QuoteValues);
>  
>  nsCSSValueSharedList* Gecko_NewCSSValueSharedList(uint32_t len);
> +
> +// Gettter for nsCSSValue

Getter

You might want to move the GetArrayItem calls up to this block.

::: layout/style/ServoBindings.cpp:1076
(Diff revision 1)
>  }
>  
> +nscoord
> +Gecko_CSSValue_GetAbsoluteLength(nsCSSValueBorrowed aCSSValue)
> +{
> +  // SetIntegerCoordValue() which is used in Gecko_CSSValue_GetAbsoluteLength()

in Gecko_CSSValue_SetAbsoluteLength()

::: layout/style/ServoBindings.cpp:1144
(Diff revision 1)
>  {
>    aCSSValue->SetCalcValue(&aCalc);
>  }
>  
> +nsStyleCoord::CalcValue
> +Gecko_CSSValue_GetCalc(nsCSSValueBorrowed aCSSValue)

Since this function relies on the structure of the nsCSSValue that is set in nsCSSValue::SetCalcValue, it would be nice to have the logic near there.  Can you add a static function to nsCSSValue to do this work, right below nsCSSValue::SetCalcValue, and call it from here?

::: layout/style/ServoBindings.cpp:1153
(Diff revision 1)
> +
> +  const nsCSSValue::Array* array = aCSSValue->GetArrayValue();
> +  MOZ_ASSERT(array->Count() == 1,
> +             "There should be a 1-length array");
> +
> +  const nsCSSValue &rootValue = array->Item(0);

Nit: "&" next to type.  (And in the rest of this function too.)

::: servo/components/style/properties/gecko.mako.rs:1454
(Diff revision 1)
> +                x => panic!("The unit should not be {:?}", x),
> +            }
> +        }
> +
> +        let mut result = vec![];
> +        if !self.gecko.mSpecifiedTransform.mRawPtr.is_null() {

If it is null, don't we want to return computed_value::T(None)?

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8832760 [details]
Bug 1332657 - Part 4: Adjust reftest expectations for transform animations.

https://reviewboard.mozilla.org/r/108988/#review110244

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8832761 [details]
Bug 1332657 - Part 5: Adjust crashtest expectations for transform animations.

https://reviewboard.mozilla.org/r/108990/#review110246

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832758 [details]
Bug 1332657 - Part 2: Implement clone_transform.

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832759 [details]
Bug 1332657 - Part 3: Make transform property animatable.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832760 [details]
Bug 1332657 - Part 4: Adjust reftest expectations for transform animations.

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832761 [details]
Bug 1332657 - Part 5: Adjust crashtest expectations for transform animations.

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

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Thank you for the review!

(In reply to Cameron McCormack (:heycam) from comment #16)
> ::: layout/style/ServoBindings.cpp:1144
> (Diff revision 1)
> >  {
> >    aCSSValue->SetCalcValue(&aCalc);
> >  }
> >  
> > +nsStyleCoord::CalcValue
> > +Gecko_CSSValue_GetCalc(nsCSSValueBorrowed aCSSValue)
> 
> Since this function relies on the structure of the nsCSSValue that is set in
> nsCSSValue::SetCalcValue, it would be nice to have the logic near there. 
> Can you add a static function to nsCSSValue to do this work, right below
> nsCSSValue::SetCalcValue, and call it from here?

Added nsCSSValue::GetCalcValue there.

> ::: servo/components/style/properties/gecko.mako.rs:1454
> (Diff revision 1)
> > +                x => panic!("The unit should not be {:?}", x),
> > +            }
> > +        }
> > +
> > +        let mut result = vec![];
> > +        if !self.gecko.mSpecifiedTransform.mRawPtr.is_null() {
> 
> If it is null, don't we want to return computed_value::T(None)?

Good catch!  Thanks!

Comment 24

[Manish Goregaokar [:manishearth]]

Comment on attachment 8832758 [details]
Bug 1332657 - Part 2: Implement clone_transform.

https://reviewboard.mozilla.org/r/108984/#review110474

::: servo/components/style/properties/gecko.mako.rs:1441
(Diff revision 2)
> +        use values::computed::LengthOrPercentage;
> +
> +        unsafe fn get_lop(value: &structs::nsCSSValue) -> LengthOrPercentage {
> +            match value.mUnit {
> +                eCSSUnit_Pixel => {
> +                    LengthOrPercentage::Length(Au(bindings::Gecko_CSSValue_GetAbsoluteLength(value)))

Can these be methods on nsCSSValue? see gecko_bindings/sugar/nscssvalue.rs for existing methods.

Comment 25

[Manish Goregaokar [:manishearth]]

Comment on attachment 8832758 [details]
Bug 1332657 - Part 2: Implement clone_transform.

https://reviewboard.mozilla.org/r/108984/#review110476

Comment 26

[Manish Goregaokar [:manishearth]]

Comment on attachment 8832759 [details]
Bug 1332657 - Part 3: Make transform property animatable.

https://reviewboard.mozilla.org/r/108986/#review110478

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832758 [details]
Bug 1332657 - Part 2: Implement clone_transform.

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

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832759 [details]
Bug 1332657 - Part 3: Make transform property animatable.

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

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832760 [details]
Bug 1332657 - Part 4: Adjust reftest expectations for transform animations.

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

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8832761 [details]
Bug 1332657 - Part 5: Adjust crashtest expectations for transform animations.

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

Comment 31

[Hiroyuki Ikezoe (:hiro)]

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

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Servo PR: https://github.com/servo/servo/pull/15363

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a68e62ebe7
Part 1: Use more proper function to set nsCSSValue. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/def7c6ecd132
Part 2: Implement clone_transform. r=heycam,manishearth
https://hg.mozilla.org/integration/mozilla-inbound/rev/76dab2f39230
Part 4: Adjust reftest expectations for transform animations. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2e3ba1f929
Part 5: Adjust crashtest expectations for transform animations. r=heycam

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1a68e62ebe7
https://hg.mozilla.org/mozilla-central/rev/def7c6ecd132
https://hg.mozilla.org/mozilla-central/rev/76dab2f39230
https://hg.mozilla.org/mozilla-central/rev/0f2e3ba1f929

Comment 35

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

Did part 3 land? Did it need to?

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Part 3 is Servo code only, so it doesn't land in m-c yet, thanks for checking Wes!