Boris Chiou [:boris] Reporter
	Description • 8 years ago

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

	Boris Chiou [:boris] Reporter
	Comment 1 • 8 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 2 • 8 years ago

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.

	Boris Chiou [:boris] Reporter
	Comment 3 • 8 years ago

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 4 • 8 years ago

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 5 • 8 years ago

Here is another try with the revised patch.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 6 • 8 years ago

(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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 7 • 8 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 8 • 8 years ago

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

	Manish Goregaokar [:manishearth]
	Comment 14 • 8 years ago
mozreview-review

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.

	Cameron McCormack (:heycam)
	Comment 15 • 8 years ago
mozreview-review

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

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

	Cameron McCormack (:heycam)
	Comment 16 • 8 years ago
mozreview-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)?

	Cameron McCormack (:heycam)
	Comment 17 • 8 years ago
mozreview-review

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

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

	Cameron McCormack (:heycam)
	Comment 18 • 8 years ago
mozreview-review

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

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 23 • 8 years ago

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!

	Manish Goregaokar [:manishearth]
	Comment 24 • 8 years ago
mozreview-review

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.

	Manish Goregaokar [:manishearth]
	Comment 25 • 8 years ago
mozreview-review

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

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

	Manish Goregaokar [:manishearth]
	Comment 26 • 8 years ago
mozreview-review

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

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 31 • 8 years ago

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 32 • 8 years ago

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

	Pulsebot
	Comment 33 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 34 • 8 years ago
bugherder

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 35 • 8 years ago

Did part 3 land? Did it need to?

	Emilio Cobos Álvarez (:emilio)
	Comment 36 • 8 years ago

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