Make PropertyId::parse less of a footgun.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)

Comment 2

11 months ago
mozreview-review
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 3

11 months ago
mozreview-review-reply
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.
(Assignee)

Comment 4

11 months ago
Yeah, sounds good, will amend it.
I think bug 1466008 comment 5 should be a reasonable approach.
(Assignee)

Comment 6

11 months ago
Oh, I hadn't got to that email ;). I did basically that yeah.
Comment hidden (mozreview-request)

Comment 8

11 months ago
mozreview-review
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+
(Assignee)

Comment 9

11 months ago
(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

11 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e1f479bfd6
Make PropertyId::parse less of a footgun. r=xidorn

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23e1f479bfd6
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.