From 6a6a53930e2c1fe0eb67dc388fd789a3368a4614 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 11 Aug 2023 00:44:06 +0200 Subject: [PATCH] style: Simplify media query evaluation code a bit This patch: * Removes generic 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 | 217 ++++-------------- .../style/media_queries/media_feature.rs | 30 +-- .../media_queries/media_feature_expression.rs | 72 +++--- 3 files changed, 89 insertions(+), 230 deletions(-) diff --git a/components/style/gecko/media_features.rs b/components/style/gecko/media_features.rs index c284fbd86a3..57c41c95e95 100644 --- a/components/style/gecko/media_features.rs +++ b/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 { } /// https://drafts.csswg.org/mediaqueries-4/#width -fn eval_width( - device: &Device, - value: Option, - range_or_operator: Option, -) -> bool { - RangeOrOperator::evaluate( - range_or_operator, - value.map(Au::from), - device.au_viewport_size().width, - ) +fn eval_width(device: &Device) -> CSSPixelLength { + CSSPixelLength::new(device.au_viewport_size().width.to_f32_px()) } /// https://drafts.csswg.org/mediaqueries-4/#device-width -fn eval_device_width( - device: &Device, - value: Option, - range_or_operator: Option, -) -> bool { - RangeOrOperator::evaluate( - range_or_operator, - value.map(Au::from), - device_size(device).width, - ) +fn eval_device_width(device: &Device) -> CSSPixelLength { + CSSPixelLength::new(device_size(device).width.to_f32_px()) } /// https://drafts.csswg.org/mediaqueries-4/#height -fn eval_height( - device: &Device, - value: Option, - range_or_operator: Option, -) -> bool { - RangeOrOperator::evaluate( - range_or_operator, - value.map(Au::from), - device.au_viewport_size().height, - ) +fn eval_height(device: &Device) -> CSSPixelLength { + CSSPixelLength::new(device.au_viewport_size().height.to_f32_px()) } /// https://drafts.csswg.org/mediaqueries-4/#device-height -fn eval_device_height( - device: &Device, - value: Option, - range_or_operator: Option, -) -> bool { - RangeOrOperator::evaluate( - range_or_operator, - value.map(Au::from), - device_size(device).height, - ) +fn eval_device_height(device: &Device) -> CSSPixelLength { + CSSPixelLength::new(device_size(device).height.to_f32_px()) } -fn eval_aspect_ratio_for( - device: &Device, - query_value: Option, - range_or_operator: Option, - get_size: F, -) -> bool +fn eval_aspect_ratio_for(device: &Device, get_size: F) -> Ratio where F: FnOnce(&Device) -> Size2D, { - // 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); - RangeOrOperator::evaluate_with_query_value(range_or_operator, query_value, value) + Ratio::new(size.width.0 as f32, size.height.0 as f32) } /// https://drafts.csswg.org/mediaqueries-4/#aspect-ratio -fn eval_aspect_ratio( - device: &Device, - query_value: Option, - range_or_operator: Option, -) -> bool { - eval_aspect_ratio_for( - device, - query_value, - range_or_operator, - Device::au_viewport_size, - ) +fn eval_aspect_ratio(device: &Device) -> Ratio { + eval_aspect_ratio_for(device, Device::au_viewport_size) } /// https://drafts.csswg.org/mediaqueries-4/#device-aspect-ratio -fn eval_device_aspect_ratio( - device: &Device, - query_value: Option, - range_or_operator: Option, -) -> bool { - eval_aspect_ratio_for(device, query_value, range_or_operator, device_size) +fn eval_device_aspect_ratio(device: &Device) -> Ratio { + eval_aspect_ratio_for(device, device_size) } /// https://compat.spec.whatwg.org/#css-media-queries-webkit-device-pixel-ratio -fn eval_device_pixel_ratio( - device: &Device, - query_value: Option, - range_or_operator: Option, -) -> bool { - eval_resolution( - device, - query_value.map(Resolution::from_dppx), - range_or_operator, - ) +fn eval_device_pixel_ratio(device: &Device) -> f32 { + eval_resolution(device).dppx() } #[derive(Clone, Copy, Debug, FromPrimitive, Parse, ToCss)] @@ -192,17 +123,15 @@ fn eval_display_mode(device: &Device, query_value: Option) -> bool } /// https://drafts.csswg.org/mediaqueries-4/#grid -fn eval_grid(_: &Device, query_value: Option, _: Option) -> 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; - query_value.map_or(supports_grid, |v| v == supports_grid) + false } /// https://compat.spec.whatwg.org/#css-media-queries-webkit-transform-3d -fn eval_transform_3d(_: &Device, query_value: Option, _: Option) -> bool { - let supports_transforms = true; - query_value.map_or(supports_transforms, |v| v == supports_transforms) +fn eval_transform_3d(_: &Device) -> bool { + true } #[derive(Clone, Copy, Debug, FromPrimitive, Parse, ToCss)] @@ -220,51 +149,26 @@ fn eval_scan(_: &Device, _: Option) -> bool { } /// https://drafts.csswg.org/mediaqueries-4/#color -fn eval_color( - device: &Device, - query_value: Option, - range_or_operator: Option, -) -> bool { - let color_bits_per_channel = - unsafe { bindings::Gecko_MediaFeatures_GetColorDepth(device.document()) }; - RangeOrOperator::evaluate(range_or_operator, query_value, color_bits_per_channel) +fn eval_color(device: &Device) -> u32 { + unsafe { bindings::Gecko_MediaFeatures_GetColorDepth(device.document()) } } /// https://drafts.csswg.org/mediaqueries-4/#color-index -fn eval_color_index( - _: &Device, - query_value: Option, - range_or_operator: Option, -) -> bool { +fn eval_color_index(_: &Device) -> u32 { // We should return zero if the device does not use a color lookup table. - let index = 0; - RangeOrOperator::evaluate(range_or_operator, query_value, index) + 0 } /// https://drafts.csswg.org/mediaqueries-4/#monochrome -fn eval_monochrome( - device: &Device, - query_value: Option, - range_or_operator: Option, -) -> bool { +fn eval_monochrome(device: &Device) -> u32 { // For color devices we should return 0. - let depth = - unsafe { bindings::Gecko_MediaFeatures_GetMonochromeBitsPerPixel(device.document()) }; - RangeOrOperator::evaluate(range_or_operator, query_value, depth) + unsafe { bindings::Gecko_MediaFeatures_GetMonochromeBitsPerPixel(device.document()) } } /// https://drafts.csswg.org/mediaqueries-4/#resolution -fn eval_resolution( - device: &Device, - query_value: Option, - range_or_operator: Option, -) -> bool { +fn eval_resolution(device: &Device) -> Resolution { let resolution_dppx = unsafe { bindings::Gecko_MediaFeatures_GetResolution(device.document()) }; - RangeOrOperator::evaluate( - range_or_operator, - query_value.map(|r| r.dppx()), - resolution_dppx, - ) + Resolution::from_dppx(resolution_dppx) } #[derive(Clone, Copy, Debug, FromPrimitive, Parse, ToCss)] @@ -540,45 +444,24 @@ fn eval_any_hover(device: &Device, query_value: Option) -> bool { eval_hover_capabilities(query_value, all_pointer_capabilities(device)) } -fn eval_moz_is_glyph( - device: &Device, - query_value: Option, - _: Option, -) -> bool { - let is_glyph = device.document().mIsSVGGlyphsDocument(); - query_value.map_or(is_glyph, |v| v == is_glyph) +fn eval_moz_is_glyph(device: &Device) -> bool { + device.document().mIsSVGGlyphsDocument() } -fn eval_moz_print_preview( - device: &Device, - query_value: Option, - _: Option, -) -> bool { +fn eval_moz_print_preview(device: &Device) -> 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( - device: &Device, - query_value: Option, - _: Option, -) -> 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_non_native_content_theme(device: &Device) -> bool { + unsafe { bindings::Gecko_MediaFeatures_ShouldAvoidNativeTheme(device.document()) } } -fn eval_moz_is_resource_document( - device: &Device, - query_value: Option, - _: Option, -) -> bool { - let is_resource_doc = - unsafe { bindings::Gecko_MediaFeatures_IsResourceDocument(device.document()) }; - query_value.map_or(is_resource_doc, |v| v == is_resource_doc) +fn eval_moz_is_resource_document(device: &Device) -> bool { + unsafe { bindings::Gecko_MediaFeatures_IsResourceDocument(device.document()) } } /// Allows front-end CSS to discern platform via media queries. @@ -613,11 +496,7 @@ fn eval_moz_platform(_: &Device, query_value: Option) -> bool { unsafe { bindings::Gecko_MediaFeatures_MatchesPlatform(query_value) } } -fn eval_moz_windows_non_native_menus( - device: &Device, - query_value: Option, - _: Option, -) -> bool { +fn eval_moz_windows_non_native_menus(device: &Device) -> 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( - device: &Device, - query_value: Option, - _: Option, -) -> bool { - let use_overlay = - unsafe { bindings::Gecko_MediaFeatures_UseOverlayScrollbars(device.document()) }; - query_value.map_or(use_overlay, |v| v == use_overlay) +fn eval_moz_overlay_scrollbars(device: &Device) -> bool { + unsafe { bindings::Gecko_MediaFeatures_UseOverlayScrollbars(device.document()) } } 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, _: Option) -> bool { - let value = $get_value(bindings::LookAndFeel_IntID::$int_id as i32); - query_value.map_or(value, |v| v == value) + fn __eval(_: &Device) -> bool { + $get_value(bindings::LookAndFeel_IntID::$int_id as i32) } 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, _: Option) -> bool { - let value = static_prefs::pref!($pref); - query_value.map_or(value, |v| v == value) + fn __eval(_: &Device) -> bool { + static_prefs::pref!($pref) } feature!( diff --git a/components/style/media_queries/media_feature.rs b/components/style/media_queries/media_feature.rs index e6edff3f68a..7eb51b9f513 100644 --- a/components/style/media_queries/media_feature.rs +++ b/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 = fn( - device: &Device, - // null == no value was given in the query. - value: Option, - range_or_operator: Option, -) -> bool; +type MediaFeatureGetter = fn(device: &Device) -> T; /// 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), - Integer(MediaFeatureEvaluator), - Float(MediaFeatureEvaluator), - BoolInteger(MediaFeatureEvaluator), + Length(MediaFeatureGetter), + Integer(MediaFeatureGetter), + Float(MediaFeatureGetter), + BoolInteger(MediaFeatureGetter), /// A non-negative number ratio, such as the one from device-pixel-ratio. - NumberRatio(MediaFeatureEvaluator), + NumberRatio(MediaFeatureGetter), /// A resolution. - Resolution(MediaFeatureEvaluator), + Resolution(MediaFeatureGetter), /// 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, + evaluator: fn(&Device, Option) -> bool, }, - Ident(MediaFeatureEvaluator), } /// 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> = diff --git a/components/style/media_queries/media_feature_expression.rs b/components/style/media_queries/media_feature_expression.rs index 80827af401c..772560143ee 100644 --- a/components/style/media_queries/media_feature_expression.rs +++ b/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(range_or_op: Option, query_value: Option, value: T) -> bool + fn evaluate(range_or_op: Option, query_value: Option, 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(range_or_op: Option, query_value: T, value: T) -> bool + fn evaluate_with_query_value(range_or_op: Option, 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, range_or_operator: Option, } -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(&self, dest: &mut CssWriter) -> 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())) - }, }) } }