1359719 - stylo: Implement frames timing function

Status: RESOLVED FIXED

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

Description

[Boris Chiou [:boris]]

1. Implement the parser for frames timing functions (and Servo should also support the frames timing function.)
2. Update the mochitest expectation.

Comment 1

[Boris Chiou [:boris]]

I fixed this bug before, but notice that we need to update mochitest expectations, so let's review them together.

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1359719 - Part 1: Add frames into style system and update animation with frames functions.

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

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1359719 - Part 2: Fix inaccurate input progress in the final animation frame.

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

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1359719 - Part 3: Add unit tests for frames timing function.

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

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1359719 - Part 4: Add unit tests for step timing function.

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

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1359719 - Part 5: Update nsTimingFunction binding for stylo.

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

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1359719 - Update mochitest expectations for frames timing function.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8861804 [details]
Bug 1359719 - Update mochitest expectations for frames timing function.

https://reviewboard.mozilla.org/r/133812/#review136698

::: layout/style/test/stylo-failures.md:56
(Diff revision 1)
>      * test_restyles_in_smil_animation.html [2]
> -  * CSS Timing Functions: Frames timing functions
> -    * test_value_storage.html `frames` [30]
>    * Property parsing and computation:
>      * test_property_syntax_errors.html `animation` [20]
> -    * test_value_storage.html `animation` [91]
> +    * test_value_storage.html `animation` [76]

With servo/servo#16455, the number of this has been reduced to 15, so I guess you are fixing all the remaining on this item. Nice!

You would need a rebase because of that, though.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8861799 [details]
Bug 1359719 - Part 1: Add frames into style system and update animation with frames functions.

https://reviewboard.mozilla.org/r/133802/#review136756

::: servo/components/style/properties/longhand/box.mako.rs:618
(Diff revision 1)
> +                    "frames" => {
> +                        // https://drafts.csswg.org/css-timing/#frames-timing-functions
> +                        let mut frames = specified::Integer::new(2);
> +                        try!(input.parse_nested_block(|input| {
> +                            frames = try!(specified::parse_integer(context, input));
> +                            if frames.value() < 2 { Err(()) } else { Ok(()) }

Instead of this, let's use `Integer::parse_with_minimum`, which would do the right thing eventually for `calc()` (in particular, `calc(-1)` is a valid specified value for this property, but is clamped at computed value time).

Also, I don't think you need to create the variable before-hand, I think:

```
let frames = try!(input.parse_nested_block(|input| {
    Integer::parse_with_minimum(context, input, 2)
}));
```

should just work.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8861800 [details]
Bug 1359719 - Part 2: Fix inaccurate input progress in the final animation frame.

https://reviewboard.mozilla.org/r/133804/#review136758

I guess :)

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8861801 [details]
Bug 1359719 - Part 3: Add unit tests for frames timing function.

https://reviewboard.mozilla.org/r/133806/#review136760

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8861802 [details]
Bug 1359719 - Part 4: Add unit tests for step timing function.

https://reviewboard.mozilla.org/r/133808/#review136762

Thanks for the extra tests! :)

::: servo/tests/unit/style/parsing/transition_timing_function.rs:27
(Diff revision 1)
>      assert!(parse(transition_timing_function::parse, "cubic-bezier(2, 0, 2, 0").is_err());
>  }
>  
>  #[test]
>  fn test_steps() {
> -    assert_roundtrip_with_context!(transition_timing_function::parse, "steps(1)");
> +    assert_roundtrip_with_context!(transition_timing_function::parse, "steps(  1)", "steps(1)");

We could also leave the first test too, but no big deal.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8861803 [details]
Bug 1359719 - Part 5: Update nsTimingFunction binding for stylo.

https://reviewboard.mozilla.org/r/133810/#review136764

Comment 14

[Boris Chiou [:boris]]

Comment on attachment 8861799 [details]
Bug 1359719 - Part 1: Add frames into style system and update animation with frames functions.

https://reviewboard.mozilla.org/r/133802/#review136756

> Instead of this, let's use `Integer::parse_with_minimum`, which would do the right thing eventually for `calc()` (in particular, `calc(-1)` is a valid specified value for this property, but is clamped at computed value time).
> 
> Also, I don't think you need to create the variable before-hand, I think:
> 
> ```
> let frames = try!(input.parse_nested_block(|input| {
>     Integer::parse_with_minimum(context, input, 2)
> }));
> ```
> 
> should just work.

Yes. Integer::parse_with_minimum is a good idea. However, it is a private fn, so I will also make it public. Thanks for suggestion.

Comment 15

[Boris Chiou [:boris]]

Comment on attachment 8861804 [details]
Bug 1359719 - Update mochitest expectations for frames timing function.

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

Comment 16

[Boris Chiou [:boris]]

Attachment: Servo PR, #16626

Comment 17

[Pulsebot]

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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