Closed Bug 1358330 Opened 7 years ago Closed 7 years ago

stylo: computed timing function should preserve keyworded function

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1358330 - Update mochitest expectations for timing-function
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
This is a counter part of https://github.com/servo/servo/issues/15086
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 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.
Attachment #8860237 - Flags: review?(xidorn+moz)
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.
Attachment #8860236 - Flags: review?(xidorn+moz)
Comment on attachment 8860238 [details]
Bug 1358330 - Update mochitest expectations for timing-function

https://reviewboard.mozilla.org/r/132238/#review135180
Attachment #8860238 - Flags: review?(xidorn+moz) → review+
(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?
(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.
Oh, I didn't notice the difference on "specified::Integer" and "u32". Probably we should leave the separate.
Blocks: 1356087
Comment on attachment 8860237 [details]
Bug 1358330 - Use FunctionKeyword for computed_value of timing-function as well.

https://reviewboard.mozilla.org/r/132236/#review137328
Attachment #8860237 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8860236 [details]
Bug 1358330 - Factor out serialize keywoded timing function values.

https://reviewboard.mozilla.org/r/132234/#review137332
Attachment #8860236 - Flags: review?(xidorn+moz) → review+
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!
Attachment #8860236 - Attachment is obsolete: true
Attachment #8860237 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a468e96f053b
Update mochitest expectations for timing-function r=xidorn
https://hg.mozilla.org/mozilla-central/rev/a468e96f053b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: