Bug 1466095: Make PropertyId::parse less of a footgun. r?xidorn
MozReview-Commit-ID: 2BmtSDPmHj9
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -1153,17 +1153,17 @@ impl<'a, 'b, 'i> DeclarationParser<'i> f
type Declaration = Importance;
type Error = StyleParseErrorKind<'i>;
fn parse_value<'t>(
&mut self,
name: CowRcStr<'i>,
input: &mut Parser<'i, 't>,
) -> Result<Importance, ParseError<'i>> {
- let id = match PropertyId::parse(&name) {
+ let id = match PropertyId::parse(&name, self.context) {
Ok(id) => id,
Err(..) => {
return Err(input.new_custom_error(if is_non_mozilla_vendor_identifier(&name) {
StyleParseErrorKind::UnknownVendorProperty
} else {
StyleParseErrorKind::UnknownProperty(name)
}));
}
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1590,17 +1590,17 @@ impl PropertyId {
PropertyId::LonghandAlias(id, _) => id,
_ => return None,
})
}
/// 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, ()> {
// FIXME(https://github.com/rust-lang/rust/issues/33156): remove this
// enum and use PropertyId when stable Rust allows destructors in
// statics.
//
// ShorthandAlias is not used in the Servo build.
// That's why we need to allow dead_code.
#[allow(dead_code)]
pub enum StaticId {
@@ -1634,23 +1634,49 @@ impl PropertyId {
},
Some(&StaticId::LonghandAlias(id, alias)) => {
PropertyId::LonghandAlias(id, alias)
},
Some(&StaticId::ShorthandAlias(id, alias)) => {
PropertyId::ShorthandAlias(id, alias)
},
None => {
- return ::custom_properties::parse_name(property_name).map(|name| {
- PropertyId::Custom(::custom_properties::Name::from(name))
- })
+ let name = ::custom_properties::parse_name(property_name)?;
+ PropertyId::Custom(::custom_properties::Name::from(name))
},
})
}
+ /// Parses a property name, and returns an error if it's unknown or isn't
+ /// enabled for all content.
+ #[inline]
+ pub fn parse_enabled_for_all_content(name: &str) -> Result<Self, ()> {
+ let id = Self::parse_without_enabledness_check(name)?;
+
+ if !id.enabled_for_all_content() {
+ return Err(());
+ }
+
+ Ok(id)
+ }
+
+
+ /// Parses a property name, and returns an error if it's unknown or isn't
+ /// allowed in this context.
+ #[inline]
+ pub fn parse(name: &str, context: &ParserContext) -> Result<Self, ()> {
+ let id = Self::parse_without_enabledness_check(name)?;
+
+ if !id.allowed_in(context) {
+ return Err(());
+ }
+
+ Ok(id)
+ }
+
/// Returns a property id from Gecko's nsCSSPropertyID.
#[cfg(feature = "gecko")]
#[allow(non_upper_case_globals)]
pub fn from_nscsspropertyid(id: nsCSSPropertyID) -> Result<Self, ()> {
use gecko_bindings::structs::*;
match id {
% for property in data.longhands:
${property.nscsspropertyid()} => {
@@ -1710,18 +1736,17 @@ impl PropertyId {
PropertyId::Custom(ref name) => {
let mut s = String::new();
write!(&mut s, "--{}", name).unwrap();
s.into()
}
}
}
- /// Returns the non-custom property id for this property.
- pub fn non_custom_id(&self) -> Option<NonCustomPropertyId> {
+ fn non_custom_id(&self) -> Option<NonCustomPropertyId> {
Some(match *self {
PropertyId::Custom(_) => return None,
PropertyId::Shorthand(shorthand_id) => shorthand_id.into(),
PropertyId::Longhand(longhand_id) => longhand_id.into(),
PropertyId::ShorthandAlias(_, alias_id) => alias_id.into(),
PropertyId::LonghandAlias(_, alias_id) => alias_id.into(),
})
}
@@ -1746,18 +1771,18 @@ impl PropertyId {
// Custom properties are allowed everywhere
None => return true,
Some(id) => id,
};
id.enabled_for_all_content()
}
- /// Returns whether the property is allowed in a given context.
- pub fn allowed_in(&self, context: &ParserContext) -> bool {
+ #[inline]
+ fn allowed_in(&self, context: &ParserContext) -> bool {
let id = match self.non_custom_id() {
// Custom properties are allowed everywhere
None => return true,
Some(id) => id,
};
id.allowed_in(context)
}
@@ -1969,22 +1994,17 @@ impl PropertyDeclaration {
pub fn parse_into<'i, 't>(
declarations: &mut SourcePropertyDeclaration,
id: PropertyId,
name: CowRcStr<'i>,
context: &ParserContext,
input: &mut Parser<'i, 't>,
) -> Result<(), ParseError<'i>> {
assert!(declarations.is_empty());
-
- if !id.allowed_in(context) {
- return Err(input.new_custom_error(
- StyleParseErrorKind::UnknownProperty(name)
- ));
- }
+ debug_assert!(id.allowed_in(context), "{:?}", id);
let start = input.state();
match id {
PropertyId::Custom(property_name) => {
// FIXME: fully implement https://github.com/w3c/csswg-drafts/issues/774
// before adding skip_whitespace here.
// This probably affects some test results.
let value = match input.try(|i| CSSWideKeyword::parse(i)) {
--- a/servo/components/style/stylesheets/keyframes_rule.rs
+++ b/servo/components/style/stylesheets/keyframes_rule.rs
@@ -615,19 +615,22 @@ impl<'a, 'b, 'i> DeclarationParser<'i> f
type Declaration = ();
type Error = StyleParseErrorKind<'i>;
fn parse_value<'t>(
&mut self,
name: CowRcStr<'i>,
input: &mut Parser<'i, 't>,
) -> Result<(), ParseError<'i>> {
- let id = PropertyId::parse(&name).map_err(|()| {
- input.new_custom_error(StyleParseErrorKind::UnknownProperty(name.clone()))
- })?;
+ let id = match PropertyId::parse(&name, self.context) {
+ Ok(id) => id,
+ Err(()) => return Err(input.new_custom_error(
+ StyleParseErrorKind::UnknownProperty(name.clone())
+ )),
+ };
// TODO(emilio): Shouldn't this use parse_entirely?
PropertyDeclaration::parse_into(self.declarations, id, name, self.context, input)?;
// In case there is still unparsed text in the declaration, we should
// roll back.
input.expect_exhausted()?;
--- a/servo/components/style/stylesheets/supports_rule.rs
+++ b/servo/components/style/stylesheets/supports_rule.rs
@@ -310,22 +310,22 @@ impl Declaration {
/// Determine if a declaration parses
///
/// <https://drafts.csswg.org/css-conditional-3/#support-definition>
pub fn eval(&self, context: &ParserContext) -> bool {
debug_assert_eq!(context.rule_type(), CssRuleType::Style);
let mut input = ParserInput::new(&self.0);
let mut input = Parser::new(&mut input);
- input
- .parse_entirely(|input| -> Result<(), CssParseError<()>> {
+ input.parse_entirely(|input| -> Result<(), CssParseError<()>> {
let prop = input.expect_ident_cloned().unwrap();
input.expect_colon().unwrap();
- let id = PropertyId::parse(&prop).map_err(|_| input.new_custom_error(()))?;
+ let id = PropertyId::parse(&prop, context)
+ .map_err(|_| input.new_custom_error(()))?;
let mut declarations = SourcePropertyDeclaration::new();
input.parse_until_before(Delimiter::Bang, |input| {
PropertyDeclaration::parse_into(&mut declarations, id, prop, &context, input)
.map_err(|_| input.new_custom_error(()))
})?;
let _ = input.try(parse_important);
Ok(())
--- a/servo/components/style/values/specified/box.rs
+++ b/servo/components/style/values/specified/box.rs
@@ -439,25 +439,21 @@ fn change_bits_for_longhand(longhand: Lo
}
if property_flags.contains(PropertyFlags::ABSPOS_CB) {
flags |= WillChangeBits::ABSPOS_CB;
}
flags
}
fn change_bits_for_maybe_property(ident: &str, context: &ParserContext) -> WillChangeBits {
- let id = match PropertyId::parse(ident) {
+ let id = match PropertyId::parse(ident, context) {
Ok(id) => id,
Err(..) => return WillChangeBits::empty(),
};
- if !id.allowed_in(context) {
- return WillChangeBits::empty();
- }
-
match id.as_shorthand() {
Ok(shorthand) => {
let mut flags = WillChangeBits::empty();
for longhand in shorthand.longhands() {
flags |= change_bits_for_longhand(longhand);
}
flags
}
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -901,30 +901,22 @@ pub extern "C" fn Servo_ComputedValues_E
Some(v) => Arc::new(v).into_strong(),
None => Strong::null(),
}
}
macro_rules! parse_enabled_property_name {
($prop_name:ident, $found:ident, $default:expr) => {{
let prop_name = $prop_name.as_ref().unwrap().as_str_unchecked();
- // XXX This can be simplified once Option::filter is stable.
- let prop_id = PropertyId::parse(prop_name).ok().and_then(|p| {
- if p.enabled_for_all_content() {
- Some(p)
- } else {
- None
- }
- });
- match prop_id {
- Some(p) => {
+ match PropertyId::parse_enabled_for_all_content(prop_name) {
+ Ok(p) => {
*$found = true;
p
}
- None => {
+ Err(..) => {
*$found = false;
return $default;
}
}
}}
}
#[no_mangle]
@@ -936,17 +928,17 @@ pub unsafe extern "C" fn Servo_Property_
prop_id.is_shorthand()
}
#[no_mangle]
pub unsafe extern "C" fn Servo_Property_IsInherited(
prop_name: *const nsACString,
) -> bool {
let prop_name = prop_name.as_ref().unwrap().as_str_unchecked();
- let prop_id = match PropertyId::parse(prop_name) {
+ let prop_id = match PropertyId::parse_enabled_for_all_content(prop_name) {
Ok(id) => id,
Err(_) => return false,
};
let longhand_id = match prop_id {
PropertyId::Custom(_) => return true,
PropertyId::Longhand(id) |
PropertyId::LonghandAlias(id, _) => id,
PropertyId::Shorthand(id) |
@@ -3471,19 +3463,19 @@ pub extern "C" fn Servo_DeclarationBlock
false
}
})
}
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) {
Ok(property_id) => property_id,
- Err(_) => { return $ret; }
+ Err(_) => return $ret,
}
}}
}
unsafe fn get_property_value(
declarations: RawServoDeclarationBlockBorrowed,
property_id: PropertyId,
value: *mut nsAString,