1358330 - stylo: computed timing function should preserve keyworded function

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

This is a counter part of https://github.com/servo/servo/issues/15086

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1358330 - Factor out serialize keywoded timing function values.

This will be used for computed timing function as well.

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1358330 - Use FunctionKeyword for computed_value of timing-function as well.

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1358330 - Update mochitest expectations for timing-function

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860237 [details]
Bug 1358330 - Use FunctionKeyword for computed_value of timing-function as well.

https://reviewboard.mozilla.org/r/132236/#review135084

::: servo/components/style/animation.rs:338
(Diff revision 1)
> -        let progress = match self.timing_function {
> -            TransitionTimingFunction::CubicBezier(p1, p2) => {
> +        macro_rules! compute_cubic_bezier_progress {
> +            ($time: ident, $p1: ident, $p2: ident, $duration: expr) => {{
>                  // See `WebCore::AnimationBase::solveEpsilon(double)` in WebKit.
> -                let epsilon = 1.0 / (200.0 * (self.duration.seconds() as f64));
> -                Bezier::new(Point2D::new(p1.x as f64, p1.y as f64),
> -                            Point2D::new(p2.x as f64, p2.y as f64)).solve(time, epsilon)
> +                let epsilon = 1.0 / (200.0 * ($duration as f64));
> +                Bezier::new(Point2D::new($p1.x as f64, $p1.y as f64),
> +                            Point2D::new($p2.x as f64, $p2.y as f64)).solve($time, epsilon)
> +            }};
> +        }
> +        macro_rules! compute_step_progress {
> +            ($time: ident, $steps: ident, StartEnd::Start) => {
> +                ($time * ($steps as f64)).ceil() / ($steps as f64)
> +            };
> +            ($time: ident, $steps: ident, StartEnd::End) => {
> +                ($time * ($steps as f64)).floor() / ($steps as f64)
> +            };

These two macros aren't great, but I have no other idea to avoid repetitions of the calculations.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8860237 [details]
Bug 1358330 - Use FunctionKeyword for computed_value of timing-function as well.

https://reviewboard.mozilla.org/r/132236/#review135176

::: servo/components/style/animation.rs:354
(Diff revision 1)
> +            };
> +            ($time: ident, $steps: ident, StartEnd::End) => {
> +                ($time * ($steps as f64)).floor() / ($steps as f64)
> +            };
> -            }
> +        }
> +        let progress = match self.timing_function {

What about
```rust
let timeing_function = match self.timing_function {
    TransitionTimingFunction::Keyword(keyword) => keyword.to_non_keyword_value(),
    other => other,
};
let progress = match timing_function {
    // ...
};
```
?

This way you can remove the macros and don't need to touch the existing code that much.

::: servo/components/style/properties/longhand/box.mako.rs:496
(Diff revision 1)
>          pub enum T {
>              CubicBezier(Point2D<f32>, Point2D<f32>),
>              Steps(u32, StartEnd),
> +            Keyword(FunctionKeyword),
>          }

So now your `computed_value::T` is identical to `SpecifiedValue`. You should just make them `SpecifiedValue` be the alias of the computed one.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8860236 [details]
Bug 1358330 - Factor out serialize keywoded timing function values.

https://reviewboard.mozilla.org/r/132234/#review135178

When you unify type of computed value and specified value, you probably don't need this patch anymore.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8860238 [details]
Bug 1358330 - Update mochitest expectations for timing-function

https://reviewboard.mozilla.org/r/132238/#review135180

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #5)

> This way you can remove the macros and don't need to touch the existing code
> that much.
> 
> ::: servo/components/style/properties/longhand/box.mako.rs:496
> (Diff revision 1)
> >          pub enum T {
> >              CubicBezier(Point2D<f32>, Point2D<f32>),
> >              Steps(u32, StartEnd),
> > +            Keyword(FunctionKeyword),
> >          }
> 
> So now your `computed_value::T` is identical to `SpecifiedValue`. You should
> just make them `SpecifiedValue` be the alias of the computed one.

SpecifiedValue is bit different;

  CubicBezier(Point2D<Number>, Point2D<Number>),                                                                  
  Steps(specified::Integer, StartEnd),

I did refer to counter-increment property which is also similar to this;
  struct SpecifiedValue(pub Vec<(String, specified::Integer)>);
  pub mod computed_value {
    pub struct T(pub Vec<(String, i32)>);
  }

Can we unify them?

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #5)
> Comment on attachment 8860237 [details]
> Bug 1358330 - Use FunctionKeyword for computed_value of timing-function as
> well.
> 
> https://reviewboard.mozilla.org/r/132236/#review135176
> 
> ::: servo/components/style/animation.rs:354
> (Diff revision 1)
> > +            };
> > +            ($time: ident, $steps: ident, StartEnd::End) => {
> > +                ($time * ($steps as f64)).floor() / ($steps as f64)
> > +            };
> > -            }
> > +        }
> > +        let progress = match self.timing_function {
> 
> What about
> ```rust
> let timeing_function = match self.timing_function {
>     TransitionTimingFunction::Keyword(keyword) =>
> keyword.to_non_keyword_value(),
>     other => other,
> };
> let progress = match timing_function {
>     // ...
> };
> ```

Oh, looks great!  Thanks! I will do it.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Oh, I didn't notice the difference on "specified::Integer" and "u32". Probably we should leave the separate.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860236 [details]
Bug 1358330 - Factor out serialize keywoded timing function values.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860237 [details]
Bug 1358330 - Use FunctionKeyword for computed_value of timing-function as well.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860238 [details]
Bug 1358330 - Update mochitest expectations for timing-function

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

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8860237 [details]
Bug 1358330 - Use FunctionKeyword for computed_value of timing-function as well.

https://reviewboard.mozilla.org/r/132236/#review137328

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8860236 [details]
Bug 1358330 - Factor out serialize keywoded timing function values.

https://reviewboard.mozilla.org/r/132234/#review137332

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Thank you Xidorn.
A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ffb4efacf49fc08c7d52262a277f20196a52205

stylo-failures.md might need to be modified because stylo people have been fixing lots of issues at a fast pace. What is the best thing is that we can see its progress thanks to stylo-failures.md which is one of Xidorn's great works!

Comment 17

[Hiroyuki Ikezoe (:hiro)]

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

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860238 [details]
Bug 1358330 - Update mochitest expectations for timing-function

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a468e96f053b
Update mochitest expectations for timing-function r=xidorn

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a468e96f053b