1317178 - Stylo: Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 1

[Boris Chiou [:boris]]

OK. I take it! Thanks.

Comment 2

[Manish Goregaokar [:manishearth]]

Will need https://github.com/servo/servo/pull/14104/commits/d002b6c591ff9abb5760ac5e5e46c00d5a7e760a to work correctly.

Comment 3

[Boris Chiou [:boris]]

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?

Comment 4

[Manish Goregaokar [: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.

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1317178 - Pass the property name to Servo_DeclarationBlock_SerializeOneValue.

In order to use single_value_to_css() in
Servo_DeclarationBlock_SerializeOneValue(), we need to pass the property name
and a flag for custom properties.

Review commit: https://reviewboard.mozilla.org/r/94672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94672/

Comment 6

[Boris Chiou [:boris]]

Attachment: [Servo] Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue.

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1317178 - [Servo] Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue.

Review commit: https://reviewboard.mozilla.org/r/94898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94898/

Comment 8

[Boris Chiou [:boris]]

Comment on attachment 8813144 [details]
Bug 1317178 - Pass the property name to Servo_DeclarationBlock_SerializeOneValue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94672/diff/1-2/

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8813486 [details]
Bug 1317178 - [Servo] Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue.

https://reviewboard.mozilla.org/r/94898/#review95106

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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/

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8813144 [details]
Bug 1317178 - Pass the property name to Servo_DeclarationBlock_SerializeOneValue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94672/diff/2-3/

Comment 12

[Boris Chiou [:boris]]

Comment on attachment 8813486 [details]
Bug 1317178 - [Servo] Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94898/diff/1-2/

Comment 13

[Boris Chiou [:boris]]

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.

Comment 14

[Boris Chiou [:boris]]

Attachment: Servo PR, #14357

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/085baebf9e4a
Pass the property name to Servo_DeclarationBlock_SerializeOneValue. r=birtles

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/085baebf9e4a