From 8417767d09a3d70803b40a575ea96b7d03da5d8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 2 May 2022 23:40:28 +0000 Subject: [PATCH] Bug 1469174 - Refactor media feature expression representation in preparation to support multi-range syntax. r=boris No behavior change. Depends on D145229 Differential Revision: https://phabricator.services.mozilla.com/D145230 --- .../style/queries/feature_expression.rs | 276 ++++++++++-------- .../components/style/values/computed/ratio.rs | 10 + 2 files changed, 159 insertions(+), 127 deletions(-) diff --git a/servo/components/style/queries/feature_expression.rs b/servo/components/style/queries/feature_expression.rs index d93d0cd32c30..bf6a8bf9cdec 100644 --- a/servo/components/style/queries/feature_expression.rs +++ b/servo/components/style/queries/feature_expression.rs @@ -50,7 +50,7 @@ impl FeatureType { /// The kind of matching that should be performed on a feature value. #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToShmem)] -pub enum Range { +enum LegacyRange { /// At least the specified value. Min, /// At most the specified value. @@ -59,7 +59,7 @@ pub enum Range { /// The operator that was specified in this feature. #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToShmem)] -pub enum Operator { +enum Operator { /// = Equal, /// > @@ -87,61 +87,86 @@ impl ToCss for Operator { } } -/// Either a `Range` or an `Operator`. -/// -/// Ranged features are not allowed with operations (that'd make no sense). -#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToShmem)] -pub enum RangeOrOperator { - /// A `Range`. - Range(Range), - /// An `Operator`. - Operator(Operator), +impl Operator { + fn evaluate(&self, cmp: Ordering) -> bool { + match *self { + Operator::Equal => cmp == Ordering::Equal, + Operator::GreaterThan => cmp == Ordering::Greater, + Operator::GreaterThanEqual => cmp == Ordering::Equal || cmp == Ordering::Greater, + Operator::LessThan => cmp == Ordering::Less, + Operator::LessThanEqual => cmp == Ordering::Equal || cmp == Ordering::Less, + } + } } -impl RangeOrOperator { +#[derive(Clone, Debug, MallocSizeOf, ToShmem, PartialEq)] +enum QueryFeatureExpressionKind { + /// Just the media feature name. + Empty, + + /// A single value. + Single(QueryExpressionValue), + + /// Legacy range syntax (min-*: value) or so. + LegacyRange(LegacyRange, QueryExpressionValue), + + /// Modern range context syntax: + /// https://drafts.csswg.org/mediaqueries-5/#mq-range-context + /// + /// TODO: Extend to support as needed. + Range { operator: Operator, value: QueryExpressionValue }, +} + +impl QueryFeatureExpressionKind { /// Evaluate a given range given an optional query value and a value from /// the browser. - fn evaluate(range_or_op: Option, query_value: Option, value: T) -> bool + fn evaluate(&self, context_value: T, mut compute: impl FnMut(&QueryExpressionValue) -> T) -> bool where T: PartialOrd + Zero, { - match query_value { - Some(v) => Self::evaluate_with_query_value(range_or_op, v, value), - None => !value.is_zero(), + match *self { + Self::Empty => return !context_value.is_zero(), + Self::Single(ref value) => { + let value = compute(value); + let cmp = match context_value.partial_cmp(&value) { + Some(c) => c, + None => return false, + }; + cmp == Ordering::Equal + }, + Self::LegacyRange(ref range, ref value) => { + let value = compute(value); + let cmp = match context_value.partial_cmp(&value) { + Some(c) => c, + None => return false, + }; + cmp == Ordering::Equal || + match range { + LegacyRange::Min => cmp == Ordering::Greater, + LegacyRange::Max => cmp == Ordering::Less, + } + } + Self::Range { ref operator, ref value } => { + let value = compute(value); + let cmp = match context_value.partial_cmp(&value) { + Some(c) => c, + None => return false, + }; + operator.evaluate(cmp) + } } } - /// Evaluate a given range given a non-optional query value and a value from - /// the browser. - fn evaluate_with_query_value(range_or_op: Option, query_value: T, value: T) -> bool - where - T: PartialOrd, - { - let cmp = match value.partial_cmp(&query_value) { - Some(c) => c, - None => return false, - }; - - let range_or_op = match range_or_op { - Some(r) => r, - None => return cmp == Ordering::Equal, - }; - - match range_or_op { - RangeOrOperator::Range(range) => { - cmp == Ordering::Equal || - match range { - Range::Min => cmp == Ordering::Greater, - Range::Max => cmp == Ordering::Less, - } - }, - RangeOrOperator::Operator(op) => match op { - Operator::Equal => cmp == Ordering::Equal, - Operator::GreaterThan => cmp == Ordering::Greater, - Operator::GreaterThanEqual => cmp == Ordering::Equal || cmp == Ordering::Greater, - Operator::LessThan => cmp == Ordering::Less, - Operator::LessThanEqual => cmp == Ordering::Equal || cmp == Ordering::Less, - }, + /// Non-ranged features only need to compare to one value at most. + fn non_ranged_value(&self) -> Option<&QueryExpressionValue> { + match *self { + Self::Empty => None, + Self::Single(ref v) => Some(v), + Self::LegacyRange(..) | + Self::Range { .. } => { + debug_assert!(false, "Unexpected ranged value in non-ranged feature!"); + None + } } } } @@ -152,8 +177,7 @@ impl RangeOrOperator { pub struct QueryFeatureExpression { feature_type: FeatureType, feature_index: usize, - value: Option, - range_or_operator: Option, + kind: QueryFeatureExpressionKind, } impl ToCss for QueryFeatureExpression { @@ -163,38 +187,23 @@ impl ToCss for QueryFeatureExpression { { dest.write_str("(")?; - let feature = self.feature(); + self.write_name(dest)?; - if feature - .requirements - .contains(ParsingRequirements::WEBKIT_PREFIX) - { - dest.write_str("-webkit-")?; - } - - if let Some(RangeOrOperator::Range(range)) = self.range_or_operator { - match range { - Range::Min => dest.write_str("min-")?, - Range::Max => dest.write_str("max-")?, + match self.kind { + QueryFeatureExpressionKind::Empty => {}, + QueryFeatureExpressionKind::Single(ref v) | + QueryFeatureExpressionKind::LegacyRange(_, ref v) => { + dest.write_str(": ")?; + v.to_css(dest, self)?; + } + QueryFeatureExpressionKind::Range { ref operator, ref value } => { + dest.write_char(' ')?; + operator.to_css(dest)?; + dest.write_char(' ')?; + value.to_css(dest, self)?; } } - - // NB: CssStringWriter not needed, feature names are under control. - write!(dest, "{}", feature.name)?; - - if let Some(RangeOrOperator::Operator(op)) = self.range_or_operator { - dest.write_char(' ')?; - op.to_css(dest)?; - dest.write_char(' ')?; - } else if self.value.is_some() { - dest.write_str(": ")?; - } - - if let Some(ref val) = self.value { - val.to_css(dest, self)?; - } - - dest.write_str(")") + dest.write_char(')') } } @@ -273,18 +282,41 @@ impl QueryFeatureExpression { fn new( feature_type: FeatureType, feature_index: usize, - value: Option, - range_or_operator: Option, + kind: QueryFeatureExpressionKind, ) -> Self { debug_assert!(feature_index < feature_type.features().len()); Self { feature_type, feature_index, - value, - range_or_operator, + kind, } } + fn write_name(&self, dest: &mut CssWriter) -> fmt::Result + where + W: fmt::Write, + { + let feature = self.feature(); + if feature + .requirements + .contains(ParsingRequirements::WEBKIT_PREFIX) + { + dest.write_str("-webkit-")?; + } + + if let QueryFeatureExpressionKind::LegacyRange(range, _) = self.kind { + match range { + LegacyRange::Min => dest.write_str("min-")?, + LegacyRange::Max => dest.write_str("max-")?, + } + } + + // NB: CssStringWriter not needed, feature names are under control. + write!(dest, "{}", feature.name)?; + + Ok(()) + } + fn feature(&self) -> &'static QueryFeatureDescription { &self.feature_type.features()[self.feature_index] } @@ -307,7 +339,7 @@ impl QueryFeatureExpression { context: &ParserContext, input: &mut Parser<'i, 't>, feature_type: FeatureType, - ) -> Result<(usize, Option), ParseError<'i>> { + ) -> Result<(usize, Option), ParseError<'i>> { let mut requirements = ParsingRequirements::empty(); let location = input.current_source_location(); let ident = input.expect_ident()?; @@ -324,10 +356,10 @@ impl QueryFeatureExpression { let range = if starts_with_ignore_ascii_case(feature_name, "min-") { feature_name = &feature_name[4..]; - Some(Range::Min) + Some(LegacyRange::Min) } else if starts_with_ignore_ascii_case(feature_name, "max-") { feature_name = &feature_name[4..]; - Some(Range::Max) + Some(LegacyRange::Max) } else { None }; @@ -375,20 +407,26 @@ impl QueryFeatureExpression { ); } - return Ok(Self::new(feature_type, feature_index, None, None)); + return Ok(Self::new(feature_type, feature_index, QueryFeatureExpressionKind::Empty)); }, Ok(operator) => operator, }; let feature = &feature_type.features()[feature_index]; - let range_or_operator = match range { + + let value = QueryExpressionValue::parse(feature, context, input).map_err(|err| { + err.location + .new_custom_error(StyleParseErrorKind::MediaQueryExpectedFeatureValue) + })?; + + let kind = match range { Some(range) => { if operator.is_some() { return Err( input.new_custom_error(StyleParseErrorKind::MediaQueryUnexpectedOperator) ); } - Some(RangeOrOperator::Range(range)) + QueryFeatureExpressionKind::LegacyRange(range, value) }, None => match operator { Some(operator) => { @@ -396,77 +434,61 @@ impl QueryFeatureExpression { return Err(input .new_custom_error(StyleParseErrorKind::MediaQueryUnexpectedOperator)); } - Some(RangeOrOperator::Operator(operator)) + QueryFeatureExpressionKind::Range { operator, value } }, - None => None, + None => QueryFeatureExpressionKind::Single(value), }, }; - let value = QueryExpressionValue::parse(feature, context, input).map_err(|err| { - err.location - .new_custom_error(StyleParseErrorKind::MediaQueryExpectedFeatureValue) - })?; - - Ok(Self::new(feature_type, feature_index, Some(value), range_or_operator)) + Ok(Self::new(feature_type, feature_index, kind)) } /// Returns whether this query evaluates to true for the given device. pub fn matches(&self, context: &computed::Context) -> bool { - let value = self.value.as_ref(); - macro_rules! expect { - ($variant:ident) => { - value.map(|value| match *value { + ($variant:ident, $v:expr) => { + match *$v { QueryExpressionValue::$variant(ref v) => v, _ => unreachable!("Unexpected QueryExpressionValue"), - }) + } }; } match self.feature().evaluator { Evaluator::Length(eval) => { - let computed = expect!(Length).map(|specified| { - specified.to_computed_value(context) - }); - let length = eval(context); - RangeOrOperator::evaluate(self.range_or_operator, computed, length) + let v = eval(context); + self.kind.evaluate(v, |v| { + expect!(Length, v).to_computed_value(context) + }) }, Evaluator::Integer(eval) => { - let computed = expect!(Integer).cloned(); - let integer = eval(context); - RangeOrOperator::evaluate(self.range_or_operator, computed, integer) + let v = eval(context); + self.kind.evaluate(v, |v| *expect!(Integer, v)) }, Evaluator::Float(eval) => { - let computed = expect!(Float).cloned(); - let float = eval(context); - RangeOrOperator::evaluate(self.range_or_operator, computed, float) + let v = eval(context); + self.kind.evaluate(v, |v| *expect!(Float, v)) } Evaluator::NumberRatio(eval) => { + let ratio = eval(context); // A ratio of 0/0 behaves as the ratio 1/0, so we need to call used_value() // to convert it if necessary. // FIXME: we may need to update here once // https://github.com/w3c/csswg-drafts/issues/4954 got resolved. - let computed = match expect!(NumberRatio).cloned() { - Some(ratio) => ratio.used_value(), - None => return true, - }; - let ratio = eval(context); - RangeOrOperator::evaluate_with_query_value(self.range_or_operator, computed, ratio) + self.kind.evaluate(ratio, |v| expect!(NumberRatio, v).used_value()) }, Evaluator::Resolution(eval) => { - let computed = expect!(Resolution).map(|specified| { - specified.to_computed_value(context).dppx() - }); - let resolution = eval(context).dppx(); - RangeOrOperator::evaluate(self.range_or_operator, computed, resolution) + let v = eval(context).dppx(); + self.kind.evaluate(v, |v| { + expect!(Resolution, v).to_computed_value(context).dppx() + }) }, Evaluator::Enumerated { evaluator, .. } => { - debug_assert!(self.range_or_operator.is_none(), "Ranges with keywords?"); - evaluator(context, expect!(Enumerated).cloned()) + let computed = self.kind.non_ranged_value().map(|v| *expect!(Enumerated, v)); + evaluator(context, computed) }, Evaluator::BoolInteger(eval) => { - debug_assert!(self.range_or_operator.is_none(), "Ranges with bools?"); - let computed = expect!(BoolInteger).cloned(); + let computed = self.kind.non_ranged_value().map(|v| *expect!(BoolInteger, v)); let boolean = eval(context); computed.map_or(boolean, |v| v == boolean) }, diff --git a/servo/components/style/values/computed/ratio.rs b/servo/components/style/values/computed/ratio.rs index ba40039eae16..ae8997cfc06c 100644 --- a/servo/components/style/values/computed/ratio.rs +++ b/servo/components/style/values/computed/ratio.rs @@ -72,6 +72,16 @@ impl ComputeSquaredDistance for Ratio { } } +impl Zero for Ratio { + fn zero() -> Self { + Self::new(Zero::zero(), One::one()) + } + + fn is_zero(&self) -> bool { + self.0.is_zero() + } +} + impl Ratio { /// Returns a new Ratio. #[inline]