1462829 - Cleanup vector types

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Just some cleanup that ended up going a bit further. I'll add a few more cleanups  on top, like to animation_value_type and such.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1462829: Refactor vector types.

This fixes clamping of mask-size and moves it out of mako while at it.

Also fixes completion keywords for vector types that allow "none", and removes a
bunch of duplicated list animation code.

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251220

I am still in the middle of reviewing,

::: commit-message-82716:3
(Diff revision 1)
> +Bug 1462829: Refactor vector types. r?hiro,xidorn
> +
> +This fixes clamping of mask-size and moves it out of mako while at it.

Though I am still trying to understand what this patch does overall, I mean I am in the middle of reviewing, IIUC this patch, the clamping issue does originally exists in 'background-size' property instead of mask-size, and mask-size has also another issue that it's not animatable as repeatable-list, right?

Anyway, that means that we don't have such test cases?

Wow, I found now that there are some comments by dbaron [1], can we now enable them?

[1] https://searchfox.org/mozilla-central/source/layout/style/test/test_transitions_per_property.html#109

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251220

> Though I am still trying to understand what this patch does overall, I mean I am in the middle of reviewing, IIUC this patch, the clamping issue does originally exists in 'background-size' property instead of mask-size, and mask-size has also another issue that it's not animatable as repeatable-list, right?
> 
> Anyway, that means that we don't have such test cases?
> 
> Wow, I found now that there are some comments by dbaron [1], can we now enable them?
> 
> [1] https://searchfox.org/mozilla-central/source/layout/style/test/test_transitions_per_property.html#109

So, background-size works because it was clamped manually in https://searchfox.org/mozilla-central/rev/9769f2300a17d3dfbebcfb457b1244bd624275e3/servo/components/style/values/computed/background.rs#49.

This didn't work for mask-size because of animation_value_type="ComputedValue".

I'll try to figure out a way to test this.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> > Though I am still trying to understand what this patch does overall, I mean I am in the middle of reviewing, IIUC this patch, the clamping issue does originally exists in 'background-size' property instead of mask-size, and mask-size has also another issue that it's not animatable as repeatable-list, right?
> > 
> > Anyway, that means that we don't have such test cases?
> > 
> > Wow, I found now that there are some comments by dbaron [1], can we now enable them?
> > 
> > [1] https://searchfox.org/mozilla-central/source/layout/style/test/test_transitions_per_property.html#109
> 
> So, background-size works because it was clamped manually in
> https://searchfox.org/mozilla-central/rev/
> 9769f2300a17d3dfbebcfb457b1244bd624275e3/servo/components/style/values/
> computed/background.rs#49.

Yeah, thanks!  I noticed it after I left the comment.  So that means that we could enable the disabled tests when we land bug 1374233 at least for stylo, I think.

> This didn't work for mask-size because of
> animation_value_type="ComputedValue".
> 
> I'll try to figure out a way to test this.

I don't have strong opinion to add the test case here in this bug, we have to write test cases for additive and accumulative animations for those properties in some day (the test cases should be in testing/web-platform/tests/web-animations/animation-model/animation-types/).  So I am fine with just opening a new bug for the test cases.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251224

Overall looks good.  r=me with enabling test_length_percent_pair_clamped/unclamped for mask-size, background-size, and mask-position(s) and background-position(s) in test_transitions_per_property.html.

Thanks!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1462829: Test {background,mask}-{size,position} clamping.

These properties no longer use calc() as an intermediate value as long as the
types of the values match.

But they don't resolve percentages to pixels in getComputedStyle as
transform-origin and perspective-origin.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8979015 [details]
Bug 1462829: Test {background,mask}-{size,position} clamping.

https://reviewboard.mozilla.org/r/245288/#review251226

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8979015 [details]
Bug 1462829: Test {background,mask}-{size,position} clamping.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I removed the bit about the "none" keyword because it was false, they get included via if_empty=

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251304

