1466095 - Make PropertyId::parse less of a footgun.

Status: RESOLVED FIXED

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1466095: Make PropertyId::parse less of a footgun.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8982497 [details]
Bug 1466095: Make PropertyId::parse less of a footgun.

https://reviewboard.mozilla.org/r/248474/#review254692

I don't think having an `Option<ParserContext>` is the right way to go. It isn't immediately clear from the callsite what `None` really means.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8982497 [details]
Bug 1466095: Make PropertyId::parse less of a footgun.

https://reviewboard.mozilla.org/r/248474/#review254692

I mean, it isn't clear from the callsite what `None` means in terms of enabled state. It can mean parse anything regardless of the environment, or parse only those enabled everywhere.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah, sounds good, will amend it.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

I think bug 1466008 comment 5 should be a reasonable approach.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Oh, I hadn't got to that email ;). I did basically that yeah.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8982497 [details]
Bug 1466095: Make PropertyId::parse less of a footgun.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8982497 [details]
Bug 1466095: Make PropertyId::parse less of a footgun.

https://reviewboard.mozilla.org/r/248474/#review254696

::: servo/components/style/properties/properties.mako.rs:1598
(Diff revision 2)
>      }
>  
>      /// Returns a given property from the string `s`.
>      ///
>      /// Returns Err(()) for unknown non-custom properties.
> -    pub fn parse(property_name: &str) -> Result<Self, ()> {
> +    fn parse_without_enabledness_check(property_name: &str) -> Result<Self, ()> {

`enabledness` sounds... weird.

`parse_ignoring_enabled_state` matches `nsCSSProps`'s wording of `EnabledState::eIgnoreEnabledState`. Or maybe `parse_unchecked` is enough serving as a warning.

::: servo/components/style/properties/properties.mako.rs:1779
(Diff revision 2)
>          };
>  
>          id.enabled_for_all_content()
>      }
>  
> -    /// Returns whether the property is allowed in a given context.
> +    #[inline]

This `inline` is probably not necessary... LLVM should be able to decide what should be inlined when inside the same crate. IIRC the only use case for `#[inline]` is for non-generic function which can be used in a foreign crate, which is apparently not a case for this function.

::: servo/ports/geckolib/glue.rs:3471
(Diff revision 2)
>  }
>  
>  macro_rules! get_property_id_from_property {
>      ($property: ident, $ret: expr) => {{
>          let property = $property.as_ref().unwrap().as_str_unchecked();
> -        match PropertyId::parse(property.into()) {
> +        match PropertyId::parse_enabled_for_all_content(property) {

So we are losing the ability to manipulate chrome-only properties via CSSOM, for both good and bad.

If it doesn't break any chrome code, I guess it's fine...

This does feel a bit scary that we allow setting chrome-only or even ua-only properties via CSSOM currently...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> Comment on attachment 8982497 [details]
> Bug 1466095: Make PropertyId::parse less of a footgun.
> 
> https://reviewboard.mozilla.org/r/248474/#review254696
> 
> ::: servo/components/style/properties/properties.mako.rs:1598
> (Diff revision 2)
> >      }
> >  
> >      /// Returns a given property from the string `s`.
> >      ///
> >      /// Returns Err(()) for unknown non-custom properties.
> > -    pub fn parse(property_name: &str) -> Result<Self, ()> {
> > +    fn parse_without_enabledness_check(property_name: &str) -> Result<Self, ()> {
> 
> `enabledness` sounds... weird.
> 
> `parse_ignoring_enabled_state` matches `nsCSSProps`'s wording of
> `EnabledState::eIgnoreEnabledState`. Or maybe `parse_unchecked` is enough
> serving as a warning.

Yeah, I like unchecked, will change.

> ::: servo/components/style/properties/properties.mako.rs:1779
> (Diff revision 2)
> >          };
> >  
> >          id.enabled_for_all_content()
> >      }
> >  
> > -    /// Returns whether the property is allowed in a given context.
> > +    #[inline]
> 
> This `inline` is probably not necessary... LLVM should be able to decide
> what should be inlined when inside the same crate. IIRC the only use case
> for `#[inline]` is for non-generic function which can be used in a foreign
> crate, which is apparently not a case for this function.

Sure.

> ::: servo/ports/geckolib/glue.rs:3471
> (Diff revision 2)
> >  }
> >  
> >  macro_rules! get_property_id_from_property {
> >      ($property: ident, $ret: expr) => {{
> >          let property = $property.as_ref().unwrap().as_str_unchecked();
> > -        match PropertyId::parse(property.into()) {
> > +        match PropertyId::parse_enabled_for_all_content(property) {
> 
> So we are losing the ability to manipulate chrome-only properties via CSSOM,
> for both good and bad.
> 
> If it doesn't break any chrome code, I guess it's fine...
> 
> This does feel a bit scary that we allow setting chrome-only or even ua-only
> properties via CSSOM currently...

This is not true, I added a test for this (test_non_content_accessible_properties.html).

It passes right now because of: https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/layout/style/nsDOMCSSDeclaration.cpp#192

That code could get some cleanup.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e1f479bfd6
Make PropertyId::parse less of a footgun. r=xidorn

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/23e1f479bfd6