Closed
Bug 1466095
Opened 6 years ago
Closed 6 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•6 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•6 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•6 years ago
|
||
Yeah, sounds good, will amend it.
Comment 5•6 years ago
|
||
I think bug 1466008 comment 5 should be a reasonable approach.
Assignee | ||
Comment 6•6 years ago
|
||
Oh, I hadn't got to that email ;). I did basically that yeah.
Comment hidden (mozreview-request) |
Comment 8•6 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•6 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•6 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/23e1f479bfd6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Assignee: nobody → emilio
You need to log in
before you can comment on or make changes to this bug.
Description
•