::: servo/components/style/properties/helpers.mako.rs:127
(Diff revision 3)
>              % if separator == "Comma":
>              #[css(comma)]
>              % endif
> -            #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
> -            % if need_animatable or animation_value_type == "ComputedValue":
> -            #[derive(Animate, ComputeSquaredDistance)]
> +            #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToAnimatedValue,
> +                     ToCss)]
> +            pub struct List<T>(

Reading through the whole patch, it is still not clear to me how it makes sense to have a separate generic `List` type and have `T = List<single_value::T>`. Could you explain?

::: servo/components/style/properties/helpers.mako.rs:144
(Diff revision 3)
> -                use values::animated::{ToAnimatedZero};
>  
> -                impl ToAnimatedZero for T {
> -                    #[inline]
> +            /// The computed value, effectively a list of single values.
> +            % if vector_animation_type:
> +            % if not animation_value_type:
> +                Sorry, this is stupid but needed for now.

Maybe `throw "Sorry, ..."`.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:783
(Diff revision 3)
> +    ($ty:ty) => {
> +        impl<T> ListAnimation<T> for $ty {

To be honest, this confused me for quite a while. To some extend, it seems to break the hygienics of naming in macro.

I'd really prefer you have the macro accept something like
```rust
(<$T:ident> for $ty:ty)
```
and make its users do
```rust
animated_list_impl!(<T> for Vec<T>)
```
which should at least make it clear how the `T` is bound.

::: servo/components/style/values/generics/background.rs:8
(Diff revision 3)
>  #[derive(Animate, Clone, ComputeSquaredDistance, Copy, Debug, MallocSizeOf,
> -         PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss)]
> +         PartialEq, SpecifiedValueInfo, ToAnimatedValue, ToAnimatedZero,
> +         ToComputedValue, ToCss)]

Becoming one of the most deriving type :)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251304

> Reading through the whole patch, it is still not clear to me how it makes sense to have a separate generic `List` type and have `T = List<single_value::T>`. Could you explain?

So the key point here is that `T = List<single_value::T>;` but `List<single_value::T>` implements `ToAnimatedValue` (via `derive`).

That way we don't implement it manually in a bunch of places which is what we were doing, incorrectly in others.

> Maybe `throw "Sorry, ..."`.

Not sure I can throw, but can try.

> To be honest, this confused me for quite a while. To some extend, it seems to break the hygienics of naming in macro.
> 
> I'd really prefer you have the macro accept something like
> ```rust
> (<$T:ident> for $ty:ty)
> ```
> and make its users do
> ```rust
> animated_list_impl!(<T> for Vec<T>)
> ```
> which should at least make it clear how the `T` is bound.

That works. I think I can also remove some duplication if I implement it for `[T]` and return an iterator instead, then we just call collect() on the callers... I'll think a bit about it.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251304

> That works. I think I can also remove some duplication if I implement it for `[T]` and return an iterator instead, then we just call collect() on the callers... I'll think a bit about it.

Ah, we can't use impl trait yet I think... I'll file a followup for that.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251304

> Not sure I can throw, but can try.

I cannot :(

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6141abe76bf
Refactor vector types. r=hiro,xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/bead6c2d07ca
Test {background,mask}-{size,position} clamping. r=hiro

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8977321 [details]
Bug 1462829: Refactor vector types.

https://reviewboard.mozilla.org/r/245212/#review251304

> So the key point here is that `T = List<single_value::T>;` but `List<single_value::T>` implements `ToAnimatedValue` (via `derive`).
> 
> That way we don't implement it manually in a bunch of places which is what we were doing, incorrectly in others.

Do you mean, by deriving on the generic type, `List` gets derived only when `single_value::T` implements the trait, so that we don't need to have extra handling for whether `single_value::T` implement it, and can rely on the compiler to automatically figure out?

In that case, this should be clearly documented in the comment, because otherwise I don't see why we need an extra generic type here.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31044a37a94f
followup: Re-disable the position-x / position-y tests since they're not correct. r=me to reopen the CLOSED TREE

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f6141abe76bf
https://hg.mozilla.org/mozilla-central/rev/bead6c2d07ca
https://hg.mozilla.org/mozilla-central/rev/31044a37a94f

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

For comment 17.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/764fd8351b8f
followup: Add a comment regarding why a type is generic. rs=xidorn

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/764fd8351b8f