Support `transform` values in stylo

Currently only supports skew, and needs style cleanup. I need to do some small refactorings too. One thing I'm not sure about is how I'm handling the array and the linked list. I could expose an API over nsCSSValueList and wrap it as an iterator on the rust side, and I could deal with nsCSSValue::Array directly. This was the simplest (but roughest) so I started with this, but I could change it. I'm not very sure what the best way to do this is, and since we won't use nscssvalue more than one, there's not that much benefit of generic bindings.

All transform functions implemented. I still need to do some cleanups in the functions from styleanimationvalue I made public, but it's mostly ready for review. The whole glue code is basically one big unsafe block. I could abstract most of it away, but since this is the only time we're ever going to deal with nsCSSValue it amounts to breaking one big unsafe block into many smaller ones. I instead went down the route of autogenerating most of the code in that function.

Comment on attachment 8806206 [details] Bug 1314200 - stylo: support transform; Bouncing this one to heycam after all. Looks reasonable on the whole but I'm not super familiar with Gecko specified values and so I'd need to do more research to review this. Birtles might also be able to review if heycam is swamped.

Comment on attachment 8806206 [details] Bug 1314200 - stylo: support transform; https://reviewboard.mozilla.org/r/89688/#review89986 r=me with the below addressed. ::: layout/style/ServoBindings.h:259 (Diff revision 2) > +nsCSSValueSharedList* Gecko_NewCSSValueSharedList(uint32_t len); > +void Gecko_SetAbsoluteLength_CSSValue(nsCSSValueBorrowedMut css_value, int32_t len); > +void Gecko_SetNumber_CSSValue(nsCSSValueBorrowedMut css_value, float number); > +void Gecko_SetKeyword_CSSValue(nsCSSValueBorrowedMut css_value, nsCSSKeyword keyword); > +void Gecko_SetPercentage_CSSValue(nsCSSValueBorrowedMut css_value, float percent); > +void Gecko_SetAngle_CSSValue(nsCSSValueBorrowedMut css_value, float radians); > +void Gecko_SetCalc_CSSValue(nsCSSValueBorrowedMut css_value, nsStyleCoord::CalcValue calc); > +void Gecko_SetArray_CSSValue(nsCSSValueBorrowedMut css_value, int32_t len); > +nsCSSValueBorrowedMut Gecko_GetArrayItem_CSSValue(nsCSSValueBorrowedMut css_value, int32_t index); > +NS_DECL_THREADSAFE_FFI_REFCOUNTING(nsCSSValueSharedList, CSSValueSharedList); > + I know we're not completely consistent yet, but should we call these Gecko_CSSValue_SetXXX instead, in line with Xidorn's suggested naming scheme? ::: layout/style/ServoBindings.h:260 (Diff revision 2) > > nsStyleQuoteValues* Gecko_NewStyleQuoteValues(uint32_t len); > NS_DECL_THREADSAFE_FFI_REFCOUNTING(nsStyleQuoteValues, QuoteValues); > > +nsCSSValueSharedList* Gecko_NewCSSValueSharedList(uint32_t len); > +void Gecko_SetAbsoluteLength_CSSValue(nsCSSValueBorrowedMut css_value, int32_t len); Can we use nscoord instead of int32_t? ::: layout/style/ServoBindings.h:265 (Diff revision 2) > +void Gecko_SetAbsoluteLength_CSSValue(nsCSSValueBorrowedMut css_value, int32_t len); > +void Gecko_SetNumber_CSSValue(nsCSSValueBorrowedMut css_value, float number); > +void Gecko_SetKeyword_CSSValue(nsCSSValueBorrowedMut css_value, nsCSSKeyword keyword); > +void Gecko_SetPercentage_CSSValue(nsCSSValueBorrowedMut css_value, float percent); > +void Gecko_SetAngle_CSSValue(nsCSSValueBorrowedMut css_value, float radians); > +void Gecko_SetCalc_CSSValue(nsCSSValueBorrowedMut css_value, nsStyleCoord::CalcValue calc); Nit: one too many spaces before "calc". ::: layout/style/ServoBindings.cpp:972 (Diff revision 2) > return values.forget().take(); > } > > NS_IMPL_THREADSAFE_FFI_REFCOUNTING(nsStyleQuoteValues, QuoteValues); > > +nsCSSValueSharedList* Gecko_NewCSSValueSharedList(uint32_t aLen) Nit: newline before "Gecko_NewCSSValueSharedList". Similarly for the other functions added below. ::: layout/style/ServoBindings.cpp:988 (Diff revision 2) > + cur = cur->mNext; > + } > + > + return list.forget().take(); > +} > +void Gecko_SetAbsoluteLength_CSSValue(nsCSSValueBorrowedMut aCSSValue, int32_t aLen) Nit: blank line between functions, please. ::: layout/style/ServoBindings.cpp:1002 (Diff revision 2) > +{ > + aCSSValue->SetIntValue(aKeyword, eCSSUnit_Enumerated); > +} > +void Gecko_SetPercentage_CSSValue(nsCSSValueBorrowedMut aCSSValue, float aPercent) > +{ > + aCSSValue->SetFloatValue(aPercent, eCSSUnit_Number); Do you think it's better to have a single FFI function for all SetFloatValue calls, and to pass in the nsCSSUnit value? Happy either way. ::: layout/style/ServoBindings.cpp:1010 (Diff revision 2) > +{ > + aCSSValue->SetFloatValue(aRadians, eCSSUnit_Radian); > +} > +void Gecko_SetCalc_CSSValue(nsCSSValueBorrowedMut aCSSValue, nsStyleCoord::CalcValue aCalc) > +{ > + mozilla::CalcValueToCSSValue(&aCalc, *aCSSValue); Drop the "mozilla::" since we have a "using namespace mozilla" at the top of the file. ::: layout/style/ServoBindings.cpp:1012 (Diff revision 2) > +} > +void Gecko_SetCalc_CSSValue(nsCSSValueBorrowedMut aCSSValue, nsStyleCoord::CalcValue aCalc) > +{ > + mozilla::CalcValueToCSSValue(&aCalc, *aCSSValue); > +} > +void Gecko_SetArray_CSSValue(nsCSSValueBorrowedMut aCSSValue, int32_t aLen) Call this Gecko_SetFunction_CSSValue (or Gecko_CSSValue_SetFunction) since there are various nsCSSValue unit types that have an array value. ::: layout/style/StyleAnimationValue.cpp:274 (Diff revision 2) > arr = aArray; > } > return arr.forget(); > } > > +// todo move to nsStyleCoord File a bug to capture this? ::: layout/style/StyleAnimationValue.cpp:275 (Diff revision 2) > } > return arr.forget(); > } > > +// todo move to nsStyleCoord > inline void Remove the inline, since we're exposing this function to other files, and the function body isn't in the header. ::: servo/components/style/binding_tools/regen.py:274 (Diff revision 2) > + "nsCSSValueSharedList", > + "nsCSSValue", Nit: the other way around to keep this sorted. ::: servo/components/style/binding_tools/regen.py:551 (Diff revision 2) > flags.append("--raw-line") > flags.append("pub type {0}Borrowed<'a> = &'a {0};".format(ty)) > zero_size_type(ty, flags) > > if "servo_immutable_borrow_types" in current_target: > - for ty in current_target["servo_immutable_borrow_types"]: > + for ty in current_target["servo_immutable_borrow_types"] + current_target["servo_borrow_types"]: This will raise an exception if servo_borrow_types isn't defined. Maybe: for ty in current_target.get("servo_immutable_borrow_types", []) + current_target.get("servo_borrow_types", []): and then just drop the "has" check for servo_immutable_borrow_types. ::: servo/components/style/binding_tools/regen.py:570 (Diff revision 2) > + flags.append("pub type {0}BorrowedMutOrNull<'a> = \ > +Option<&'a mut {0}>;".format(ty)) Nit: just put this on one line? There are a bunch of other long lines around here. ::: servo/components/style/properties/gecko.mako.rs:982 (Diff revision 2) > + pattern = ", ".join([b + str(a+1) for (a,b) in enumerate(items)]) > + if name == "matrix": > + # m11, m12, m13, .. > + indices = [str(i) + str(j) for i in range(1, 5) for j in range(1, 5)] > + # m12: item1 > + single_patterns = ["m%s: number%s" % (index, i + 1) for (i, index) in enumerate(indices)] > + pattern = "ComputedMatrix { %s }" % ", ".join(single_patterns) How about: if name == "matrix": ... pattern = ... else: pattern = ", ".join(...) rather than setting the pattern for the non-matrix cases and then overwriting it. ::: servo/components/style/properties/gecko.mako.rs:986 (Diff revision 2) > + # Generate contents of pattern from items > + pattern = ", ".join([b + str(a+1) for (a,b) in enumerate(items)]) > + if name == "matrix": > + # m11, m12, m13, .. > + indices = [str(i) + str(j) for i in range(1, 5) for j in range(1, 5)] > + # m12: item1 Should this be: # m11: number1, m12: number2, ... ? ::: servo/components/style/properties/gecko.mako.rs:992 (Diff revision 2) > + single_patterns = ["m%s: number%s" % (index, i + 1) for (i, index) in enumerate(indices)] > + pattern = "ComputedMatrix { %s }" % ", ".join(single_patterns) > + > + # First %s substituted with the call to GetArrayItem, the second > + # %s substituted with the corresponding variable > + function_names = { Since these are whole function calls, not just function name, should we call this something else, like "css_value_setters"? ::: servo/components/style/properties/gecko.mako.rs:1056 (Diff revision 2) > + ${transform_function_arm("matrix", "matrix3d", ["number"] * 16)} > + ${transform_function_arm("skew", "skew", ["angle"] * 2)} > + ${transform_function_arm("translate", "translate3d", ["lop", "lop", "length"])} > + ${transform_function_arm("scale", "scale3d", ["number"] * 3)} > + ${transform_function_arm("rotate", "rotate3d", ["number"] * 3 + ["angle"])} > + ${transform_function_arm("perspective", "perspective", ["length"])} If we don't have one already, can you file an issue on Servo to store the computed value as described by the spec (i.e. preserving the translation kind, etc., and not filling in implied values)? ::: servo/components/style/properties/gecko.mako.rs:1066 (Diff revision 2) > + ${transform_function_arm("perspective", "perspective", ["length"])} > + } > + cur = (*cur).mNext; > + } > + } > + Should we debug_assert! here that iter doesn't have any more items? ::: servo/components/style/properties/gecko.mako.rs:1067 (Diff revision 2) > + } > + cur = (*cur).mNext; > + } > + } > + > + unsafe {self.gecko.mSpecifiedTransform.set_move(list)}; Nit: spaces within the {}s.

