Closed Bug 1360659 Opened 6 years ago Closed 6 years ago

stylo: Preserve units in computed Angle

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: canova, Assigned: canova)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1360659 - stylo: Support other unit types in computed angle
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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 on attachment 8862970 [details]
Bug 1360659 - stylo: Use computed Angle in computed value of filter property

https://reviewboard.mozilla.org/r/134828/#review137774
Attachment #8862970 - Flags: review?(emilio+bugs) → review+
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?
Attachment #8862971 - Flags: review?(emilio+bugs)
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)
Attachment #8862971 - Flags: review?(emilio+bugs) → review+
Attachment #8862970 - Attachment is obsolete: true
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
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/717ed8f6b2ee
stylo: Support other unit types in computed angle r=emilio
https://hg.mozilla.org/mozilla-central/rev/717ed8f6b2ee
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.