style: Simplify media query evaluation code a bit

This patch:

  * Removes generic <ident> support for media features. These were used
    for some privileged media features but are no longer used.

  * Simplifies media feature getters by shifting the responsibility of
    dealing with RangeOrOperator to the caller. This makes it easier to
    implement container-query / mediaqueries-4 syntax, and also cleans up
    the code a bunch.

There should be no change in behavior.

Differential Revision: https://phabricator.services.mozilla.com/D144051

components/style/gecko/media_features.rs

@@ -8,7 +8,6 @@ use crate::gecko_bindings::bindings;
 use crate::gecko_bindings::structs;
 use crate::media_queries::media_feature::{AllowsRanges, ParsingRequirements};
 use crate::media_queries::media_feature::{Evaluator, MediaFeatureDescription};
-use crate::media_queries::media_feature_expression::RangeOrOperator;
 use crate::media_queries::{Device, MediaType};
 use crate::values::computed::CSSPixelLength;
 use crate::values::computed::Ratio;
@@ -26,114 +25,46 @@ fn device_size(device: &Device) -> Size2D<Au> {
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#width
-fn eval_width(
+fn eval_width(device: &Device) -> CSSPixelLength {
-    device: &Device,
+    CSSPixelLength::new(device.au_viewport_size().width.to_f32_px())
-    value: Option<CSSPixelLength>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    RangeOrOperator::evaluate(
-        range_or_operator,
-        value.map(Au::from),
-        device.au_viewport_size().width,
-    )
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#device-width
-fn eval_device_width(
+fn eval_device_width(device: &Device) -> CSSPixelLength {
-    device: &Device,
+    CSSPixelLength::new(device_size(device).width.to_f32_px())
-    value: Option<CSSPixelLength>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    RangeOrOperator::evaluate(
-        range_or_operator,
-        value.map(Au::from),
-        device_size(device).width,
-    )
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#height
-fn eval_height(
+fn eval_height(device: &Device) -> CSSPixelLength {
-    device: &Device,
+    CSSPixelLength::new(device.au_viewport_size().height.to_f32_px())
-    value: Option<CSSPixelLength>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    RangeOrOperator::evaluate(
-        range_or_operator,
-        value.map(Au::from),
-        device.au_viewport_size().height,
-    )
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#device-height
-fn eval_device_height(
+fn eval_device_height(device: &Device) -> CSSPixelLength {
-    device: &Device,
+    CSSPixelLength::new(device_size(device).height.to_f32_px())
-    value: Option<CSSPixelLength>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    RangeOrOperator::evaluate(
-        range_or_operator,
-        value.map(Au::from),
-        device_size(device).height,
-    )
 }
 
-fn eval_aspect_ratio_for<F>(
+fn eval_aspect_ratio_for<F>(device: &Device, get_size: F) -> Ratio
-    device: &Device,
-    query_value: Option<Ratio>,
-    range_or_operator: Option<RangeOrOperator>,
-    get_size: F,
-) -> bool
 where
     F: FnOnce(&Device) -> Size2D<Au>,
 {
-    // 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 query_value = match query_value {
-        Some(v) => v.used_value(),
-        None => return true,
-    };
-
     let size = get_size(device);
-    let value = Ratio::new(size.width.0 as f32, size.height.0 as f32);
+    Ratio::new(size.width.0 as f32, size.height.0 as f32)
-    RangeOrOperator::evaluate_with_query_value(range_or_operator, query_value, value)
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#aspect-ratio
-fn eval_aspect_ratio(
+fn eval_aspect_ratio(device: &Device) -> Ratio {
-    device: &Device,
+    eval_aspect_ratio_for(device, Device::au_viewport_size)
-    query_value: Option<Ratio>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    eval_aspect_ratio_for(
-        device,
-        query_value,
-        range_or_operator,
-        Device::au_viewport_size,
-    )
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#device-aspect-ratio
-fn eval_device_aspect_ratio(
+fn eval_device_aspect_ratio(device: &Device) -> Ratio {
-    device: &Device,
+    eval_aspect_ratio_for(device, device_size)
-    query_value: Option<Ratio>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    eval_aspect_ratio_for(device, query_value, range_or_operator, device_size)
 }
 
 /// https://compat.spec.whatwg.org/#css-media-queries-webkit-device-pixel-ratio
-fn eval_device_pixel_ratio(
+fn eval_device_pixel_ratio(device: &Device) -> f32 {
-    device: &Device,
+    eval_resolution(device).dppx()
-    query_value: Option<f32>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    eval_resolution(
-        device,
-        query_value.map(Resolution::from_dppx),
-        range_or_operator,
-    )
 }
 
 #[derive(Clone, Copy, Debug, FromPrimitive, Parse, ToCss)]
@@ -192,17 +123,15 @@ fn eval_display_mode(device: &Device, query_value: Option<DisplayMode>) -> bool
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#grid
-fn eval_grid(_: &Device, query_value: Option<bool>, _: Option<RangeOrOperator>) -> bool {
+fn eval_grid(_: &Device) -> bool {
     // Gecko doesn't support grid devices (e.g., ttys), so the 'grid' feature
     // is always 0.
-    let supports_grid = false;
+    false
-    query_value.map_or(supports_grid, |v| v == supports_grid)
 }
 
 /// https://compat.spec.whatwg.org/#css-media-queries-webkit-transform-3d
-fn eval_transform_3d(_: &Device, query_value: Option<bool>, _: Option<RangeOrOperator>) -> bool {
+fn eval_transform_3d(_: &Device) -> bool {
-    let supports_transforms = true;
+    true
-    query_value.map_or(supports_transforms, |v| v == supports_transforms)
 }
 
 #[derive(Clone, Copy, Debug, FromPrimitive, Parse, ToCss)]
@@ -220,51 +149,26 @@ fn eval_scan(_: &Device, _: Option<Scan>) -> bool {
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#color
-fn eval_color(
+fn eval_color(device: &Device) -> u32 {
-    device: &Device,
+    unsafe { bindings::Gecko_MediaFeatures_GetColorDepth(device.document()) }
-    query_value: Option<u32>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
-    let color_bits_per_channel =
-        unsafe { bindings::Gecko_MediaFeatures_GetColorDepth(device.document()) };
-    RangeOrOperator::evaluate(range_or_operator, query_value, color_bits_per_channel)
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#color-index
-fn eval_color_index(
+fn eval_color_index(_: &Device) -> u32 {
-    _: &Device,
-    query_value: Option<u32>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
     // We should return zero if the device does not use a color lookup table.
-    let index = 0;
+    0
-    RangeOrOperator::evaluate(range_or_operator, query_value, index)
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#monochrome
-fn eval_monochrome(
+fn eval_monochrome(device: &Device) -> u32 {
-    device: &Device,
-    query_value: Option<u32>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
     // For color devices we should return 0.
-    let depth =
+    unsafe { bindings::Gecko_MediaFeatures_GetMonochromeBitsPerPixel(device.document()) }
-        unsafe { bindings::Gecko_MediaFeatures_GetMonochromeBitsPerPixel(device.document()) };
-    RangeOrOperator::evaluate(range_or_operator, query_value, depth)
 }
 
 /// https://drafts.csswg.org/mediaqueries-4/#resolution
-fn eval_resolution(
+fn eval_resolution(device: &Device) -> Resolution {
-    device: &Device,
-    query_value: Option<Resolution>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool {
     let resolution_dppx = unsafe { bindings::Gecko_MediaFeatures_GetResolution(device.document()) };
-    RangeOrOperator::evaluate(
+    Resolution::from_dppx(resolution_dppx)
-        range_or_operator,
-        query_value.map(|r| r.dppx()),
-        resolution_dppx,
-    )
 }
 
 #[derive(Clone, Copy, Debug, FromPrimitive, Parse, ToCss)]
@@ -540,45 +444,24 @@ fn eval_any_hover(device: &Device, query_value: Option<Hover>) -> bool {
     eval_hover_capabilities(query_value, all_pointer_capabilities(device))
 }
 
-fn eval_moz_is_glyph(
+fn eval_moz_is_glyph(device: &Device) -> bool {
-    device: &Device,
+    device.document().mIsSVGGlyphsDocument()
-    query_value: Option<bool>,
-    _: Option<RangeOrOperator>,
-) -> bool {
-    let is_glyph = device.document().mIsSVGGlyphsDocument();
-    query_value.map_or(is_glyph, |v| v == is_glyph)
 }
 
-fn eval_moz_print_preview(
+fn eval_moz_print_preview(device: &Device) -> bool {
-    device: &Device,
-    query_value: Option<bool>,
-    _: Option<RangeOrOperator>,
-) -> bool {
     let is_print_preview = device.is_print_preview();
     if is_print_preview {
         debug_assert_eq!(device.media_type(), MediaType::print());
     }
-    query_value.map_or(is_print_preview, |v| v == is_print_preview)
+    is_print_preview
 }
 
-fn eval_moz_non_native_content_theme(
+fn eval_moz_non_native_content_theme(device: &Device) -> bool {
-    device: &Device,
+    unsafe { bindings::Gecko_MediaFeatures_ShouldAvoidNativeTheme(device.document()) }
-    query_value: Option<bool>,
-    _: Option<RangeOrOperator>,
-) -> bool {
-    let non_native_theme =
-        unsafe { bindings::Gecko_MediaFeatures_ShouldAvoidNativeTheme(device.document()) };
-    query_value.map_or(non_native_theme, |v| v == non_native_theme)
 }
 
-fn eval_moz_is_resource_document(
+fn eval_moz_is_resource_document(device: &Device) -> bool {
-    device: &Device,
+    unsafe { bindings::Gecko_MediaFeatures_IsResourceDocument(device.document()) }
-    query_value: Option<bool>,
-    _: Option<RangeOrOperator>,
-) -> bool {
-    let is_resource_doc =
-        unsafe { bindings::Gecko_MediaFeatures_IsResourceDocument(device.document()) };
-    query_value.map_or(is_resource_doc, |v| v == is_resource_doc)
 }
 
 /// Allows front-end CSS to discern platform via media queries.
@@ -613,11 +496,7 @@ fn eval_moz_platform(_: &Device, query_value: Option<Platform>) -> bool {
     unsafe { bindings::Gecko_MediaFeatures_MatchesPlatform(query_value) }
 }
 
-fn eval_moz_windows_non_native_menus(
+fn eval_moz_windows_non_native_menus(device: &Device) -> bool {
-    device: &Device,
-    query_value: Option<bool>,
-    _: Option<RangeOrOperator>,
-) -> bool {
     let use_non_native_menus = match static_prefs::pref!("browser.display.windows.non_native_menus")
     {
         0 => false,
@@ -628,17 +507,11 @@ fn eval_moz_windows_non_native_menus(
         },
     };
 
-    query_value.map_or(use_non_native_menus, |v| v == use_non_native_menus)
+    use_non_native_menus
 }
 
-fn eval_moz_overlay_scrollbars(
+fn eval_moz_overlay_scrollbars(device: &Device) -> bool {
-    device: &Device,
+    unsafe { bindings::Gecko_MediaFeatures_UseOverlayScrollbars(device.document()) }
-    query_value: Option<bool>,
-    _: Option<RangeOrOperator>,
-) -> bool {
-    let use_overlay =
-        unsafe { bindings::Gecko_MediaFeatures_UseOverlayScrollbars(device.document()) };
-    query_value.map_or(use_overlay, |v| v == use_overlay)
 }
 
 fn get_lnf_int(int_id: i32) -> i32 {
@@ -667,9 +540,8 @@ fn get_scrollbar_end_forward(int_id: i32) -> bool {
 
 macro_rules! lnf_int_feature {
     ($feature_name:expr, $int_id:ident, $get_value:ident) => {{
-        fn __eval(_: &Device, query_value: Option<bool>, _: Option<RangeOrOperator>) -> bool {
+        fn __eval(_: &Device) -> bool {
-            let value = $get_value(bindings::LookAndFeel_IntID::$int_id as i32);
+            $get_value(bindings::LookAndFeel_IntID::$int_id as i32)
-            query_value.map_or(value, |v| v == value)
         }
 
         feature!(
@@ -695,9 +567,8 @@ macro_rules! lnf_int_feature {
 /// you also need to add them to kMediaQueryPrefs in nsXPLookAndFeel.cpp
 macro_rules! bool_pref_feature {
     ($feature_name:expr, $pref:tt) => {{
-        fn __eval(_: &Device, query_value: Option<bool>, _: Option<RangeOrOperator>) -> bool {
+        fn __eval(_: &Device) -> bool {
-            let value = static_prefs::pref!($pref);
+            static_prefs::pref!($pref)
-            query_value.map_or(value, |v| v == value)
         }
 
         feature!(

components/style/media_queries/media_feature.rs

@@ -4,7 +4,6 @@
 
 //! Media features.
 
-use super::media_feature_expression::RangeOrOperator;
 use super::Device;
 use crate::parser::ParserContext;
 use crate::values::computed::Ratio;
@@ -17,12 +16,7 @@ use style_traits::ParseError;
 /// A generic discriminant for an enum value.
 pub type KeywordDiscriminant = u8;
 
-type MediaFeatureEvaluator<T> = fn(
+type MediaFeatureGetter<T> = fn(device: &Device) -> T;
-    device: &Device,
-    // null == no value was given in the query.
-    value: Option<T>,
-    range_or_operator: Option<RangeOrOperator>,
-) -> bool;
 
 /// Serializes a given discriminant.
 ///
@@ -41,14 +35,14 @@ pub type KeywordParser = for<'a, 'i, 't> fn(
 /// This determines the kind of values that get parsed, too.
 #[allow(missing_docs)]
 pub enum Evaluator {
-    Length(MediaFeatureEvaluator<CSSPixelLength>),
+    Length(MediaFeatureGetter<CSSPixelLength>),
-    Integer(MediaFeatureEvaluator<u32>),
+    Integer(MediaFeatureGetter<u32>),
-    Float(MediaFeatureEvaluator<f32>),
+    Float(MediaFeatureGetter<f32>),
-    BoolInteger(MediaFeatureEvaluator<bool>),
+    BoolInteger(MediaFeatureGetter<bool>),
     /// A non-negative number ratio, such as the one from device-pixel-ratio.
-    NumberRatio(MediaFeatureEvaluator<Ratio>),
+    NumberRatio(MediaFeatureGetter<Ratio>),
     /// A resolution.
-    Resolution(MediaFeatureEvaluator<Resolution>),
+    Resolution(MediaFeatureGetter<Resolution>),
     /// A keyword value.
     Enumerated {
         /// The parser to get a discriminant given a string.
@@ -60,9 +54,8 @@ pub enum Evaluator {
         serializer: KeywordSerializer,
         /// The evaluator itself. This is guaranteed to be called with a
         /// keyword that `parser` has produced.
-        evaluator: MediaFeatureEvaluator<KeywordDiscriminant>,
+        evaluator: fn(&Device, Option<KeywordDiscriminant>) -> bool,
     },
-    Ident(MediaFeatureEvaluator<Atom>),
 }
 
 /// A simple helper macro to create a keyword evaluator.
@@ -93,14 +86,7 @@ macro_rules! keyword_evaluator {
         fn __evaluate(
             device: &$crate::media_queries::Device,
             value: Option<$crate::media_queries::media_feature::KeywordDiscriminant>,
-            range_or_operator: Option<
-                $crate::media_queries::media_feature_expression::RangeOrOperator,
-            >,
         ) -> bool {
-            debug_assert!(
-                range_or_operator.is_none(),
-                "Since when do keywords accept ranges?"
-            );
             // This unwrap is ok because the only discriminants that get
             // back to us is the ones that `parse` produces.
             let value: Option<$keyword_type> =

components/style/media_queries/media_feature_expression.rs

@@ -17,7 +17,7 @@ use crate::servo::media_queries::MEDIA_FEATURES;
 use crate::str::{starts_with_ignore_ascii_case, string_as_ascii_lowercase};
 use crate::values::computed::{self, Ratio, ToComputedValue};
 use crate::values::specified::{Integer, Length, Number, Resolution};
-use crate::values::{serialize_atom_identifier, CSSFloat};
+use crate::values::CSSFloat;
 use crate::{Atom, Zero};
 use cssparser::{Parser, Token};
 use std::cmp::{Ordering, PartialOrd};
@@ -78,7 +78,7 @@ pub enum RangeOrOperator {
 impl RangeOrOperator {
     /// Evaluate a given range given an optional query value and a value from
     /// the browser.
-    pub fn evaluate<T>(range_or_op: Option<Self>, query_value: Option<T>, value: T) -> bool
+    fn evaluate<T>(range_or_op: Option<Self>, query_value: Option<T>, value: T) -> bool
     where
         T: PartialOrd + Zero,
     {
@@ -90,7 +90,7 @@ impl RangeOrOperator {
 
     /// Evaluate a given range given a non-optional query value and a value from
     /// the browser.
-    pub fn evaluate_with_query_value<T>(range_or_op: Option<Self>, query_value: T, value: T) -> bool
+    fn evaluate_with_query_value<T>(range_or_op: Option<Self>, query_value: T, value: T) -> bool
     where
         T: PartialOrd,
     {
@@ -125,21 +125,13 @@ impl RangeOrOperator {
 
 /// A feature expression contains a reference to the media feature, the value
 /// the media query contained, and the range to evaluate.
-#[derive(Clone, Debug, MallocSizeOf, ToShmem)]
+#[derive(Clone, Debug, MallocSizeOf, ToShmem, PartialEq)]
 pub struct MediaFeatureExpression {
     feature_index: usize,
     value: Option<MediaExpressionValue>,
     range_or_operator: Option<RangeOrOperator>,
 }
 
-impl PartialEq for MediaFeatureExpression {
-    fn eq(&self, other: &Self) -> bool {
-        self.feature_index == other.feature_index &&
-            self.value == other.value &&
-            self.range_or_operator == other.range_or_operator
-    }
-}
-
 impl ToCss for MediaFeatureExpression {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
@@ -408,34 +400,50 @@ impl MediaFeatureExpression {
                         specified.to_computed_value(context)
                     })
                 });
-                eval(device, computed, self.range_or_operator)
+                let length = eval(device);
+                RangeOrOperator::evaluate(self.range_or_operator, computed, length)
             },
             Evaluator::Integer(eval) => {
-                eval(device, expect!(Integer).cloned(), self.range_or_operator)
+                let computed = expect!(Integer).cloned();
+                let integer = eval(device);
+                RangeOrOperator::evaluate(self.range_or_operator, computed, integer)
+            },
+            Evaluator::Float(eval) => {
+                let computed = expect!(Float).cloned();
+                let float = eval(device);
+                RangeOrOperator::evaluate(self.range_or_operator, computed, float)
+            }
+            Evaluator::NumberRatio(eval) => {
+                // 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(device);
+                RangeOrOperator::evaluate_with_query_value(self.range_or_operator, computed, ratio)
             },
-            Evaluator::Float(eval) => eval(device, expect!(Float).cloned(), self.range_or_operator),
-            Evaluator::NumberRatio(eval) => eval(
-                device,
-                expect!(NumberRatio).cloned(),
-                self.range_or_operator,
-            ),
             Evaluator::Resolution(eval) => {
                 let computed = expect!(Resolution).map(|specified| {
                     computed::Context::for_media_query_evaluation(device, quirks_mode, |context| {
-                        specified.to_computed_value(context)
+                        specified.to_computed_value(context).dppx()
                     })
                 });
-                eval(device, computed, self.range_or_operator)
+                let resolution = eval(device).dppx();
+                RangeOrOperator::evaluate(self.range_or_operator, computed, resolution)
             },
             Evaluator::Enumerated { evaluator, .. } => {
-                evaluator(device, expect!(Enumerated).cloned(), self.range_or_operator)
+                debug_assert!(self.range_or_operator.is_none(), "Ranges with keywords?");
+                evaluator(device, expect!(Enumerated).cloned())
+            },
+            Evaluator::BoolInteger(eval) => {
+                debug_assert!(self.range_or_operator.is_none(), "Ranges with bools?");
+                let computed = expect!(BoolInteger).cloned();
+                let boolean = eval(device);
+                computed.map_or(boolean, |v| v == boolean)
             },
-            Evaluator::Ident(eval) => eval(device, expect!(Ident).cloned(), self.range_or_operator),
-            Evaluator::BoolInteger(eval) => eval(
-                device,
-                expect!(BoolInteger).cloned(),
-                self.range_or_operator,
-            ),
         }
     }
 }
@@ -466,8 +474,6 @@ pub enum MediaExpressionValue {
     /// An enumerated value, defined by the variant keyword table in the
     /// feature's `mData` member.
     Enumerated(KeywordDiscriminant),
-    /// An identifier.
-    Ident(Atom),
 }
 
 impl MediaExpressionValue {
@@ -482,7 +488,6 @@ impl MediaExpressionValue {
             MediaExpressionValue::BoolInteger(v) => dest.write_str(if v { "1" } else { "0" }),
             MediaExpressionValue::NumberRatio(ratio) => ratio.to_css(dest),
             MediaExpressionValue::Resolution(ref r) => r.to_css(dest),
-            MediaExpressionValue::Ident(ref ident) => serialize_atom_identifier(ident, dest),
             MediaExpressionValue::Enumerated(value) => match for_expr.feature().evaluator {
                 Evaluator::Enumerated { serializer, .. } => dest.write_str(&*serializer(value)),
                 _ => unreachable!(),
@@ -527,9 +532,6 @@ impl MediaExpressionValue {
             Evaluator::Enumerated { parser, .. } => {
                 MediaExpressionValue::Enumerated(parser(context, input)?)
             },
-            Evaluator::Ident(..) => {
-                MediaExpressionValue::Ident(Atom::from(input.expect_ident()?.as_ref()))
-            },
         })
     }
 }