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

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 <value> <op> <name> <op> <value> 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<T>(range_or_op: Option<Self>, query_value: Option<T>, value: T) -> bool
+    fn evaluate<T>(&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<T>(range_or_op: Option<Self>, 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<QueryExpressionValue>,
-    range_or_operator: Option<RangeOrOperator>,
+    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<QueryExpressionValue>,
-        range_or_operator: Option<RangeOrOperator>,
+        kind: QueryFeatureExpressionKind,
     ) -> Self {
         debug_assert!(feature_index < feature_type.features().len());
         Self {
             feature_type,
             feature_index,
-            value,
-            range_or_operator,
+            kind,
         }
     }
 
+    fn write_name<W>(&self, dest: &mut CssWriter<W>) -> 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<Range>), ParseError<'i>> {
+    ) -> Result<(usize, Option<LegacyRange>), 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)
             },

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]