1361303 - stylo: custom property name serialization is wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

This is https://github.com/servo/servo/issues/15399.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Correctly serialize CSS Custom Property names.

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

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1361303 - Part 1: Add test for serialization of CSS Custom Property names.

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

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1361303 - Part 2: Update test expectations.

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

Comment 4

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73528eeec38e0ff3c0c30a14494958f37659b536

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8863637 [details]
Correctly serialize CSS Custom Property names.

https://reviewboard.mozilla.org/r/135434/#review138382

::: servo/components/style/properties/properties.mako.rs:813
(Diff revision 1)
>              _ => false,
>          }
>      }
> +
> +    /// Returns the name of the property without CSS escaping.
> +    pub fn name(&self) -> String {

We can probably use `Cow<'static, str>` here, but that probably doesn't matter a lot.

::: servo/components/style/properties/properties.mako.rs:817
(Diff revision 1)
> +                let mut s = String::from("--");
> +                s += name.to_string().as_str();
> +                s

Maybe better
```rust
let mut s = String::new();
write!(&mut s, "--{}", name).unwrap();
s
```
so that we can avoid the allocation from `name.to_string()`.

::: servo/components/style/properties/properties.mako.rs:961
(Diff revision 1)
> +                let mut s = String::from("--");
> +                s += name.to_string().as_str();
> +                s

Ditto.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8863638 [details]
Bug 1361303 - Part 1: Add test for serialization of CSS Custom Property names.

https://reviewboard.mozilla.org/r/135436/#review138392

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8863639 [details]
Bug 1361303 - Part 2: Update test expectations.

https://reviewboard.mozilla.org/r/135438/#review138394

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8863638 [details]
Bug 1361303 - Part 1: Add test for serialization of CSS Custom Property names.

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

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8863639 [details]
Bug 1361303 - Part 2: Update test expectations.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc77087efa2e
Part 1: Add test for serialization of CSS Custom Property names. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/545f5678ceaf
Part 2: Update test expectations. r=xidorn

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc77087efa2e
https://hg.mozilla.org/mozilla-central/rev/545f5678ceaf