1352891 - stylo: Set control points values to nsTimingFunction even if the function type is keyworded types (e.g. ease, ease-in, ...)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

We had to set control points values to nsTimingFunction when we convert servo's specified timing function value to nsTimingFunction.  In Gecko we do it [1], I did totally miss that.

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

[1] https://hg.mozilla.org/mozilla-central/file/38894655c89e/layout/style/nsStyleStruct.cpp#l3164

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1352891 - Set each control points when converting specified keyworded timing function to nsTimingFunction.

Gecko's timing function (nsTimingFunction) needs to be specified
each control points if timing function can be represented as cubic-bezier
function. To avoid scattering control points values (e.g. 0.25, 0.1, ...)
we convert specified value to computed value and then use control points
values of the computed value.

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1352891 - Update mochitest expectations for animation.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8853825 [details]
Bug 1352891 - Set each control points when converting specified keyworded timing function to nsTimingFunction.

https://reviewboard.mozilla.org/r/125862/#review128332

r=me with those, thanks for doing this!

::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:24
(Diff revision 1)
>          unsafe {
>              self.__bindgen_anon_1.__bindgen_anon_1.as_mut().mStepsOrFrames = steps;
>          }
>      }
>  
> -    fn set_as_cubic_bezier(&mut self, p1: Point2D<f32>, p2: Point2D<f32>) {
> +    fn set_as_bezier(&mut self, function_type: nsTimingFunction_Type,

nit: let's put each argument in its own line.

::: servo/components/style/properties/longhand/box.mako.rs:707
(Diff revision 1)
>          }
>      }
>  
> +    impl SpecifiedValue {
> +        #[inline]
> +        pub fn keyword_to_computed_value(&self) -> computed_value::T {

Let's move this to `FunctionKeyword` instead (in rust you can implement methods on enums), and call it also from `SpecifiedValue::to_computed_value`. This would get rid from the `panic!` below, and share more code.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)

> ::: servo/components/style/properties/longhand/box.mako.rs:707
> (Diff revision 1)
> >          }
> >      }
> >  
> > +    impl SpecifiedValue {
> > +        #[inline]
> > +        pub fn keyword_to_computed_value(&self) -> computed_value::T {
> 
> Let's move this to `FunctionKeyword` instead (in rust you can implement
> methods on enums), and call it also from
> `SpecifiedValue::to_computed_value`. This would get rid from the `panic!`
> below, and share more code.

Fantastic! Your review always makes code clean and makes me happy!!

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8853826 [details]
Bug 1352891 - Update mochitest expectations for animation.

https://reviewboard.mozilla.org/r/125864/#review128334

self stamping.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

https://github.com/servo/servo/pull/16233

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8853826 [details]
Bug 1352891 - Update mochitest expectations for animation.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cd3a3e5f3dd
Update mochitest expectations for animation. r=hiro

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8cd3a3e5f3dd