Closed
Bug 1360659
Opened 6 years ago
Closed 6 years ago
stylo: Preserve units in computed Angle
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: canova, Assigned: canova)
References
Details
Attachments
(1 file, 1 obsolete file)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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 4•6 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8862970 -
Attachment is obsolete: true
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 10•6 years ago
|
||
Thanks! Split the servo side here: https://github.com/servo/servo/pull/16674
Comment 11•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/717ed8f6b2ee stylo: Support other unit types in computed angle r=emilio
Comment 12•6 years ago
|
||
bugherder |
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.
Description
•