Closed Bug 1466095 Opened 2 years ago Closed 2 years ago

Make PropertyId::parse less of a footgun.

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:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(1 file)

Bug 1466095: Make PropertyId::parse less of a footgun.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
No description provided.
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.
Attachment #8982497 - Flags: review?(xidorn+moz) → review-
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.
Yeah, sounds good, will amend it.
I think bug 1466008 comment 5 should be a reasonable approach.
Oh, I hadn't got to that email ;). I did basically that yeah.
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...
Attachment #8982497 - Flags: review?(xidorn+moz) → review+
(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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e1f479bfd6
Make PropertyId::parse less of a footgun. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/23e1f479bfd6
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.