Closed Bug 1317178 Opened 3 years ago Closed 3 years ago

Stylo: Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

Details

Attachments

(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).

[1] https://dxr.mozilla.org/servo/rev/38e97d26c6e812bb2a8ba3f46707625ee733e56e/ports/geckolib/glue.rs#479
[2] https://github.com/servo/servo/issues/13423
Assignee: nobody → boris.chiou
OK. I take it! Thanks.
Priority: -- → P3
Status: NEW → ASSIGNED
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.

e.g.
  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 = declarations.read().single_value_to_css("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.

https://github.com/servo/servo/blob/master/tests/unit/style/properties/serialization.rs#L1015 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.

https://reviewboard.mozilla.org/r/94898/#review95106
Attachment #8813486 - Flags: review?(manishearth) → review+
Comment on attachment 8813144 [details]
Bug 1317178 - Pass the property name to Servo_DeclarationBlock_SerializeOneValue.

https://reviewboard.mozilla.org/r/94672/#review95434

::: 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

s/If/When/
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:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20a89968c1387dc24433d61ca0c857e7bccf329b

Looks like these errors are not related to my patches.
Attachment #8813486 - Attachment is obsolete: true
Attached file Servo PR, #14357
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/085baebf9e4a
Pass the property name to Servo_DeclarationBlock_SerializeOneValue. r=birtles
https://hg.mozilla.org/mozilla-central/rev/085baebf9e4a
Status: ASSIGNED → RESOLVED
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.