Closed Bug 1317178 Opened 3 years ago Closed 3 years ago

Stylo: Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue


(Core :: DOM: Animation, defect, P3)




Tracking Status
firefox53 --- fixed


(Reporter: birtles, Assigned: boris)




(2 files, 2 obsolete files)

In bug 1302949 I introduced Servo_DeclarationBlock_SerializeOneValue[1] which does all sorts of hacky things to get a single property value out of a declaration block.

As a follow-up, Manish implemented to_css_single_value[2] to do this more robustly.

So, we should use to_css_single_value in Servo_DeclarationBlock_SerializeOneValue.

Boris, if you're interested in working on animations in Stylo, this would be a very easy bug to get started on. It should be purely rust code but you'll need to build Stylo to test it. Note that most of the animations tests don't pass, but I think some of the tests for getKeyframes() and that's where this is used (I can't remember if the getProperties() tests also work).

Assignee: nobody → boris.chiou
OK. I take it! Thanks.
Priority: -- → P3
Summary: Stylo: Use to_css_single_value in Servo_DeclarationBlock_SerializeOneValue → Stylo: Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue
Just notice one thing:

If the declaration block only has longhand properties, e.g. margin-left and margin-top, I use "margin" to query the value, single_value_to_css() returns Err.

  var effect =
    new KeyframeEffectReadOnly(div,
                               [ { marginLeft: "51px", marginRight: "0px",
                                   marginTop: "10px", marginBottom: "0px" } ,
                                 { marginLeft: "123px", marginRight: "0px",
                                   marginTop: "20px", marginBottom: "0px" } ]);

In Servo_DeclarationBlock_SerializeOneValue():

    let mut string = String::new();
    let x ="margin", &mut string);

x.is_ok() returns false, even though |string| can get a valid serialized string. Is this a bug or a expected returned status?
Flags: needinfo?(manishearth)
I don't think that should be happening. uses the same pattern and passes.
Flags: needinfo?(manishearth)
Attachment #8813145 - Attachment is obsolete: true
Comment on attachment 8813486 [details]
Bug 1317178 - [Servo] Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue.
Attachment #8813486 - Flags: review?(manishearth) → review+
Comment on attachment 8813144 [details]
Bug 1317178 - Pass the property name to Servo_DeclarationBlock_SerializeOneValue.

::: dom/animation/KeyframeEffectReadOnly.cpp:920
(Diff revision 2)
>      JS::Rooted<JSObject*> keyframeObject(aCx, &keyframeJSValue.toObject());
>      for (const PropertyValuePair& propertyValue : keyframe.mPropertyValues) {
> -
> -      const char* name = nsCSSProps::PropertyIDLName(propertyValue.mProperty);
> -
> +      nsAutoString stringValue;
> +      if (propertyValue.mServoDeclarationBlock) {
> +        // FIXME: If we support animations for custom properties on servo, we

Attachment #8813144 - Flags: review?(bbirtles) → review+
I got some weird errors on try pushed by moz-review, so I reverted my patches and push them again:

Looks like these errors are not related to my patches.
Attachment #8813486 - Attachment is obsolete: true
Attached file Servo PR, #14357
Pushed by
Pass the property name to Servo_DeclarationBlock_SerializeOneValue. r=birtles
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.