Closed Bug 1332657 Opened 7 years ago Closed 7 years ago

stylo: Make transform animatable

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: boris, Assigned: hiro)

References

Details

Attachments

(5 files)

Bug 1332657 - Part 1: Use more proper function to set nsCSSValue.
59 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details
Bug 1332657 - Part 2: Implement clone_transform.
59 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details
Bug 1332657 - Part 3: Make transform property animatable.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1332657 - Part 4: Adjust reftest expectations for transform animations.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1332657 - Part 5: Adjust crashtest expectations for transform animations.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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
Blocks: 1323733
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.
Priority: -- → P3
Blocks: 1328505
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.
(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!
(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.
Blocks: 1335342
Here is another try with the revised patch.
(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
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.
Filed bug 1335998 for the interpolation problem that needs reference box.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Blocks: 1324693
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.
Attachment #8832757 - Flags: review+
Comment on attachment 8832757 [details]
Bug 1332657 - Part 1: Use more proper function to set nsCSSValue.

https://reviewboard.mozilla.org/r/108982/#review110220
Attachment #8832757 - Flags: review?(cam) → review+
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)?
Attachment #8832758 - Flags: review?(cam) → review+
Comment on attachment 8832760 [details]
Bug 1332657 - Part 4: Adjust reftest expectations for transform animations.

https://reviewboard.mozilla.org/r/108988/#review110244
Attachment #8832760 - Flags: review?(cam) → review+
Comment on attachment 8832761 [details]
Bug 1332657 - Part 5: Adjust crashtest expectations for transform animations.

https://reviewboard.mozilla.org/r/108990/#review110246
Attachment #8832761 - Flags: review?(cam) → review+
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 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 on attachment 8832758 [details]
Bug 1332657 - Part 2: Implement clone_transform.

https://reviewboard.mozilla.org/r/108984/#review110476
Attachment #8832758 - Flags: review?(manishearth) → review+
Comment on attachment 8832759 [details]
Bug 1332657 - Part 3: Make transform property animatable.

https://reviewboard.mozilla.org/r/108986/#review110478
Attachment #8832759 - Flags: review?(manishearth) → review+
Pushed by hikezoe@mozilla.com:
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
Did part 3 land? Did it need to?
Flags: needinfo?(hikezoe)
Blocks: 1336581
Part 3 is Servo code only, so it doesn't land in m-c yet, thanks for checking Wes!
Flags: needinfo?(hikezoe)
No longer blocks: 1336581
Blocks: 1335942
Blocks: 1335998
Blocks: 1337735
You need to log in before you can comment on or make changes to this bug.