1457635 - Move #[value_info(represents_keyword)] attribute to #[css()] and have #[derive(ToCss)] use it

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from bug 1434130 comment 65)
> Any reason represents_keyword shouldn't live in `css`? It would allow us to
> derive serialization for more stuff.
> 
> I had talked with nox about adding it a while ago actually.
> 
> Even if you don't implement the to_css functionality here, probably keeping
> it as a `css(..)` attribute makes sense, wdyt?

(In reply to Xidorn Quan [:xidorn] from bug 1434130 comment 66)
> I'd prefer not adding stuff not currently working in ToCss into css
> attribute. I think it wouldn't be too hard to move in the future if we do
> want that.

(In reply to Emilio Cobos Álvarez [:emilio] from bug 1434130 comment 67)
> Hmm, ok. Can you file followups for that and ni? me? I'll get to them or
> mentor them otherwise. Thank you!

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1457635: Move represents_keyword to the css attributes.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1457635: Remove values::Verbatim.

No point of having two things that do the same.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8971846 [details]
Bug 1457635: Move represents_keyword to the css attributes.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971846 [details]
Bug 1457635: Move represents_keyword to the css attributes.

https://reviewboard.mozilla.org/r/240596/#review246286

::: servo/components/style_derive/to_css.rs:197
(Diff revision 2)
> +    } else if attrs.represents_keyword {
> +        let ident =
> +            field.ast().ident.as_ref().expect("Unnamed field with represents_keyword?");
> +        let ident = cg::to_css_identifier(ident.as_ref());
> +        quote! {
> +            if *#field {

Why do we need to dereference here?

::: servo/components/style_derive/to_css.rs:206
(Diff revision 2)
>      } else {
>          if attrs.field_bound {
>              let ty = &field.ast().ty;
>              cg::add_predicate(where_clause, parse_quote!(#ty: ::style_traits::ToCss));
>          }
> +

Seems to be unrelated line adding?

::: servo/components/style_derive/to_css.rs:249
(Diff revision 2)
>  pub struct CssFieldAttrs {
>      pub if_empty: Option<String>,
>      pub field_bound: bool,
>      pub iterable: bool,
>      pub skip: bool,
> +    pub represents_keyword: bool,

Please update the document of `ToCss` to include this new field.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8971848 [details]
Bug 1457635: Remove values::Verbatim.

https://reviewboard.mozilla.org/r/240602/#review246288

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Comment on attachment 8971846 [details]
> Bug 1457635: Move represents_keyword to the css attributes.
> 
> https://reviewboard.mozilla.org/r/240596/#review246286
> 
> ::: servo/components/style_derive/to_css.rs:197
> (Diff revision 2)
> > +    } else if attrs.represents_keyword {
> > +        let ident =
> > +            field.ast().ident.as_ref().expect("Unnamed field with represents_keyword?");
> > +        let ident = cg::to_css_identifier(ident.as_ref());
> > +        quote! {
> > +            if *#field {
> 
> Why do we need to dereference here?

The derive code binds by reference by default.

> ::: servo/components/style_derive/to_css.rs:206
> (Diff revision 2)
> >      } else {
> >          if attrs.field_bound {
> >              let ty = &field.ast().ty;
> >              cg::add_predicate(where_clause, parse_quote!(#ty: ::style_traits::ToCss));
> >          }
> > +
> 
> Seems to be unrelated line adding?
> 
> ::: servo/components/style_derive/to_css.rs:249
> (Diff revision 2)
> >  pub struct CssFieldAttrs {
> >      pub if_empty: Option<String>,
> >      pub field_bound: bool,
> >      pub iterable: bool,
> >      pub skip: bool,
> > +    pub represents_keyword: bool,
> 
> Please update the document of `ToCss` to include this new field.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bfee15c5c845
Move represents_keyword to the css attributes. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/3eb5075978e3
Remove values::Verbatim. r=xidorn

Comment 8

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/bfee15c5c845
https://hg.mozilla.org/mozilla-central/rev/3eb5075978e3