1360659 - stylo: Preserve units in computed Angle

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Currently we are converting all angle units to radians in computed Angle. But both Firefox and Chrome preserve the angle unit and serialize it that way. We need to preserve all units(radian/degree/gradian/turn) too. This causes test failures in test_computed_style.html but they are currently -moz- prefixed and we don't support prefixed gradients. So we need to wait for bug 1358710 to update the expectations.

Servo side issue: https://github.com/servo/servo/issues/16594

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1360659 - stylo: Use computed Angle in computed value of filter property

Using specified Angle was wrong because it can keep calc
value and serialize with calc(). Also that will allow us to pass
angle unit later.

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

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1360659 - stylo: Support other unit types in computed angle

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862970 [details]
Bug 1360659 - stylo: Use computed Angle in computed value of filter property

https://reviewboard.mozilla.org/r/134828/#review137774

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862971 [details]
Bug 1360659 - stylo: Support other unit types in computed angle

https://reviewboard.mozilla.org/r/134830/#review137772

Looks generally ok, but I have a few suggestions.

::: layout/style/ServoBindings.cpp:1681
(Diff revision 1)
>  {
>    return aCSSValue->GetPercentValue();
>  }
>  
>  void
> -Gecko_CSSValue_SetAngle(nsCSSValueBorrowedMut aCSSValue, float aRadians)
> +Gecko_CSSValue_SetAngle(nsCSSValueBorrowedMut aCSSValue, float aRadians,

Why, exactly, do we need all these binding functions?


We have bindings to nsCSSValue in `components/style/gecko_bindings/sugar/ns_css_value.rs`, can we clean them up and avoid FFI calls for these dumb setters/getters?

::: servo/components/style/properties/gecko.mako.rs:2000
(Diff revision 1)
>              # %s substituted with the corresponding variable
>              css_value_setters = {
> -                "length" : "bindings::Gecko_CSSValue_SetAbsoluteLength(%s, %s.0)",
> -                "percentage" : "bindings::Gecko_CSSValue_SetPercentage(%s, %s)",
> -                "lop" : "%s.set_lop(%s)",
> -                "angle" : "bindings::Gecko_CSSValue_SetAngle(%s, %s.radians())",
> +                "length" : "bindings::Gecko_CSSValue_SetAbsoluteLength({0}, {1}.0)",
> +                "percentage" : "bindings::Gecko_CSSValue_SetPercentage({0}, {1})",
> +                "lop" : "{0}.set_lop({1})",
> +                "angle" : "{{ let (value, unit) = {1}.to_gecko_values(); \

Can we add a function to `nsCSSValue` to do this instead of making these take two lines?.

::: servo/components/style/properties/gecko.mako.rs:2002
(Diff revision 1)
> -                "length" : "bindings::Gecko_CSSValue_SetAbsoluteLength(%s, %s.0)",
> -                "percentage" : "bindings::Gecko_CSSValue_SetPercentage(%s, %s)",
> -                "lop" : "%s.set_lop(%s)",
> -                "angle" : "bindings::Gecko_CSSValue_SetAngle(%s, %s.radians())",
> -                "number" : "bindings::Gecko_CSSValue_SetNumber(%s, %s)",
> +                "length" : "bindings::Gecko_CSSValue_SetAbsoluteLength({0}, {1}.0)",
> +                "percentage" : "bindings::Gecko_CSSValue_SetPercentage({0}, {1})",
> +                "lop" : "{0}.set_lop({1})",
> +                "angle" : "{{ let (value, unit) = {1}.to_gecko_values(); \
> +                           bindings::Gecko_CSSValue_SetAngle({0}, value, unit); }}",
> +                "number" : "bindings::Gecko_CSSValue_SetNumber({0}, {1})",

Similarly, I don't think why do we have a function to set a number in `nsCSSValue`, if you don't want to clean it up here, that's fine, but let's ensure we have a followup bug on file to avoid these dumb FFI calls.

::: servo/components/style/values/computed/mod.rs:131
(Diff revision 1)
>  /// A computed `<angle>` value.
>  #[derive(Clone, PartialEq, PartialOrd, Copy, Debug)]
>  #[cfg_attr(feature = "servo", derive(HeapSizeOf, Deserialize, Serialize))]
> -pub struct Angle {
> -    radians: CSSFloat,
> +pub enum Angle {
> +    /// An angle with degree unit
> +    Degree(CSSFloat),

So seems that with this a specified angle is `{ unit, value }`, and a computed angle is an enum, but with the same units.

The only difference is that `specified::Angle` tracks whether it was specified as `Calc`. Could we, perhaps, align both?

One idea is making the specified angle:

```
struct Angle {
    computed: computed::Angle,
    was_calc: bool,
}
```

Does that sound reasonable?

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8862971 [details]
Bug 1360659 - stylo: Support other unit types in computed angle

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

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8862971 [details]
Bug 1360659 - stylo: Support other unit types in computed angle

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862971 [details]
Bug 1360659 - stylo: Support other unit types in computed angle

https://reviewboard.mozilla.org/r/134830/#review137928

::: layout/style/ServoBindings.cpp:1678
(Diff revision 3)
>  
>  float
>  Gecko_CSSValue_GetPercentage(nsCSSValueBorrowed aCSSValue)
>  {
>    return aCSSValue->GetPercentValue();
>  }

Can you file a followup to get rid of these too?

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:177
(Diff revision 3)
>      /// Generic set from any value that implements the ToNsCssValue trait.
>      pub fn set_from<T: ToNsCssValue>(&mut self, value: &T) {
>          value.convert(self)
>      }
> +
> +    /// Returns Angle value.

```
/// Returns an `Angle` value from this `nsCSSValue`.
///
/// Panics if the unit is not `eCSSUnit_Turn`, `eCSSUnit_Radian`, ...
```

(mention the four units we check)

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8862971 [details]
Bug 1360659 - stylo: Support other unit types in computed angle

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

Comment 9

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8862971 [details]
Bug 1360659 - stylo: Support other unit types in computed angle

https://reviewboard.mozilla.org/r/134830/#review137928

> Can you file a followup to get rid of these too?

Filed bug 1361032

Comment 10

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks! Split the servo side here: https://github.com/servo/servo/pull/16674

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/717ed8f6b2ee
stylo: Support other unit types in computed angle r=emilio

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/717ed8f6b2ee