Comment on attachment 8806206 [details] Bug 1314200 - stylo: support transform; https://reviewboard.mozilla.org/r/89688/#review90164 ::: layout/style/StyleAnimationValue.cpp:274 (Diff revision 2) > arr = aArray; > } > return arr.forget(); > } > > +// todo move to nsStyleCoord Nah, this is work that I plan to do as part of this bug itself. ::: servo/components/style/properties/gecko.mako.rs:1056 (Diff revision 2) > + ${transform_function_arm("matrix", "matrix3d", ["number"] * 16)} > + ${transform_function_arm("skew", "skew", ["angle"] * 2)} > + ${transform_function_arm("translate", "translate3d", ["lop", "lop", "length"])} > + ${transform_function_arm("scale", "scale3d", ["number"] * 3)} > + ${transform_function_arm("rotate", "rotate3d", ["number"] * 3 + ["angle"])} > + ${transform_function_arm("perspective", "perspective", ["length"])} Is this necessary? AIUI storing it as a 3d transformation doesn't lose any important information (except perhaps for `getComputedValues()`?), and the animations do a mathematical check to see if the transformation is a 2d or 3d one. ::: servo/components/style/properties/gecko.mako.rs:1066 (Diff revision 2) > + ${transform_function_arm("perspective", "perspective", ["length"])} > + } > + cur = (*cur).mNext; > + } > + } > + Good idea. Should we make the `expect()` crash above to be a debug only one as well?

Comment on attachment 8806206 [details] Bug 1314200 - stylo: support transform; https://reviewboard.mozilla.org/r/89688/#review89986 > Do you think it's better to have a single FFI function for all SetFloatValue calls, and to pass in the nsCSSUnit value? Happy either way. Was thinking about this too, don't really have a preference either. > If we don't have one already, can you file an issue on Servo to store the computed value as described by the spec (i.e. preserving the translation kind, etc., and not filling in implied values)? Is this necessary? AIUI storing it as a 3d transformation doesn't lose any important information (except perhaps for getComputedValues()?), and the animations do a mathematical check to see if the transformation is a 2d or 3d one.

Servo bug at https://github.com/servo/servo/pull/14168

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0d8346f8bbcb stylo: support transform; r=heycam