Closed Bug 1352891 Opened 5 years ago Closed 5 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(1 file, 1 obsolete file)

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 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.
Attachment #8853825 - Flags: review?(emilio+bugs) → review+
(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 on attachment 8853826 [details]
Bug 1352891 - Update mochitest expectations for animation.

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

self stamping.
Attachment #8853826 - Flags: review?(hikezoe) → review+
Attachment #8853825 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cd3a3e5f3dd
Update mochitest expectations for animation. r=hiro
https://hg.mozilla.org/mozilla-central/rev/8cd3a3e5f3dd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.