1466645 - PropertyId::name & users could be nicer.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

We only use it in error paths.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1466645: Avoid useless allocations in custom property name serialization.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1466645: Remove PropertyId::name.

It's only used for the error path in property parsing, so most of the time is
not useful.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1466645: Make getting a property name explicitly an indexing operation.

The six milliseconds spent in Olli's profile make me thing this is not getting
optimized and we expected.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Just realised that serialize_identifier for custom properties doesn't work, and produces a wrong result with properties like `--0`.

Will fix, though it requires cssparser changes.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8983161 [details]
Bug 1466645: Avoid useless allocations in custom property name serialization.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8983163 [details]
Bug 1466645: Make getting a property name explicitly an indexing operation.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8983162 [details]
Bug 1466645: Remove PropertyId::name.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983161 [details]
Bug 1466645: Avoid useless allocations in custom property name serialization.

https://reviewboard.mozilla.org/r/249016/#review255686

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983162 [details]
Bug 1466645: Remove PropertyId::name.

https://reviewboard.mozilla.org/r/249018/#review255688

::: servo/components/style_traits/lib.rs:180
(Diff revision 2)
> -    pub fn new_invalid(name: CowRcStr<'i>, value_error: ParseError<'i>) -> ParseError<'i> {
> +    pub fn new_invalid<S>(name: S, value_error: ParseError<'i>) -> ParseError<'i>
> +    where
> +        S: Into<CowRcStr<'i>>,

Maybe you can just have `new_invalid` accept a `&PropertyId` instead, which should simplify your code above.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983163 [details]
Bug 1466645: Make getting a property name explicitly an indexing operation.

https://reviewboard.mozilla.org/r/249020/#review255692

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfc3d86b042
Avoid useless allocations in custom property name serialization. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5c7353cfa4
Make getting a property name explicitly an indexing operation. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/521224a55cbe
Remove PropertyId::name. r=xidorn

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> Comment on attachment 8983162 [details]
> Bug 1466645: Remove PropertyId::name.
> 
> https://reviewboard.mozilla.org/r/249018/#review255688
> 
> ::: servo/components/style_traits/lib.rs:180
> (Diff revision 2)
> > -    pub fn new_invalid(name: CowRcStr<'i>, value_error: ParseError<'i>) -> ParseError<'i> {
> > +    pub fn new_invalid<S>(name: S, value_error: ParseError<'i>) -> ParseError<'i>
> > +    where
> > +        S: Into<CowRcStr<'i>>,
> 
> Maybe you can just have `new_invalid` accept a `&PropertyId` instead, which
> should simplify your code above.

I cannot because it lives in style_traits.

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7bfc3d86b042
https://hg.mozilla.org/mozilla-central/rev/9f5c7353cfa4
https://hg.mozilla.org/mozilla-central/rev/521224a55cbe