Closed
Bug 1466095
Opened 7 years ago
Closed 7 years ago
Make PropertyId::parse less of a footgun.
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
Details
Attachments
(1 file)
No description provided.
Comment hidden (mozreview-request) |
Comment 2•7 years 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•7 years 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•7 years ago
|
||
Yeah, sounds good, will amend it.
Comment 5•7 years ago
|
||
I think bug 1466008 comment 5 should be a reasonable approach.
Assignee | ||
Comment 6•7 years ago
|
||
Oh, I hadn't got to that email ;). I did basically that yeah.
Comment hidden (mozreview-request) |
Comment 8•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•7 years ago
|
Assignee: nobody → emilio
You need to log in
before you can comment on or make changes to this bug.
Description
•