Closed Bug 1457635 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: emilio)

References

Details

Attachments

(2 files)

Bug 1457635: Move represents_keyword to the css attributes.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457635: Remove values::Verbatim.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
(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!
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Flags: needinfo?(emilio)
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.
Attachment #8971846 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8971848 [details]
Bug 1457635: Remove values::Verbatim.

https://reviewboard.mozilla.org/r/240602/#review246288
Attachment #8971848 - Flags: review?(xidorn+moz) → review+
(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.
Pushed by ecoal95@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/bfee15c5c845
https://hg.mozilla.org/mozilla-central/rev/3eb5075978e3
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.