From da6316fbe34bca671685b6a5153525162729c246 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 15:58:13 +0100 Subject: [PATCH 01/11] =?UTF-8?q?Return=20shorthand=20decarations=20as=20a?= =?UTF-8?q?=20new=20enum,=20don=E2=80=99t=20push=20them=20to=20a=20Vec.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- components/style/properties/helpers.mako.rs | 39 ++++------- .../style/properties/properties.mako.rs | 66 ++++++++++++++++++- 2 files changed, 74 insertions(+), 31 deletions(-) diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index b85f9de75db..e2f182e3fcb 100644 --- a/components/style/properties/helpers.mako.rs +++ b/components/style/properties/helpers.mako.rs @@ -339,7 +339,7 @@ input.reset(start); let (first_token_type, css) = try!( ::custom_properties::parse_non_custom_with_var(input)); - return Ok(DeclaredValue::WithVariables(Box::new(UnparsedValue { + return Ok(DeclaredValue::WithVariables(Arc::new(UnparsedValue { css: css.into_owned(), first_token_type: first_token_type, base_url: context.base_url.clone(), @@ -483,10 +483,10 @@ #[allow(unused_imports)] use cssparser::Parser; use parser::ParserContext; - use properties::{DeclaredValue, PropertyDeclaration}; + use properties::{DeclaredValue, PropertyDeclaration, ParsedDeclaration}; use properties::{ShorthandId, UnparsedValue, longhands}; - use properties::declaration_block::Importance; use std::fmt; + use std::sync::Arc; use style_traits::ToCss; pub struct Longhands { @@ -551,10 +551,7 @@ /// Parse the given shorthand and fill the result into the /// `declarations` vector. - pub fn parse(context: &ParserContext, - input: &mut Parser, - declarations: &mut Vec<(PropertyDeclaration, Importance)>) - -> Result<(), ()> { + pub fn parse(context: &ParserContext, input: &mut Parser) -> Result { input.look_for_var_functions(); let start = input.position(); let value = input.parse_entirely(|input| parse_value(context, input)); @@ -563,31 +560,17 @@ } let var = input.seen_var_functions(); if let Ok(value) = value { - % for sub_property in shorthand.sub_properties: - declarations.push((PropertyDeclaration::${sub_property.camel_case}( - % if sub_property.boxed: - DeclaredValue::Value(Box::new(value.${sub_property.ident})) - % else: - DeclaredValue::Value(value.${sub_property.ident}) - % endif - ), Importance::Normal)); - % endfor - Ok(()) + Ok(ParsedDeclaration::${shorthand.camel_case}(value)) } else if var { input.reset(start); let (first_token_type, css) = try!( ::custom_properties::parse_non_custom_with_var(input)); - % for sub_property in shorthand.sub_properties: - declarations.push((PropertyDeclaration::${sub_property.camel_case}( - DeclaredValue::WithVariables(Box::new(UnparsedValue { - css: css.clone().into_owned(), - first_token_type: first_token_type, - base_url: context.base_url.clone(), - from_shorthand: Some(ShorthandId::${shorthand.camel_case}), - })) - ), Importance::Normal)); - % endfor - Ok(()) + Ok(ParsedDeclaration::${shorthand.camel_case}WithVariables(Arc::new(UnparsedValue { + css: css.into_owned(), + first_token_type: first_token_type, + base_url: context.base_url.clone(), + from_shorthand: Some(ShorthandId::${shorthand.camel_case}), + }))) } else { Err(()) } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index e0a2b24b6cc..f0969ac1eea 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -568,7 +568,7 @@ pub enum DeclaredValue { /// A known specified value from the stylesheet. Value(T), /// An unparsed value that contains `var()` functions. - WithVariables(Box), + WithVariables(Arc), /// An CSS-wide keyword. CSSWideKeyword(CSSWideKeyword), } @@ -797,6 +797,61 @@ impl PropertyId { } } +/// Includes shorthands before expansion +pub enum ParsedDeclaration { + % for shorthand in data.shorthands: + /// ${shorthand.name} + ${shorthand.camel_case}(shorthands::${shorthand.ident}::Longhands), + % endfor + % for shorthand in data.shorthands: + /// ${shorthand.name} with var() functions + ${shorthand.camel_case}WithVariables(Arc), + % endfor + /// Not a shorthand + LonghandOrCustom(PropertyDeclaration), +} + +impl ParsedDeclaration { + /// Transform this ParsedDeclaration into a sequence of PropertyDeclaration + /// by expanding shorthand declarations into their corresponding longhands + pub fn expand(self, mut f: F) where F: FnMut(PropertyDeclaration) { + match self { + % for shorthand in data.shorthands: + ParsedDeclaration::${shorthand.camel_case}( + shorthands::${shorthand.ident}::Longhands { + % for sub_property in shorthand.sub_properties: + ${sub_property.ident}, + % endfor + } + ) => { + % for sub_property in shorthand.sub_properties: + f(PropertyDeclaration::${sub_property.camel_case}( + % if sub_property.boxed: + DeclaredValue::Value(Box::new(${sub_property.ident})) + % else: + DeclaredValue::Value(${sub_property.ident}) + % endif + )); + % endfor + } + % endfor + % for shorthand in data.shorthands: + ParsedDeclaration::${shorthand.camel_case}WithVariables(value) => { + debug_assert_eq!( + value.from_shorthand, + Some(ShorthandId::${shorthand.camel_case}) + ); + % for sub_property in shorthand.sub_properties: + f(PropertyDeclaration::${sub_property.camel_case}( + DeclaredValue::WithVariables(value.clone()))); + % endfor + } + % endfor + ParsedDeclaration::LonghandOrCustom(declaration) => f(declaration), + } + } +} + /// Servo's representation for a property declaration. #[derive(PartialEq, Clone)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] @@ -1075,8 +1130,13 @@ impl PropertyDeclaration { % endfor PropertyDeclarationParseResult::ValidOrIgnoredDeclaration }, - Err(()) => match shorthands::${shorthand.ident}::parse(context, input, result_list) { - Ok(()) => PropertyDeclarationParseResult::ValidOrIgnoredDeclaration, + Err(()) => match shorthands::${shorthand.ident}::parse(context, input) { + Ok(parsed) => { + parsed.expand(|declaration| { + result_list.push((declaration, Importance::Normal)) + }); + PropertyDeclarationParseResult::ValidOrIgnoredDeclaration + } Err(()) => PropertyDeclarationParseResult::InvalidValue, } } From 7455ad5eb4977898baf4559b5710f38353b58350 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 16:19:06 +0100 Subject: [PATCH 02/11] PropertyDeclarationParseResult -> Result<(), PropertyDeclarationParseError> --- components/style/keyframes.rs | 10 ++---- .../style/properties/declaration_block.rs | 10 +++--- .../style/properties/properties.mako.rs | 34 +++++++++---------- components/style/supports.rs | 10 +----- ports/geckolib/glue.rs | 11 +++--- 5 files changed, 30 insertions(+), 45 deletions(-) diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs index 87bebc308d2..40d8ef60b09 100644 --- a/components/style/keyframes.rs +++ b/components/style/keyframes.rs @@ -13,7 +13,6 @@ use parser::{ParserContext, ParserContextExtraData, log_css_error}; use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId}; use properties::{PropertyDeclarationId, LonghandId, DeclaredValue}; use properties::LonghandIdSet; -use properties::PropertyDeclarationParseResult; use properties::animated_properties::TransitionProperty; use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction; use std::fmt; @@ -407,12 +406,9 @@ impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> { fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<(), ()> { let id = try!(PropertyId::parse(name.into())); let old_len = self.declarations.len(); - match PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, true) { - PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => {} - _ => { - self.declarations.truncate(old_len); - return Err(()); - } + if PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, true).is_err() { + self.declarations.truncate(old_len); + return Err(()) } // In case there is still unparsed text in the declaration, we should roll back. if !input.is_exhausted() { diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index ac618dcbf72..b21fa79e3ec 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -561,8 +561,8 @@ pub fn parse_one_declaration(id: PropertyId, Parser::new(input).parse_entirely(|parser| { let mut results = vec![]; match PropertyDeclaration::parse(id, &context, parser, &mut results, false) { - PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => Ok(results), - _ => Err(()) + Ok(()) => Ok(results), + Err(_) => Err(()) } }) } @@ -592,10 +592,8 @@ impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> { let id = try!(PropertyId::parse(name.into())); let old_len = self.declarations.len(); let parse_result = input.parse_until_before(Delimiter::Bang, |input| { - match PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, false) { - PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => Ok(()), - _ => Err(()) - } + PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, false) + .map_err(|_| ()) }); if let Err(_) = parse_result { // rollback diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index f0969ac1eea..642b56ff851 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -886,7 +886,7 @@ impl HasViewportPercentage for PropertyDeclaration { /// The result of parsing a property declaration. #[derive(Eq, PartialEq, Copy, Clone)] -pub enum PropertyDeclarationParseResult { +pub enum PropertyDeclarationParseError { /// The property declaration was for an unknown property. UnknownProperty, /// The property declaration was for a disabled experimental property. @@ -898,8 +898,6 @@ pub enum PropertyDeclarationParseResult { /// /// See: https://drafts.csswg.org/css-animations/#keyframes AnimationPropertyInKeyframeBlock, - /// The declaration was either valid or ignored. - ValidOrIgnoredDeclaration, } impl fmt::Debug for PropertyDeclaration { @@ -933,7 +931,7 @@ impl ToCss for PropertyDeclaration { % if property.experimental and product == "servo": if !PREFS.get("${property.experimental}") .as_boolean().unwrap_or(false) { - return PropertyDeclarationParseResult::ExperimentalProperty + return Err(PropertyDeclarationParseError::ExperimentalProperty) } % endif % if product == "gecko": @@ -950,7 +948,7 @@ impl ToCss for PropertyDeclaration { let id = structs::${helpers.to_nscsspropertyid(property.ident)}; let enabled = unsafe { bindings::Gecko_PropertyId_IsPrefEnabled(id) }; if !enabled { - return PropertyDeclarationParseResult::ExperimentalProperty + return Err(PropertyDeclarationParseError::ExperimentalProperty) } } % endif @@ -1060,19 +1058,19 @@ impl PropertyDeclaration { pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser, result_list: &mut Vec<(PropertyDeclaration, Importance)>, in_keyframe_block: bool) - -> PropertyDeclarationParseResult { + -> Result<(), PropertyDeclarationParseError> { match id { PropertyId::Custom(name) => { let value = match input.try(|i| CSSWideKeyword::parse(context, i)) { Ok(keyword) => DeclaredValue::CSSWideKeyword(keyword), Err(()) => match ::custom_properties::SpecifiedValue::parse(context, input) { Ok(value) => DeclaredValue::Value(value), - Err(()) => return PropertyDeclarationParseResult::InvalidValue, + Err(()) => return Err(PropertyDeclarationParseError::InvalidValue), } }; result_list.push((PropertyDeclaration::Custom(name, value), Importance::Normal)); - return PropertyDeclarationParseResult::ValidOrIgnoredDeclaration; + return Ok(()); } PropertyId::Longhand(id) => match id { % for property in data.longhands: @@ -1080,12 +1078,12 @@ impl PropertyDeclaration { % if not property.derived_from: % if not property.allowed_in_keyframe_block: if in_keyframe_block { - return PropertyDeclarationParseResult::AnimationPropertyInKeyframeBlock + return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock) } % endif % if property.internal: if context.stylesheet_origin != Origin::UserAgent { - return PropertyDeclarationParseResult::UnknownProperty + return Err(PropertyDeclarationParseError::UnknownProperty) } % endif @@ -1095,12 +1093,12 @@ impl PropertyDeclaration { Ok(value) => { result_list.push((PropertyDeclaration::${property.camel_case}(value), Importance::Normal)); - PropertyDeclarationParseResult::ValidOrIgnoredDeclaration + Ok(()) }, - Err(()) => PropertyDeclarationParseResult::InvalidValue, + Err(()) => Err(PropertyDeclarationParseError::InvalidValue), } % else: - PropertyDeclarationParseResult::UnknownProperty + Err(PropertyDeclarationParseError::UnknownProperty) % endif } % endfor @@ -1110,12 +1108,12 @@ impl PropertyDeclaration { ShorthandId::${shorthand.camel_case} => { % if not shorthand.allowed_in_keyframe_block: if in_keyframe_block { - return PropertyDeclarationParseResult::AnimationPropertyInKeyframeBlock + return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock) } % endif % if shorthand.internal: if context.stylesheet_origin != Origin::UserAgent { - return PropertyDeclarationParseResult::UnknownProperty + return Err(PropertyDeclarationParseError::UnknownProperty) } % endif @@ -1128,16 +1126,16 @@ impl PropertyDeclaration { PropertyDeclaration::${sub_property.camel_case}( DeclaredValue::CSSWideKeyword(keyword)), Importance::Normal)); % endfor - PropertyDeclarationParseResult::ValidOrIgnoredDeclaration + Ok(()) }, Err(()) => match shorthands::${shorthand.ident}::parse(context, input) { Ok(parsed) => { parsed.expand(|declaration| { result_list.push((declaration, Importance::Normal)) }); - PropertyDeclarationParseResult::ValidOrIgnoredDeclaration + Ok(()) } - Err(()) => PropertyDeclarationParseResult::InvalidValue, + Err(()) => Err(PropertyDeclarationParseError::InvalidValue), } } } diff --git a/components/style/supports.rs b/components/style/supports.rs index 0451204eb1e..170d874801e 100644 --- a/components/style/supports.rs +++ b/components/style/supports.rs @@ -205,7 +205,6 @@ impl Declaration { /// /// https://drafts.csswg.org/css-conditional-3/#support-definition pub fn eval(&self, cx: &ParserContext) -> bool { - use properties::PropertyDeclarationParseResult::*; let id = if let Ok(id) = PropertyId::parse((&*self.prop).into()) { id } else { @@ -219,13 +218,6 @@ impl Declaration { if !input.is_exhausted() { return false; } - match res { - UnknownProperty => false, - ExperimentalProperty => false, // only happens for experimental props - // that haven't been enabled - InvalidValue => false, - AnimationPropertyInKeyframeBlock => unreachable!(), - ValidOrIgnoredDeclaration => true, - } + res.is_ok() } } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 4dccb916c72..3047b727293 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -68,7 +68,7 @@ use style::keyframes::KeyframesStepValue; use style::parallel; use style::parser::{ParserContext, ParserContextExtraData}; use style::properties::{ComputedValues, Importance, PropertyDeclaration}; -use style::properties::{PropertyDeclarationParseResult, PropertyDeclarationBlock, PropertyId}; +use style::properties::{PropertyDeclarationBlock, PropertyId}; use style::properties::animated_properties::{AnimationValue, Interpolate, TransitionProperty}; use style::properties::parse_one_declaration; use style::restyle_hints::{self, RestyleHint}; @@ -714,10 +714,11 @@ pub extern "C" fn Servo_ParseProperty(property: *const nsACString, value: *const extra_data); let mut results = vec![]; - match PropertyDeclaration::parse(id, &context, &mut Parser::new(value), - &mut results, false) { - PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => {}, - _ => return RawServoDeclarationBlockStrong::null(), + let result = PropertyDeclaration::parse( + id, &context, &mut Parser::new(value), &mut results, false + ); + if result.is_err() { + return RawServoDeclarationBlockStrong::null() } Arc::new(RwLock::new(PropertyDeclarationBlock { From 7444be686c5706c7a15b617ec04be5e4e715cc6e Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 16:20:15 +0100 Subject: [PATCH 03/11] Fix a warning --- tests/unit/style/rule_tree/bench.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/style/rule_tree/bench.rs b/tests/unit/style/rule_tree/bench.rs index fccab1252f9..ba76d79b861 100644 --- a/tests/unit/style/rule_tree/bench.rs +++ b/tests/unit/style/rule_tree/bench.rs @@ -17,7 +17,7 @@ use test::{self, Bencher}; struct ErrorringErrorReporter; impl ParseErrorReporter for ErrorringErrorReporter { - fn report_error(&self, input: &mut Parser, position: SourcePosition, message: &str, + fn report_error(&self, _input: &mut Parser, position: SourcePosition, message: &str, url: &ServoUrl) { panic!("CSS error: {}\t\n{:?} {}", url.as_str(), position, message); } From 81604902724d928eb29e3178a52f89aa679818e6 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 15:19:41 +0100 Subject: [PATCH 04/11] Remove some unused impls. --- components/style/font_face.rs | 5 ++--- components/style/keyframes.rs | 13 +++++-------- components/style/properties/declaration_block.rs | 2 -- components/style/properties/properties.mako.rs | 5 +---- components/style/stylesheets.rs | 2 -- 5 files changed, 8 insertions(+), 19 deletions(-) diff --git a/components/style/font_face.rs b/components/style/font_face.rs index 4b3f6934f90..0d4bc4f8a09 100644 --- a/components/style/font_face.rs +++ b/components/style/font_face.rs @@ -21,7 +21,7 @@ use values::specified::url::SpecifiedUrl; /// A source for a font-face rule. #[derive(Clone, Debug, PartialEq, Eq)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf, Deserialize, Serialize))] +#[cfg_attr(feature = "servo", derive(Deserialize, Serialize))] pub enum Source { /// A `url()` source. Url(UrlSource), @@ -54,7 +54,7 @@ impl OneOrMoreCommaSeparated for Source {} /// /// https://drafts.csswg.org/css-fonts/#src-desc #[derive(Clone, Debug, PartialEq, Eq)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf, Deserialize, Serialize))] +#[cfg_attr(feature = "servo", derive(Deserialize, Serialize))] pub struct UrlSource { /// The specified url. pub url: SpecifiedUrl, @@ -184,7 +184,6 @@ macro_rules! font_face_descriptors { /// /// https://drafts.csswg.org/css-fonts/#font-face-rule #[derive(Debug, PartialEq, Eq)] - #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct FontFaceRule { $( #[$m_doc] diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs index 40d8ef60b09..048d9904043 100644 --- a/components/style/keyframes.rs +++ b/components/style/keyframes.rs @@ -70,8 +70,7 @@ impl KeyframePercentage { /// A keyframes selector is a list of percentages or from/to symbols, which are /// converted at parse time to percentages. -#[derive(Debug, Clone, PartialEq)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] +#[derive(Debug, PartialEq)] pub struct KeyframeSelector(Vec); impl KeyframeSelector { /// Return the list of percentages this selector contains. @@ -93,8 +92,7 @@ impl KeyframeSelector { } /// A keyframe. -#[derive(Debug, Clone)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] +#[derive(Debug)] pub struct Keyframe { /// The selector this keyframe was specified from. pub selector: KeyframeSelector, @@ -103,7 +101,6 @@ pub struct Keyframe { /// /// Note that `!important` rules in keyframes don't apply, but we keep this /// `Arc` just for convenience. - #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] pub block: Arc>, } @@ -148,7 +145,7 @@ impl Keyframe { /// declarations to apply. /// /// TODO: Find a better name for this? -#[derive(Debug, Clone)] +#[derive(Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum KeyframesStepValue { /// A step formed by a declaration block specified by the CSS. @@ -163,7 +160,7 @@ pub enum KeyframesStepValue { } /// A single step from a keyframe animation. -#[derive(Debug, Clone)] +#[derive(Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct KeyframesStep { /// The percentage of the animation duration when this step starts. @@ -235,7 +232,7 @@ impl KeyframesStep { /// of keyframes, in order. /// /// It only takes into account animable properties. -#[derive(Debug, Clone)] +#[derive(Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct KeyframesAnimation { /// The difference steps of the animation. diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index b21fa79e3ec..243bc7cbc5c 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -49,12 +49,10 @@ impl Default for Importance { /// Overridden declarations are skipped. #[derive(Debug, PartialEq, Clone)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct PropertyDeclarationBlock { /// The group of declarations, along with their importance. /// /// Only deduplicated declarations appear here. - #[cfg_attr(feature = "servo", ignore_heap_size_of = "#7038")] pub declarations: Vec<(PropertyDeclaration, Importance)>, /// The number of entries in `self.declaration` with `Importance::Important` diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 642b56ff851..9b2c94798d5 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -563,7 +563,6 @@ impl ShorthandId { /// Servo's representation of a declared value for a given `T`, which is the /// declared value for that property. #[derive(Clone, PartialEq, Eq, Debug)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum DeclaredValue { /// A known specified value from the stylesheet. Value(T), @@ -574,8 +573,7 @@ pub enum DeclaredValue { } /// An unparsed property value that contains `var()` functions. -#[derive(Clone, PartialEq, Eq, Debug)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] +#[derive(PartialEq, Eq, Debug)] pub struct UnparsedValue { /// The css serialization for this value. css: String, @@ -854,7 +852,6 @@ impl ParsedDeclaration { /// Servo's representation for a property declaration. #[derive(PartialEq, Clone)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum PropertyDeclaration { % for property in data.longhands: /// ${property.name} diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index 6ee7b6843a0..758d81125b2 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -52,7 +52,6 @@ pub enum Origin { /// A set of namespaces applying to a given stylesheet. #[derive(Default, Debug)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] #[allow(missing_docs)] pub struct Namespaces { pub default: Option, @@ -389,7 +388,6 @@ impl ToCss for CssRule { } #[derive(Debug, PartialEq)] -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] #[allow(missing_docs)] pub struct NamespaceRule { /// `None` for the default Namespace From 9d663ea7af3771531e87669d8323916e9f106075 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 17:12:37 +0100 Subject: [PATCH 05/11] Make PropertyDeclaration::parse return an enum rather than push to a Vec. --- components/script/dom/cssstyledeclaration.rs | 12 ++--- components/style/keyframes.rs | 36 ++++++------- .../style/properties/declaration_block.rs | 53 +++++++------------ .../style/properties/properties.mako.rs | 50 ++++++++--------- components/style/supports.rs | 9 +--- ports/geckolib/glue.rs | 36 ++++++------- 6 files changed, 83 insertions(+), 113 deletions(-) diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs index f658ed5375e..450d0b7da4b 100644 --- a/components/script/dom/cssstyledeclaration.rs +++ b/components/script/dom/cssstyledeclaration.rs @@ -250,14 +250,14 @@ impl CSSStyleDeclaration { // Step 6 let window = self.owner.window(); - let declarations = + let result = parse_one_declaration(id, &value, &self.owner.base_url(), window.css_error_reporter(), ParserContextExtraData::default()); // Step 7 - let declarations = match declarations { - Ok(declarations) => declarations, + let parsed = match result { + Ok(parsed) => parsed, Err(_) => { *changed = false; return Ok(()); @@ -267,9 +267,9 @@ impl CSSStyleDeclaration { // Step 8 // Step 9 *changed = false; - for declaration in declarations { - *changed |= pdb.set_parsed_declaration(declaration.0, importance); - } + parsed.expand(|declaration| { + *changed |= pdb.set_parsed_declaration(declaration, importance); + }); Ok(()) }) diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs index 048d9904043..e48e8c4e909 100644 --- a/components/style/keyframes.rs +++ b/components/style/keyframes.rs @@ -11,7 +11,7 @@ use cssparser::{DeclarationListParser, DeclarationParser, parse_one_rule}; use parking_lot::RwLock; use parser::{ParserContext, ParserContextExtraData, log_css_error}; use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId}; -use properties::{PropertyDeclarationId, LonghandId, DeclaredValue}; +use properties::{PropertyDeclarationId, LonghandId, DeclaredValue, ParsedDeclaration}; use properties::LonghandIdSet; use properties::animated_properties::TransitionProperty; use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction; @@ -360,12 +360,12 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> { -> Result { let parser = KeyframeDeclarationParser { context: self.context, - declarations: vec![], }; let mut iter = DeclarationListParser::new(input, parser); + let mut declarations = Vec::new(); while let Some(declaration) = iter.next() { match declaration { - Ok(_) => (), + Ok(parsed) => parsed.expand(|d| declarations.push((d, Importance::Normal))), Err(range) => { let pos = range.start; let message = format!("Unsupported keyframe property declaration: '{}'", @@ -378,7 +378,7 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> { Ok(Arc::new(RwLock::new(Keyframe { selector: prelude, block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: iter.parser.declarations, + declarations: declarations, important_count: 0, })), }))) @@ -387,32 +387,30 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> { struct KeyframeDeclarationParser<'a, 'b: 'a> { context: &'a ParserContext<'b>, - declarations: Vec<(PropertyDeclaration, Importance)> } /// Default methods reject all at rules. impl<'a, 'b> AtRuleParser for KeyframeDeclarationParser<'a, 'b> { type Prelude = (); - type AtRule = (); + type AtRule = ParsedDeclaration; } impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> { /// We parse rules directly into the declarations object - type Declaration = (); + type Declaration = ParsedDeclaration; - fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<(), ()> { + fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result { let id = try!(PropertyId::parse(name.into())); - let old_len = self.declarations.len(); - if PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, true).is_err() { - self.declarations.truncate(old_len); - return Err(()) - } - // In case there is still unparsed text in the declaration, we should roll back. - if !input.is_exhausted() { - self.declarations.truncate(old_len); - Err(()) - } else { - Ok(()) + match PropertyDeclaration::parse(id, self.context, input, true) { + Ok(parsed) => { + // In case there is still unparsed text in the declaration, we should roll back. + if !input.is_exhausted() { + Err(()) + } else { + Ok(parsed) + } + } + Err(_) => Err(()) } } } diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 243bc7cbc5c..cb63c97f50c 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -56,7 +56,7 @@ pub struct PropertyDeclarationBlock { pub declarations: Vec<(PropertyDeclaration, Importance)>, /// The number of entries in `self.declaration` with `Importance::Important` - pub important_count: u32, + pub important_count: usize, } impl PropertyDeclarationBlock { @@ -75,7 +75,7 @@ impl PropertyDeclarationBlock { /// which should be maintained whenever `declarations` is changed. // FIXME: make fields private and maintain it here in methods? pub fn any_normal(&self) -> bool { - self.declarations.len() > self.important_count as usize + self.declarations.len() > self.important_count } /// Get a declaration for a given property. @@ -554,63 +554,46 @@ pub fn parse_one_declaration(id: PropertyId, base_url: &ServoUrl, error_reporter: StdBox, extra_data: ParserContextExtraData) - -> Result, ()> { + -> Result { let context = ParserContext::new_with_extra_data(Origin::Author, base_url, error_reporter, extra_data); Parser::new(input).parse_entirely(|parser| { - let mut results = vec![]; - match PropertyDeclaration::parse(id, &context, parser, &mut results, false) { - Ok(()) => Ok(results), - Err(_) => Err(()) - } + PropertyDeclaration::parse(id, &context, parser, false) + .map_err(|_| ()) }) } /// A struct to parse property declarations. struct PropertyDeclarationParser<'a, 'b: 'a> { context: &'a ParserContext<'b>, - declarations: Vec<(PropertyDeclaration, Importance)> } /// Default methods reject all at rules. impl<'a, 'b> AtRuleParser for PropertyDeclarationParser<'a, 'b> { type Prelude = (); - type AtRule = (u32, Importance); + type AtRule = (ParsedDeclaration, Importance); } impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> { - /// Declarations are pushed to the internal vector. This will only - /// let you know if the decl was !important and how many longhands - /// it expanded to - type Declaration = (u32, Importance); + type Declaration = (ParsedDeclaration, Importance); fn parse_value(&mut self, name: &str, input: &mut Parser) - -> Result<(u32, Importance), ()> { + -> Result<(ParsedDeclaration, Importance), ()> { let id = try!(PropertyId::parse(name.into())); - let old_len = self.declarations.len(); - let parse_result = input.parse_until_before(Delimiter::Bang, |input| { - PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, false) - .map_err(|_| ()) - }); - if let Err(_) = parse_result { - // rollback - self.declarations.truncate(old_len); - return Err(()) - } + let parsed = input.parse_until_before(Delimiter::Bang, |input| { + PropertyDeclaration::parse(id, self.context, input, false) + .map_err(|_| ()) + })?; let importance = match input.try(parse_important) { Ok(()) => Importance::Important, Err(()) => Importance::Normal, }; // In case there is still unparsed text in the declaration, we should roll back. if !input.is_exhausted() { - self.declarations.truncate(old_len); return Err(()) } - for decl in &mut self.declarations[old_len..] { - decl.1 = importance - } - Ok(((self.declarations.len() - old_len) as u32, importance)) + Ok((parsed, importance)) } } @@ -620,17 +603,19 @@ impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> { pub fn parse_property_declaration_list(context: &ParserContext, input: &mut Parser) -> PropertyDeclarationBlock { + let mut declarations = Vec::new(); let mut important_count = 0; let parser = PropertyDeclarationParser { context: context, - declarations: vec![], }; let mut iter = DeclarationListParser::new(input, parser); while let Some(declaration) = iter.next() { match declaration { - Ok((count, importance)) => { + Ok((parsed, importance)) => { + let old_len = declarations.len(); + parsed.expand(|d| declarations.push((d, importance))); if importance.important() { - important_count += count; + important_count += declarations.len() - old_len; } } Err(range) => { @@ -642,7 +627,7 @@ pub fn parse_property_declaration_list(context: &ParserContext, } } let mut block = PropertyDeclarationBlock { - declarations: iter.parser.declarations, + declarations: declarations, important_count: important_count, }; block.deduplicate(); diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 9b2c94798d5..8730680db9b 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -800,11 +800,14 @@ pub enum ParsedDeclaration { % for shorthand in data.shorthands: /// ${shorthand.name} ${shorthand.camel_case}(shorthands::${shorthand.ident}::Longhands), - % endfor - % for shorthand in data.shorthands: + + /// ${shorthand.name} with a CSS-wide keyword + ${shorthand.camel_case}CSSWideKeyword(CSSWideKeyword), + /// ${shorthand.name} with var() functions ${shorthand.camel_case}WithVariables(Arc), % endfor + /// Not a shorthand LonghandOrCustom(PropertyDeclaration), } @@ -832,8 +835,13 @@ impl ParsedDeclaration { )); % endfor } - % endfor - % for shorthand in data.shorthands: + ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword) => { + % for sub_property in shorthand.sub_properties: + f(PropertyDeclaration::${sub_property.camel_case}( + DeclaredValue::CSSWideKeyword(keyword) + )); + % endfor + } ParsedDeclaration::${shorthand.camel_case}WithVariables(value) => { debug_assert_eq!( value.from_shorthand, @@ -841,7 +849,8 @@ impl ParsedDeclaration { ); % for sub_property in shorthand.sub_properties: f(PropertyDeclaration::${sub_property.camel_case}( - DeclaredValue::WithVariables(value.clone()))); + DeclaredValue::WithVariables(value.clone()) + )); % endfor } % endfor @@ -1053,9 +1062,8 @@ impl PropertyDeclaration { /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser, /// we only set them here so that we don't have to reallocate pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser, - result_list: &mut Vec<(PropertyDeclaration, Importance)>, in_keyframe_block: bool) - -> Result<(), PropertyDeclarationParseError> { + -> Result { match id { PropertyId::Custom(name) => { let value = match input.try(|i| CSSWideKeyword::parse(context, i)) { @@ -1065,9 +1073,7 @@ impl PropertyDeclaration { Err(()) => return Err(PropertyDeclarationParseError::InvalidValue), } }; - result_list.push((PropertyDeclaration::Custom(name, value), - Importance::Normal)); - return Ok(()); + Ok(ParsedDeclaration::LonghandOrCustom(PropertyDeclaration::Custom(name, value))) } PropertyId::Longhand(id) => match id { % for property in data.longhands: @@ -1088,9 +1094,9 @@ impl PropertyDeclaration { match longhands::${property.ident}::parse_declared(context, input) { Ok(value) => { - result_list.push((PropertyDeclaration::${property.camel_case}(value), - Importance::Normal)); - Ok(()) + Ok(ParsedDeclaration::LonghandOrCustom( + PropertyDeclaration::${property.camel_case}(value) + )) }, Err(()) => Err(PropertyDeclarationParseError::InvalidValue), } @@ -1118,21 +1124,11 @@ impl PropertyDeclaration { match input.try(|i| CSSWideKeyword::parse(context, i)) { Ok(keyword) => { - % for sub_property in shorthand.sub_properties: - result_list.push(( - PropertyDeclaration::${sub_property.camel_case}( - DeclaredValue::CSSWideKeyword(keyword)), Importance::Normal)); - % endfor - Ok(()) + Ok(ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword)) }, - Err(()) => match shorthands::${shorthand.ident}::parse(context, input) { - Ok(parsed) => { - parsed.expand(|declaration| { - result_list.push((declaration, Importance::Normal)) - }); - Ok(()) - } - Err(()) => Err(PropertyDeclarationParseError::InvalidValue), + Err(()) => { + shorthands::${shorthand.ident}::parse(context, input) + .map_err(|()| PropertyDeclarationParseError::InvalidValue) } } } diff --git a/components/style/supports.rs b/components/style/supports.rs index 170d874801e..ccdde3231bb 100644 --- a/components/style/supports.rs +++ b/components/style/supports.rs @@ -211,13 +211,8 @@ impl Declaration { return false }; let mut input = Parser::new(&self.val); - let mut list = Vec::new(); - let res = PropertyDeclaration::parse(id, cx, &mut input, - &mut list, /* in_keyframe */ false); + let res = PropertyDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false); let _ = input.try(parse_important); - if !input.is_exhausted() { - return false; - } - res.is_ok() + res.is_ok() && input.is_exhausted() } } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 3047b727293..c8b5d7013b6 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -713,18 +713,17 @@ pub extern "C" fn Servo_ParseProperty(property: *const nsACString, value: *const Box::new(StdoutErrorReporter), extra_data); - let mut results = vec![]; - let result = PropertyDeclaration::parse( - id, &context, &mut Parser::new(value), &mut results, false - ); - if result.is_err() { - return RawServoDeclarationBlockStrong::null() + match PropertyDeclaration::parse(id, &context, &mut Parser::new(value), false) { + Ok(parsed) => { + let mut declarations = Vec::new(); + parsed.expand(|d| declarations.push((d, Importance::Normal))); + Arc::new(RwLock::new(PropertyDeclarationBlock { + declarations: declarations, + important_count: 0, + })).into_strong() + } + Err(_) => RawServoDeclarationBlockStrong::null() } - - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: results, - important_count: 0, - })).into_strong() } #[no_mangle] @@ -834,14 +833,14 @@ fn set_property(declarations: RawServoDeclarationBlockBorrowed, property_id: Pro // FIXME Needs real URL and ParserContextExtraData. let base_url = &*DUMMY_BASE_URL; let extra_data = ParserContextExtraData::default(); - if let Ok(decls) = parse_one_declaration(property_id, value, &base_url, - Box::new(StdoutErrorReporter), extra_data) { + if let Ok(parsed) = parse_one_declaration(property_id, value, &base_url, + Box::new(StdoutErrorReporter), extra_data) { let mut declarations = RwLock::::as_arc(&declarations).write(); let importance = if is_important { Importance::Important } else { Importance::Normal }; let mut changed = false; - for decl in decls.into_iter() { - changed |= declarations.set_parsed_declaration(decl.0, importance); - } + parsed.expand(|decl| { + changed |= declarations.set_parsed_declaration(decl, importance); + }); changed } else { false @@ -1166,10 +1165,7 @@ pub extern "C" fn Servo_CSSSupports2(property: *const nsACString, value: *const let base_url = &*DUMMY_BASE_URL; let extra_data = ParserContextExtraData::default(); - match parse_one_declaration(id, &value, &base_url, Box::new(StdoutErrorReporter), extra_data) { - Ok(decls) => !decls.is_empty(), - Err(()) => false, - } + parse_one_declaration(id, &value, &base_url, Box::new(StdoutErrorReporter), extra_data).is_ok() } #[no_mangle] From 4b4a873c3ec0b024ef63fbcc02e8fbff0dc75d6a Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 17:17:54 +0100 Subject: [PATCH 06/11] Move parse method of PropertyDeclaration to ParsedDeclaration This is what it now returns. --- components/style/keyframes.rs | 2 +- .../style/properties/declaration_block.rs | 4 +- .../style/properties/properties.mako.rs | 172 +++++++++--------- components/style/supports.rs | 4 +- ports/geckolib/glue.rs | 4 +- 5 files changed, 93 insertions(+), 93 deletions(-) diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs index e48e8c4e909..0fde0c64284 100644 --- a/components/style/keyframes.rs +++ b/components/style/keyframes.rs @@ -401,7 +401,7 @@ impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> { fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result { let id = try!(PropertyId::parse(name.into())); - match PropertyDeclaration::parse(id, self.context, input, true) { + match ParsedDeclaration::parse(id, self.context, input, true) { Ok(parsed) => { // In case there is still unparsed text in the declaration, we should roll back. if !input.is_exhausted() { diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index cb63c97f50c..ff6047289a6 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -557,7 +557,7 @@ pub fn parse_one_declaration(id: PropertyId, -> Result { let context = ParserContext::new_with_extra_data(Origin::Author, base_url, error_reporter, extra_data); Parser::new(input).parse_entirely(|parser| { - PropertyDeclaration::parse(id, &context, parser, false) + ParsedDeclaration::parse(id, &context, parser, false) .map_err(|_| ()) }) } @@ -582,7 +582,7 @@ impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> { -> Result<(ParsedDeclaration, Importance), ()> { let id = try!(PropertyId::parse(name.into())); let parsed = input.parse_until_before(Delimiter::Bang, |input| { - PropertyDeclaration::parse(id, self.context, input, false) + ParsedDeclaration::parse(id, self.context, input, false) .map_err(|_| ()) })?; let importance = match input.try(parse_important) { diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 8730680db9b..74b28d25d95 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -857,6 +857,92 @@ impl ParsedDeclaration { ParsedDeclaration::LonghandOrCustom(declaration) => f(declaration), } } + + /// The `in_keyframe_block` parameter controls this: + /// + /// https://drafts.csswg.org/css-animations/#keyframes + /// > The inside of accepts any CSS property + /// > except those defined in this specification, + /// > but does accept the `animation-play-state` property and interprets it specially. + /// + /// This will not actually parse Importance values, and will always set things + /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser, + /// we only set them here so that we don't have to reallocate + pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser, + in_keyframe_block: bool) + -> Result { + match id { + PropertyId::Custom(name) => { + let value = match input.try(|i| CSSWideKeyword::parse(context, i)) { + Ok(keyword) => DeclaredValue::CSSWideKeyword(keyword), + Err(()) => match ::custom_properties::SpecifiedValue::parse(context, input) { + Ok(value) => DeclaredValue::Value(value), + Err(()) => return Err(PropertyDeclarationParseError::InvalidValue), + } + }; + Ok(ParsedDeclaration::LonghandOrCustom(PropertyDeclaration::Custom(name, value))) + } + PropertyId::Longhand(id) => match id { + % for property in data.longhands: + LonghandId::${property.camel_case} => { + % if not property.derived_from: + % if not property.allowed_in_keyframe_block: + if in_keyframe_block { + return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock) + } + % endif + % if property.internal: + if context.stylesheet_origin != Origin::UserAgent { + return Err(PropertyDeclarationParseError::UnknownProperty) + } + % endif + + ${property_pref_check(property)} + + match longhands::${property.ident}::parse_declared(context, input) { + Ok(value) => { + Ok(ParsedDeclaration::LonghandOrCustom( + PropertyDeclaration::${property.camel_case}(value) + )) + }, + Err(()) => Err(PropertyDeclarationParseError::InvalidValue), + } + % else: + Err(PropertyDeclarationParseError::UnknownProperty) + % endif + } + % endfor + }, + PropertyId::Shorthand(id) => match id { + % for shorthand in data.shorthands: + ShorthandId::${shorthand.camel_case} => { + % if not shorthand.allowed_in_keyframe_block: + if in_keyframe_block { + return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock) + } + % endif + % if shorthand.internal: + if context.stylesheet_origin != Origin::UserAgent { + return Err(PropertyDeclarationParseError::UnknownProperty) + } + % endif + + ${property_pref_check(shorthand)} + + match input.try(|i| CSSWideKeyword::parse(context, i)) { + Ok(keyword) => { + Ok(ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword)) + }, + Err(()) => { + shorthands::${shorthand.ident}::parse(context, input) + .map_err(|()| PropertyDeclarationParseError::InvalidValue) + } + } + } + % endfor + } + } + } } /// Servo's representation for a property declaration. @@ -1051,92 +1137,6 @@ impl PropertyDeclaration { } } - /// The `in_keyframe_block` parameter controls this: - /// - /// https://drafts.csswg.org/css-animations/#keyframes - /// > The inside of accepts any CSS property - /// > except those defined in this specification, - /// > but does accept the `animation-play-state` property and interprets it specially. - /// - /// This will not actually parse Importance values, and will always set things - /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser, - /// we only set them here so that we don't have to reallocate - pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser, - in_keyframe_block: bool) - -> Result { - match id { - PropertyId::Custom(name) => { - let value = match input.try(|i| CSSWideKeyword::parse(context, i)) { - Ok(keyword) => DeclaredValue::CSSWideKeyword(keyword), - Err(()) => match ::custom_properties::SpecifiedValue::parse(context, input) { - Ok(value) => DeclaredValue::Value(value), - Err(()) => return Err(PropertyDeclarationParseError::InvalidValue), - } - }; - Ok(ParsedDeclaration::LonghandOrCustom(PropertyDeclaration::Custom(name, value))) - } - PropertyId::Longhand(id) => match id { - % for property in data.longhands: - LonghandId::${property.camel_case} => { - % if not property.derived_from: - % if not property.allowed_in_keyframe_block: - if in_keyframe_block { - return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock) - } - % endif - % if property.internal: - if context.stylesheet_origin != Origin::UserAgent { - return Err(PropertyDeclarationParseError::UnknownProperty) - } - % endif - - ${property_pref_check(property)} - - match longhands::${property.ident}::parse_declared(context, input) { - Ok(value) => { - Ok(ParsedDeclaration::LonghandOrCustom( - PropertyDeclaration::${property.camel_case}(value) - )) - }, - Err(()) => Err(PropertyDeclarationParseError::InvalidValue), - } - % else: - Err(PropertyDeclarationParseError::UnknownProperty) - % endif - } - % endfor - }, - PropertyId::Shorthand(id) => match id { - % for shorthand in data.shorthands: - ShorthandId::${shorthand.camel_case} => { - % if not shorthand.allowed_in_keyframe_block: - if in_keyframe_block { - return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock) - } - % endif - % if shorthand.internal: - if context.stylesheet_origin != Origin::UserAgent { - return Err(PropertyDeclarationParseError::UnknownProperty) - } - % endif - - ${property_pref_check(shorthand)} - - match input.try(|i| CSSWideKeyword::parse(context, i)) { - Ok(keyword) => { - Ok(ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword)) - }, - Err(()) => { - shorthands::${shorthand.ident}::parse(context, input) - .map_err(|()| PropertyDeclarationParseError::InvalidValue) - } - } - } - % endfor - } - } - } - /// The shorthands that this longhand is part of. pub fn shorthands(&self) -> &'static [ShorthandId] { // first generate longhand to shorthands lookup map diff --git a/components/style/supports.rs b/components/style/supports.rs index ccdde3231bb..96c4bd3b5dc 100644 --- a/components/style/supports.rs +++ b/components/style/supports.rs @@ -6,7 +6,7 @@ use cssparser::{parse_important, Parser, Token}; use parser::ParserContext; -use properties::{PropertyDeclaration, PropertyId}; +use properties::{PropertyId, ParsedDeclaration}; use std::fmt; use style_traits::ToCss; @@ -211,7 +211,7 @@ impl Declaration { return false }; let mut input = Parser::new(&self.val); - let res = PropertyDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false); + let res = ParsedDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false); let _ = input.try(parse_important); res.is_ok() && input.is_exhausted() } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index c8b5d7013b6..d331293e530 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -67,7 +67,7 @@ use style::gecko_properties::{self, style_structs}; use style::keyframes::KeyframesStepValue; use style::parallel; use style::parser::{ParserContext, ParserContextExtraData}; -use style::properties::{ComputedValues, Importance, PropertyDeclaration}; +use style::properties::{ComputedValues, Importance, ParsedDeclaration}; use style::properties::{PropertyDeclarationBlock, PropertyId}; use style::properties::animated_properties::{AnimationValue, Interpolate, TransitionProperty}; use style::properties::parse_one_declaration; @@ -713,7 +713,7 @@ pub extern "C" fn Servo_ParseProperty(property: *const nsACString, value: *const Box::new(StdoutErrorReporter), extra_data); - match PropertyDeclaration::parse(id, &context, &mut Parser::new(value), false) { + match ParsedDeclaration::parse(id, &context, &mut Parser::new(value), false) { Ok(parsed) => { let mut declarations = Vec::new(); parsed.expand(|d| declarations.push((d, Importance::Normal))); From 460fd6eba86b9e316eae538a694ee8cacb98a2b5 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 19:38:25 +0100 Subject: [PATCH 07/11] Deduplicate declarations when inserting each one, not at the end of parsing. This will reduce the amount of re-allocations and copies. It will be further optimized with a bit map. --- components/style/keyframes.rs | 9 +-- .../style/properties/declaration_block.rs | 77 +++++++------------ ports/geckolib/glue.rs | 28 +++---- 3 files changed, 40 insertions(+), 74 deletions(-) diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs index 0fde0c64284..77cf7cc814d 100644 --- a/components/style/keyframes.rs +++ b/components/style/keyframes.rs @@ -362,10 +362,10 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> { context: self.context, }; let mut iter = DeclarationListParser::new(input, parser); - let mut declarations = Vec::new(); + let mut block = PropertyDeclarationBlock::new(); while let Some(declaration) = iter.next() { match declaration { - Ok(parsed) => parsed.expand(|d| declarations.push((d, Importance::Normal))), + Ok(parsed) => parsed.expand(|d| block.push(d, Importance::Normal)), Err(range) => { let pos = range.start; let message = format!("Unsupported keyframe property declaration: '{}'", @@ -377,10 +377,7 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> { } Ok(Arc::new(RwLock::new(Keyframe { selector: prelude, - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: declarations, - important_count: 0, - })), + block: Arc::new(RwLock::new(block)), }))) } } diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index ff6047289a6..683aa580e1a 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -60,6 +60,14 @@ pub struct PropertyDeclarationBlock { } impl PropertyDeclarationBlock { + /// Create an empty block + pub fn new() -> Self { + PropertyDeclarationBlock { + declarations: Vec::new(), + important_count: 0, + } + } + /// Returns wheather this block contains any declaration with `!important`. /// /// This is based on the `important_count` counter, @@ -169,11 +177,21 @@ impl PropertyDeclarationBlock { } /// Adds or overrides the declaration for a given property in this block, - /// without taking into account any kind of priority. Returns whether the - /// declaration block is actually changed. + /// except if an existing declaration for the same property is more important. + pub fn push(&mut self, declaration: PropertyDeclaration, importance: Importance) { + self.push_common(declaration, importance, false); + } + + /// Adds or overrides the declaration for a given property in this block, + /// Returns whether the declaration block is actually changed. pub fn set_parsed_declaration(&mut self, declaration: PropertyDeclaration, importance: Importance) -> bool { + self.push_common(declaration, importance, true) + } + + fn push_common(&mut self, declaration: PropertyDeclaration, importance: Importance, + overwrite_more_important: bool) -> bool { for slot in &mut *self.declarations { if slot.0.id() == declaration.id() { match (slot.1, importance) { @@ -181,7 +199,11 @@ impl PropertyDeclarationBlock { self.important_count += 1; } (Importance::Important, Importance::Normal) => { - self.important_count -= 1; + if overwrite_more_important { + self.important_count -= 1; + } else { + return false + } } _ => if slot.0 == declaration { return false; @@ -276,39 +298,6 @@ impl PropertyDeclarationBlock { } } } - - /// Only keep the "winning" declaration for any given property, by importance then source order. - pub fn deduplicate(&mut self) { - let mut deduplicated = Vec::with_capacity(self.declarations.len()); - let mut seen_normal = PropertyDeclarationIdSet::new(); - let mut seen_important = PropertyDeclarationIdSet::new(); - - for (declaration, importance) in self.declarations.drain(..).rev() { - if importance.important() { - let id = declaration.id(); - if seen_important.contains(id) { - self.important_count -= 1; - continue - } - if seen_normal.contains(id) { - let previous_len = deduplicated.len(); - deduplicated.retain(|&(ref d, _)| PropertyDeclaration::id(d) != id); - debug_assert_eq!(deduplicated.len(), previous_len - 1); - } - seen_important.insert(id); - } else { - let id = declaration.id(); - if seen_normal.contains(id) || - seen_important.contains(id) { - continue - } - seen_normal.insert(id) - } - deduplicated.push((declaration, importance)) - } - deduplicated.reverse(); - self.declarations = deduplicated; - } } impl ToCss for PropertyDeclarationBlock { @@ -603,21 +592,14 @@ impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> { pub fn parse_property_declaration_list(context: &ParserContext, input: &mut Parser) -> PropertyDeclarationBlock { - let mut declarations = Vec::new(); - let mut important_count = 0; + let mut block = PropertyDeclarationBlock::new(); let parser = PropertyDeclarationParser { context: context, }; let mut iter = DeclarationListParser::new(input, parser); while let Some(declaration) = iter.next() { match declaration { - Ok((parsed, importance)) => { - let old_len = declarations.len(); - parsed.expand(|d| declarations.push((d, importance))); - if importance.important() { - important_count += declarations.len() - old_len; - } - } + Ok((parsed, importance)) => parsed.expand(|d| block.push(d, importance)), Err(range) => { let pos = range.start; let message = format!("Unsupported property declaration: '{}'", @@ -626,10 +608,5 @@ pub fn parse_property_declaration_list(context: &ParserContext, } } } - let mut block = PropertyDeclarationBlock { - declarations: declarations, - important_count: important_count, - }; - block.deduplicate(); block } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index d331293e530..a700e2a33aa 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -251,18 +251,13 @@ pub extern "C" fn Servo_AnimationValues_Uncompute(value: RawServoAnimationValueB -> RawServoDeclarationBlockStrong { let value = unsafe { value.as_ref().unwrap() }; - let uncomputed_values = value.into_iter() - .map(|v| { - let raw_anim = unsafe { v.as_ref().unwrap() }; - let anim = AnimationValue::as_arc(&raw_anim); - (anim.uncompute(), Importance::Normal) - }) - .collect(); - - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: uncomputed_values, - important_count: 0, - })).into_strong() + let mut block = PropertyDeclarationBlock::new(); + for v in value.iter() { + let raw_anim = unsafe { v.as_ref().unwrap() }; + let anim = AnimationValue::as_arc(&raw_anim); + block.push(anim.uncompute(), Importance::Normal); + } + Arc::new(RwLock::new(block)).into_strong() } macro_rules! get_property_id_from_nscsspropertyid { @@ -715,12 +710,9 @@ pub extern "C" fn Servo_ParseProperty(property: *const nsACString, value: *const match ParsedDeclaration::parse(id, &context, &mut Parser::new(value), false) { Ok(parsed) => { - let mut declarations = Vec::new(); - parsed.expand(|d| declarations.push((d, Importance::Normal))); - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: declarations, - important_count: 0, - })).into_strong() + let mut block = PropertyDeclarationBlock::new(); + parsed.expand(|d| block.push(d, Importance::Normal)); + Arc::new(RwLock::new(block)).into_strong() } Err(_) => RawServoDeclarationBlockStrong::null() } From da4e5146e97078ac1cf1b6ed28748bf581b4bdeb Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 19:45:12 +0100 Subject: [PATCH 08/11] Make PropertyDeclarationBlock::important_count private. --- components/script/dom/cssstyledeclaration.rs | 10 +- components/script/dom/element.rs | 7 +- .../style/properties/declaration_block.rs | 10 +- components/style/properties/gecko.mako.rs | 30 ++- ports/geckolib/glue.rs | 15 +- tests/unit/style/keyframes.rs | 107 +++++------ tests/unit/style/properties/serialization.rs | 21 +-- tests/unit/style/rule_tree/bench.rs | 13 +- tests/unit/style/stylesheets.rs | 176 +++++++++--------- tests/unit/style/stylist.rs | 13 +- 10 files changed, 183 insertions(+), 219 deletions(-) diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs index 450d0b7da4b..dc53a58c8c5 100644 --- a/components/script/dom/cssstyledeclaration.rs +++ b/components/script/dom/cssstyledeclaration.rs @@ -62,10 +62,7 @@ impl CSSStyleOwner { let result = f(&mut pdb, &mut changed); result } else { - let mut pdb = PropertyDeclarationBlock { - important_count: 0, - declarations: vec![], - }; + let mut pdb = PropertyDeclarationBlock::new(); let result = f(&mut pdb, &mut changed); // Here `changed` is somewhat silly, because we know the @@ -116,10 +113,7 @@ impl CSSStyleOwner { match *el.style_attribute().borrow() { Some(ref pdb) => f(&pdb.read()), None => { - let pdb = PropertyDeclarationBlock { - important_count: 0, - declarations: vec![], - }; + let pdb = PropertyDeclarationBlock::new(); f(&pdb) } } diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index eeff67ed318..a0fb1d0f179 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -385,10 +385,9 @@ impl LayoutElementHelpers for LayoutJS { #[inline] fn from_declaration(declaration: PropertyDeclaration) -> ApplicableDeclarationBlock { ApplicableDeclarationBlock::from_declarations( - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![(declaration, Importance::Normal)], - important_count: 0, - })), + Arc::new(RwLock::new(PropertyDeclarationBlock::with_one( + declaration, Importance::Normal + ))), CascadeLevel::PresHints) } diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 683aa580e1a..a88fee2856e 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -56,7 +56,7 @@ pub struct PropertyDeclarationBlock { pub declarations: Vec<(PropertyDeclaration, Importance)>, /// The number of entries in `self.declaration` with `Importance::Important` - pub important_count: usize, + important_count: usize, } impl PropertyDeclarationBlock { @@ -68,6 +68,14 @@ impl PropertyDeclarationBlock { } } + /// Create a block with a single declaration + pub fn with_one(declaration: PropertyDeclaration, importance: Importance) -> Self { + PropertyDeclarationBlock { + declarations: vec![(declaration, importance)], + important_count: if importance.important() { 1 } else { 0 }, + } + } + /// Returns wheather this block contains any declaration with `!important`. /// /// This is based on the `important_count` counter, diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 69a1cca08bb..8e52d9d739c 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -168,23 +168,19 @@ impl ComputedValues { % for prop in data.longhands: % if prop.animatable: PropertyDeclarationId::Longhand(LonghandId::${prop.camel_case}) => { - PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::${prop.camel_case}(DeclaredValue::Value( - % if prop.boxed: - Box::new( - % endif - longhands::${prop.ident}::SpecifiedValue::from_computed_value( - &self.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}()) - % if prop.boxed: - ) - % endif - - )), - Importance::Normal) - ], - important_count: 0 - } + PropertyDeclarationBlock::with_one( + PropertyDeclaration::${prop.camel_case}(DeclaredValue::Value( + % if prop.boxed: + Box::new( + % endif + longhands::${prop.ident}::SpecifiedValue::from_computed_value( + &self.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}()) + % if prop.boxed: + ) + % endif + )), + Importance::Normal + ) }, % endif % endfor diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index a700e2a33aa..31bf032fc21 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -276,10 +276,8 @@ pub extern "C" fn Servo_AnimationValue_Serialize(value: RawServoAnimationValueBo { let uncomputed_value = AnimationValue::as_arc(&value).uncompute(); let mut string = String::new(); - let rv = PropertyDeclarationBlock { - declarations: vec![(uncomputed_value, Importance::Normal)], - important_count: 0 - }.single_value_to_css(&get_property_id_from_nscsspropertyid!(property, ()), &mut string); + let rv = PropertyDeclarationBlock::with_one(uncomputed_value, Importance::Normal) + .single_value_to_css(&get_property_id_from_nscsspropertyid!(property, ()), &mut string); debug_assert!(rv.is_ok()); write!(unsafe { &mut *buffer }, "{}", string).expect("Failed to copy string"); @@ -726,7 +724,7 @@ pub extern "C" fn Servo_ParseStyleAttribute(data: *const nsACString) -> RawServo #[no_mangle] pub extern "C" fn Servo_DeclarationBlock_CreateEmpty() -> RawServoDeclarationBlockStrong { - Arc::new(RwLock::new(PropertyDeclarationBlock { declarations: vec![], important_count: 0 })).into_strong() + Arc::new(RwLock::new(PropertyDeclarationBlock::new())).into_strong() } #[no_mangle] @@ -1479,10 +1477,9 @@ pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSet (*keyframe).mPropertyValues.set_len((index + 1) as u32); (*keyframe).mPropertyValues[index].mProperty = property.into(); (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky( - Arc::new(RwLock::new( - PropertyDeclarationBlock { declarations: vec![ (declaration.clone(), - Importance::Normal) ], - important_count: 0 }))); + Arc::new(RwLock::new(PropertyDeclarationBlock::with_one( + declaration.clone(), Importance::Normal + )))); if step.start_percentage.0 == 0. || step.start_percentage.0 == 1. { seen.set_transition_property_bit(&property); diff --git a/tests/unit/style/keyframes.rs b/tests/unit/style/keyframes.rs index d01dbcab2d0..a1861754419 100644 --- a/tests/unit/style/keyframes.rs +++ b/tests/unit/style/keyframes.rs @@ -27,10 +27,7 @@ fn test_no_property_in_keyframe() { let keyframes = vec![ Arc::new(RwLock::new(Keyframe { selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]), - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![], - important_count: 0, - })) + block: Arc::new(RwLock::new(PropertyDeclarationBlock::new())) })), ]; let animation = KeyframesAnimation::from_keyframes(&keyframes); @@ -45,27 +42,26 @@ fn test_no_property_in_keyframe() { #[test] fn test_missing_property_in_initial_keyframe() { let declarations_on_initial_keyframe = - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Width( - DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - ], - important_count: 0, - })); + Arc::new(RwLock::new(PropertyDeclarationBlock::with_one( + PropertyDeclaration::Width( + DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), + Importance::Normal + ))); let declarations_on_final_keyframe = - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Width( + Arc::new(RwLock::new({ + let mut block = PropertyDeclarationBlock::new(); + block.push( + PropertyDeclaration::Width( DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - - (PropertyDeclaration::Height( + Importance::Normal + ); + block.push( + PropertyDeclaration::Height( DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - ], - important_count: 0, + Importance::Normal + ); + block })); let keyframes = vec![ @@ -102,28 +98,27 @@ fn test_missing_property_in_initial_keyframe() { #[test] fn test_missing_property_in_final_keyframe() { let declarations_on_initial_keyframe = - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Width( + Arc::new(RwLock::new({ + let mut block = PropertyDeclarationBlock::new(); + block.push( + PropertyDeclaration::Width( DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - - (PropertyDeclaration::Height( + Importance::Normal + ); + block.push( + PropertyDeclaration::Height( DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - ], - important_count: 0, + Importance::Normal + ); + block })); let declarations_on_final_keyframe = - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Height( - DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - ], - important_count: 0, - })); + Arc::new(RwLock::new(PropertyDeclarationBlock::with_one( + PropertyDeclaration::Height( + DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), + Importance::Normal, + ))); let keyframes = vec![ Arc::new(RwLock::new(Keyframe { @@ -159,26 +154,25 @@ fn test_missing_property_in_final_keyframe() { #[test] fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() { let declarations = - Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Width( - DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - - (PropertyDeclaration::Height( - DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), - Importance::Normal), - ], - important_count: 0, + Arc::new(RwLock::new({ + let mut block = PropertyDeclarationBlock::new(); + block.push( + PropertyDeclaration::Width( + DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), + Importance::Normal + ); + block.push( + PropertyDeclaration::Height( + DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))), + Importance::Normal + ); + block })); let keyframes = vec![ Arc::new(RwLock::new(Keyframe { selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]), - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![], - important_count: 0, - })) + block: Arc::new(RwLock::new(PropertyDeclarationBlock::new())) })), Arc::new(RwLock::new(Keyframe { selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.5)]), @@ -191,11 +185,10 @@ fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() { KeyframesStep { start_percentage: KeyframePercentage(0.), value: KeyframesStepValue::Declarations { - block: Arc::new(RwLock::new(PropertyDeclarationBlock { + block: Arc::new(RwLock::new( // XXX: Should we use ComputedValues in this case? - declarations: vec![], - important_count: 0, - })) + PropertyDeclarationBlock::new() + )) }, declared_timing_function: false, }, diff --git a/tests/unit/style/properties/serialization.rs b/tests/unit/style/properties/serialization.rs index 9433d97ed3d..f69e75ed571 100644 --- a/tests/unit/style/properties/serialization.rs +++ b/tests/unit/style/properties/serialization.rs @@ -17,6 +17,7 @@ use style::values::specified::{BorderStyle, BorderWidth, CSSColor, Length, NoCal use style::values::specified::{LengthOrPercentage, LengthOrPercentageOrAuto, LengthOrPercentageOrAutoOrContent}; use style::values::specified::url::SpecifiedUrl; use style_traits::ToCss; +use stylesheets::block_from; fn parse_declaration_block(css_properties: &str) -> PropertyDeclarationBlock { let url = ServoUrl::parse("http://localhost").unwrap(); @@ -56,10 +57,7 @@ fn property_declaration_block_should_serialize_correctly() { Importance::Normal), ]; - let block = PropertyDeclarationBlock { - declarations: declarations, - important_count: 0, - }; + let block = block_from(declarations); let css_string = block.to_css_string(); @@ -73,10 +71,7 @@ mod shorthand_serialization { pub use super::*; pub fn shorthand_properties_to_string(properties: Vec) -> String { - let block = PropertyDeclarationBlock { - declarations: properties.into_iter().map(|d| (d, Importance::Normal)).collect(), - important_count: 0, - }; + let block = block_from(properties.into_iter().map(|d| (d, Importance::Normal))); block.to_css_string() } @@ -882,10 +877,7 @@ mod shorthand_serialization { Importance::Normal) ]; - let block = PropertyDeclarationBlock { - declarations: declarations, - important_count: 0 - }; + let block = block_from(declarations); let mut s = String::new(); @@ -905,10 +897,7 @@ mod shorthand_serialization { Importance::Normal) ]; - let block = PropertyDeclarationBlock { - declarations: declarations, - important_count: 0 - }; + let block = block_from(declarations); let mut s = String::new(); diff --git a/tests/unit/style/rule_tree/bench.rs b/tests/unit/style/rule_tree/bench.rs index ba76d79b861..74242967766 100644 --- a/tests/unit/style/rule_tree/bench.rs +++ b/tests/unit/style/rule_tree/bench.rs @@ -68,14 +68,11 @@ fn test_insertion(rule_tree: &RuleTree, rules: Vec<(StyleSource, CascadeLevel)>) fn test_insertion_style_attribute(rule_tree: &RuleTree, rules: &[(StyleSource, CascadeLevel)]) -> StrongRuleNode { let mut rules = rules.to_vec(); - rules.push((StyleSource::Declarations(Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Display(DeclaredValue::Value( - longhands::display::SpecifiedValue::block)), - Importance::Normal), - ], - important_count: 0, - }))), CascadeLevel::UserNormal)); + rules.push((StyleSource::Declarations(Arc::new(RwLock::new(PropertyDeclarationBlock::with_one( + PropertyDeclaration::Display(DeclaredValue::Value( + longhands::display::SpecifiedValue::block)), + Importance::Normal + )))), CascadeLevel::UserNormal)); test_insertion(rule_tree, rules) } diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index 586f2616ecf..a85e5e36fb0 100644 --- a/tests/unit/style/stylesheets.rs +++ b/tests/unit/style/stylesheets.rs @@ -24,6 +24,15 @@ use style::stylesheets::{Origin, Namespaces}; use style::stylesheets::{Stylesheet, NamespaceRule, CssRule, CssRules, StyleRule, KeyframesRule}; use style::values::specified::{LengthOrPercentageOrAuto, Percentage}; +pub fn block_from(iterable: I) -> PropertyDeclarationBlock +where I: IntoIterator { + let mut block = PropertyDeclarationBlock::new(); + for (d, i) in iterable { + block.push(d, i) + } + block +} + #[test] fn test_parse_stylesheet() { let css = r" @@ -98,17 +107,14 @@ fn test_parse_stylesheet() { specificity: (0 << 20) + (1 << 10) + (1 << 0), }, ]), - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Display(DeclaredValue::Value( - longhands::display::SpecifiedValue::none)), - Importance::Important), - (PropertyDeclaration::Custom(Atom::from("a"), - DeclaredValue::CSSWideKeyword(CSSWideKeyword::Inherit)), - Importance::Important), - ], - important_count: 2, - })), + block: Arc::new(RwLock::new(block_from(vec![ + (PropertyDeclaration::Display(DeclaredValue::Value( + longhands::display::SpecifiedValue::none)), + Importance::Important), + (PropertyDeclaration::Custom(Atom::from("a"), + DeclaredValue::CSSWideKeyword(CSSWideKeyword::Inherit)), + Importance::Important), + ]))), }))), CssRule::Style(Arc::new(RwLock::new(StyleRule { selectors: SelectorList(vec![ @@ -147,14 +153,11 @@ fn test_parse_stylesheet() { specificity: (0 << 20) + (0 << 10) + (1 << 0), }, ]), - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Display(DeclaredValue::Value( - longhands::display::SpecifiedValue::block)), - Importance::Normal), - ], - important_count: 0, - })), + block: Arc::new(RwLock::new(block_from(vec![ + (PropertyDeclaration::Display(DeclaredValue::Value( + longhands::display::SpecifiedValue::block)), + Importance::Normal), + ]))), }))), CssRule::Style(Arc::new(RwLock::new(StyleRule { selectors: SelectorList(vec![ @@ -182,58 +185,55 @@ fn test_parse_stylesheet() { specificity: (1 << 20) + (1 << 10) + (0 << 0), }, ]), - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::BackgroundColor(DeclaredValue::Value( - longhands::background_color::SpecifiedValue { - authored: Some("blue".to_owned().into_boxed_str()), - parsed: cssparser::Color::RGBA(cssparser::RGBA::new(0, 0, 255, 255)), - } - )), - Importance::Normal), - (PropertyDeclaration::BackgroundPositionX(DeclaredValue::Value( - longhands::background_position_x::SpecifiedValue( - vec![longhands::background_position_x::single_value - ::get_initial_position_value()]))), - Importance::Normal), - (PropertyDeclaration::BackgroundPositionY(DeclaredValue::Value( - longhands::background_position_y::SpecifiedValue( - vec![longhands::background_position_y::single_value - ::get_initial_position_value()]))), - Importance::Normal), - (PropertyDeclaration::BackgroundRepeat(DeclaredValue::Value( - longhands::background_repeat::SpecifiedValue( - vec![longhands::background_repeat::single_value - ::get_initial_specified_value()]))), - Importance::Normal), - (PropertyDeclaration::BackgroundAttachment(DeclaredValue::Value( - longhands::background_attachment::SpecifiedValue( - vec![longhands::background_attachment::single_value - ::get_initial_specified_value()]))), - Importance::Normal), - (PropertyDeclaration::BackgroundImage(DeclaredValue::Value( - longhands::background_image::SpecifiedValue( - vec![longhands::background_image::single_value - ::get_initial_specified_value()]))), - Importance::Normal), - (PropertyDeclaration::BackgroundSize(DeclaredValue::Value( - longhands::background_size::SpecifiedValue( - vec![longhands::background_size::single_value - ::get_initial_specified_value()]))), - Importance::Normal), - (PropertyDeclaration::BackgroundOrigin(DeclaredValue::Value( - longhands::background_origin::SpecifiedValue( - vec![longhands::background_origin::single_value - ::get_initial_specified_value()]))), - Importance::Normal), - (PropertyDeclaration::BackgroundClip(DeclaredValue::Value( - longhands::background_clip::SpecifiedValue( - vec![longhands::background_clip::single_value - ::get_initial_specified_value()]))), - Importance::Normal), - ], - important_count: 0, - })), + block: Arc::new(RwLock::new(block_from(vec![ + (PropertyDeclaration::BackgroundColor(DeclaredValue::Value( + longhands::background_color::SpecifiedValue { + authored: Some("blue".to_owned().into_boxed_str()), + parsed: cssparser::Color::RGBA(cssparser::RGBA::new(0, 0, 255, 255)), + } + )), + Importance::Normal), + (PropertyDeclaration::BackgroundPositionX(DeclaredValue::Value( + longhands::background_position_x::SpecifiedValue( + vec![longhands::background_position_x::single_value + ::get_initial_position_value()]))), + Importance::Normal), + (PropertyDeclaration::BackgroundPositionY(DeclaredValue::Value( + longhands::background_position_y::SpecifiedValue( + vec![longhands::background_position_y::single_value + ::get_initial_position_value()]))), + Importance::Normal), + (PropertyDeclaration::BackgroundRepeat(DeclaredValue::Value( + longhands::background_repeat::SpecifiedValue( + vec![longhands::background_repeat::single_value + ::get_initial_specified_value()]))), + Importance::Normal), + (PropertyDeclaration::BackgroundAttachment(DeclaredValue::Value( + longhands::background_attachment::SpecifiedValue( + vec![longhands::background_attachment::single_value + ::get_initial_specified_value()]))), + Importance::Normal), + (PropertyDeclaration::BackgroundImage(DeclaredValue::Value( + longhands::background_image::SpecifiedValue( + vec![longhands::background_image::single_value + ::get_initial_specified_value()]))), + Importance::Normal), + (PropertyDeclaration::BackgroundSize(DeclaredValue::Value( + longhands::background_size::SpecifiedValue( + vec![longhands::background_size::single_value + ::get_initial_specified_value()]))), + Importance::Normal), + (PropertyDeclaration::BackgroundOrigin(DeclaredValue::Value( + longhands::background_origin::SpecifiedValue( + vec![longhands::background_origin::single_value + ::get_initial_specified_value()]))), + Importance::Normal), + (PropertyDeclaration::BackgroundClip(DeclaredValue::Value( + longhands::background_clip::SpecifiedValue( + vec![longhands::background_clip::single_value + ::get_initial_specified_value()]))), + Importance::Normal), + ]))), }))), CssRule::Keyframes(Arc::new(RwLock::new(KeyframesRule { name: "foo".into(), @@ -241,30 +241,24 @@ fn test_parse_stylesheet() { Arc::new(RwLock::new(Keyframe { selector: KeyframeSelector::new_for_unit_testing( vec![KeyframePercentage::new(0.)]), - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Width(DeclaredValue::Value( - LengthOrPercentageOrAuto::Percentage(Percentage(0.)))), - Importance::Normal), - ], - important_count: 0, - })) + block: Arc::new(RwLock::new(block_from(vec![ + (PropertyDeclaration::Width(DeclaredValue::Value( + LengthOrPercentageOrAuto::Percentage(Percentage(0.)))), + Importance::Normal), + ]))) })), Arc::new(RwLock::new(Keyframe { selector: KeyframeSelector::new_for_unit_testing( vec![KeyframePercentage::new(1.)]), - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Width(DeclaredValue::Value( - LengthOrPercentageOrAuto::Percentage(Percentage(1.)))), - Importance::Normal), - (PropertyDeclaration::AnimationPlayState(DeclaredValue::Value( - animation_play_state::SpecifiedValue( - vec![animation_play_state::SingleSpecifiedValue::running]))), - Importance::Normal), - ], - important_count: 0, - })), + block: Arc::new(RwLock::new(block_from(vec![ + (PropertyDeclaration::Width(DeclaredValue::Value( + LengthOrPercentageOrAuto::Percentage(Percentage(1.)))), + Importance::Normal), + (PropertyDeclaration::AnimationPlayState(DeclaredValue::Value( + animation_play_state::SpecifiedValue( + vec![animation_play_state::SingleSpecifiedValue::running]))), + Importance::Normal), + ]))), })), ] }))) diff --git a/tests/unit/style/stylist.rs b/tests/unit/style/stylist.rs index abb8f459211..46fb3eef164 100644 --- a/tests/unit/style/stylist.rs +++ b/tests/unit/style/stylist.rs @@ -23,14 +23,11 @@ fn get_mock_rules(css_selectors: &[&str]) -> Vec> { let rule = Arc::new(RwLock::new(StyleRule { selectors: selectors, - block: Arc::new(RwLock::new(PropertyDeclarationBlock { - declarations: vec![ - (PropertyDeclaration::Display(DeclaredValue::Value( - longhands::display::SpecifiedValue::block)), - Importance::Normal), - ], - important_count: 0, - })), + block: Arc::new(RwLock::new(PropertyDeclarationBlock::with_one( + PropertyDeclaration::Display(DeclaredValue::Value( + longhands::display::SpecifiedValue::block)), + Importance::Normal + ))), })); let guard = rule.read(); From f70a49974a001e34cb65d55f315702966e29c6c3 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 19:30:51 +0100 Subject: [PATCH 09/11] Make PropertyDeclarationBlock fields private --- components/script/dom/cssstyledeclaration.rs | 6 ++-- components/style/animation.rs | 4 +-- components/style/keyframes.rs | 4 +-- .../style/properties/declaration_block.rs | 14 ++++------ .../style/properties/properties.mako.rs | 2 +- components/style/rule_tree/mod.rs | 2 +- components/style/stylesheets.rs | 4 +-- ports/geckolib/glue.rs | 28 +++++++++---------- 8 files changed, 31 insertions(+), 33 deletions(-) diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs index dc53a58c8c5..8af3b3975d2 100644 --- a/components/script/dom/cssstyledeclaration.rs +++ b/components/script/dom/cssstyledeclaration.rs @@ -67,7 +67,7 @@ impl CSSStyleOwner { // Here `changed` is somewhat silly, because we know the // exact conditions under it changes. - changed = !pdb.declarations.is_empty(); + changed = !pdb.declarations().is_empty(); if changed { attr = Some(Arc::new(RwLock::new(pdb))); } @@ -274,7 +274,7 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration { // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-length fn Length(&self) -> u32 { self.owner.with_block(|pdb| { - pdb.declarations.len() as u32 + pdb.declarations().len() as u32 }) } @@ -399,7 +399,7 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration { // https://dev.w3.org/csswg/cssom/#the-cssstyledeclaration-interface fn IndexedGetter(&self, index: u32) -> Option { self.owner.with_block(|pdb| { - pdb.declarations.get(index as usize).map(|entry| { + pdb.declarations().get(index as usize).map(|entry| { let (ref declaration, importance) = *entry; let mut css = declaration.to_css_string(); if importance.important() { diff --git a/components/style/animation.rs b/components/style/animation.rs index aa115527d57..bebda0e6339 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -418,11 +418,11 @@ fn compute_style_for_animation_step(context: &SharedStyleContext, let guard = declarations.read(); // No !important in keyframes. - debug_assert!(guard.declarations.iter() + debug_assert!(guard.declarations().iter() .all(|&(_, importance)| importance == Importance::Normal)); let iter = || { - guard.declarations.iter().rev().map(|&(ref decl, _importance)| decl) + guard.declarations().iter().rev().map(|&(ref decl, _importance)| decl) }; let computed = diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs index 77cf7cc814d..31ba401cddb 100644 --- a/components/style/keyframes.rs +++ b/components/style/keyframes.rs @@ -181,7 +181,7 @@ impl KeyframesStep { value: KeyframesStepValue) -> Self { let declared_timing_function = match value { KeyframesStepValue::Declarations { ref block } => { - block.read().declarations.iter().any(|&(ref prop_decl, _)| { + block.read().declarations().iter().any(|&(ref prop_decl, _)| { match *prop_decl { PropertyDeclaration::AnimationTimingFunction(..) => true, _ => false, @@ -249,7 +249,7 @@ fn get_animated_properties(keyframes: &[Arc>]) -> Vec Self { - Importance::Normal - } -} - /// Overridden declarations are skipped. #[derive(Debug, PartialEq, Clone)] pub struct PropertyDeclarationBlock { /// The group of declarations, along with their importance. /// /// Only deduplicated declarations appear here. - pub declarations: Vec<(PropertyDeclaration, Importance)>, + declarations: Vec<(PropertyDeclaration, Importance)>, /// The number of entries in `self.declaration` with `Importance::Important` important_count: usize, @@ -76,6 +69,11 @@ impl PropertyDeclarationBlock { } } + /// The declarations in this block + pub fn declarations(&self) -> &[(PropertyDeclaration, Importance)] { + &self.declarations + } + /// Returns wheather this block contains any declaration with `!important`. /// /// This is based on the `important_count` counter, diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 74b28d25d95..849cb74e965 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1817,7 +1817,7 @@ pub fn cascade(viewport_size: Size2D, }).collect::>(); let iter_declarations = || { lock_guards.iter().flat_map(|&(ref source, source_importance)| { - source.declarations.iter() + source.declarations().iter() // Yield declarations later in source order (with more precedence) first. .rev() .filter_map(move |&(ref declaration, declaration_importance)| { diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index 1ecd0a0cb14..9a725a19286 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -99,7 +99,7 @@ impl StyleSource { let _ = write!(writer, "{:?}", rule.read().selectors); } - let _ = write!(writer, " -> {:?}", self.read().declarations); + let _ = write!(writer, " -> {:?}", self.read().declarations()); } /// Read the style source guard, and obtain thus read access to the diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index 758d81125b2..cc72acd4c15 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -534,7 +534,7 @@ impl ToCss for StyleRule { let declaration_block = self.block.read(); try!(declaration_block.to_css(dest)); // Step 4 - if declaration_block.declarations.len() > 0 { + if declaration_block.declarations().len() > 0 { try!(write!(dest, " ")); } // Step 5 @@ -952,7 +952,7 @@ impl<'a> QualifiedRuleParser for TopLevelRuleParser<'a> { } } -#[derive(Clone)] // shallow, relatively cheap clone +#[derive(Clone)] // shallow, relatively cheap .clone struct NestedRuleParser<'a, 'b: 'a> { stylesheet_origin: Origin, context: &'a ParserContext<'b>, diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 31bf032fc21..509e7efe316 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -765,14 +765,14 @@ pub extern "C" fn Servo_DeclarationBlock_SerializeOneValue( #[no_mangle] pub extern "C" fn Servo_DeclarationBlock_Count(declarations: RawServoDeclarationBlockBorrowed) -> u32 { let declarations = RwLock::::as_arc(&declarations); - declarations.read().declarations.len() as u32 + declarations.read().declarations().len() as u32 } #[no_mangle] pub extern "C" fn Servo_DeclarationBlock_GetNthProperty(declarations: RawServoDeclarationBlockBorrowed, index: u32, result: *mut nsAString) -> bool { let declarations = RwLock::::as_arc(&declarations); - if let Some(&(ref decl, _)) = declarations.read().declarations.get(index as usize) { + if let Some(&(ref decl, _)) = declarations.read().declarations().get(index as usize) { let result = unsafe { result.as_mut().unwrap() }; decl.id().to_css(result).unwrap(); true @@ -923,7 +923,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetIdentStringValue(declarations: let prop = match_wrap_declared! { long, XLang => Lang(Atom::from(value)), }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -960,7 +960,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetKeywordValue(declarations: BorderBottomStyle => BorderStyle::from_gecko_keyword(value), BorderLeftStyle => BorderStyle::from_gecko_keyword(value), }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -975,7 +975,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetIntValue(declarations: RawServoDecla let prop = match_wrap_declared! { long, XSpan => Span(value), }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -1014,7 +1014,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetPixelValue(declarations: } ), }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -1037,7 +1037,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetPercentValue(declarations: MarginBottom => pc.into(), MarginLeft => pc.into(), }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -1059,7 +1059,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetAutoValue(declarations: MarginBottom => auto, MarginLeft => auto, }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -1080,7 +1080,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetCurrentColor(declarations: BorderBottomColor => cc, BorderLeftColor => cc, }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -1107,7 +1107,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetColorValue(declarations: Color => longhands::color::SpecifiedValue(color), BackgroundColor => color, }; - declarations.write().declarations.push((prop, Default::default())); + declarations.write().push(prop, Importance::Normal); } #[no_mangle] @@ -1124,7 +1124,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetFontFamily(declarations: if let Ok(family) = FontFamily::parse(&mut parser) { if parser.is_exhausted() { let decl = PropertyDeclaration::FontFamily(DeclaredValue::Value(family)); - declarations.write().declarations.push((decl, Default::default())); + declarations.write().push(decl, Importance::Normal); } } } @@ -1139,7 +1139,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetTextDecorationColorOverride(declarat let mut decoration = text_decoration_line::computed_value::none; decoration |= text_decoration_line::COLOR_OVERRIDE; let decl = PropertyDeclaration::TextDecorationLine(DeclaredValue::Value(decoration)); - declarations.write().declarations.push((decl, Default::default())); + declarations.write().push(decl, Importance::Normal); } #[no_mangle] @@ -1356,7 +1356,7 @@ pub extern "C" fn Servo_GetComputedKeyframeValues(keyframes: RawGeckoKeyframeLis let declarations = RwLock::::as_arc(&declarations); let guard = declarations.read(); - let anim_iter = guard.declarations + let anim_iter = guard.declarations() .iter() .filter_map(|&(ref decl, imp)| { if imp == Importance::Normal { @@ -1463,7 +1463,7 @@ pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSet let guard = block.read(); // Filter out non-animatable properties. let animatable = - guard.declarations + guard.declarations() .iter() .filter(|&&(ref declaration, _)| { declaration.is_animatable() From 60f454d7c4ca2249bf914b9f6180e53df9ebe4a4 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 7 Mar 2017 20:12:06 +0100 Subject: [PATCH 10/11] Use a bitmap to optimize adding to a PropertyDeclarationBlock. --- .../style/properties/declaration_block.rs | 68 ++++++++++++++----- .../style/properties/properties.mako.rs | 8 +++ ports/geckolib/glue.rs | 3 +- 3 files changed, 61 insertions(+), 18 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 0ddf73a62d5..2cb5cba4874 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -41,7 +41,7 @@ impl Importance { } /// Overridden declarations are skipped. -#[derive(Debug, PartialEq, Clone)] +#[derive(Clone)] pub struct PropertyDeclarationBlock { /// The group of declarations, along with their importance. /// @@ -50,6 +50,14 @@ pub struct PropertyDeclarationBlock { /// The number of entries in `self.declaration` with `Importance::Important` important_count: usize, + + longhands: LonghandIdSet, +} + +impl fmt::Debug for PropertyDeclarationBlock { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.declarations.fmt(f) + } } impl PropertyDeclarationBlock { @@ -58,14 +66,20 @@ impl PropertyDeclarationBlock { PropertyDeclarationBlock { declarations: Vec::new(), important_count: 0, + longhands: LonghandIdSet::new(), } } /// Create a block with a single declaration pub fn with_one(declaration: PropertyDeclaration, importance: Importance) -> Self { + let mut longhands = LonghandIdSet::new(); + if let PropertyDeclarationId::Longhand(id) = declaration.id() { + longhands.insert(id); + } PropertyDeclarationBlock { declarations: vec![(declaration, importance)], important_count: if importance.important() { 1 } else { 0 }, + longhands: longhands, } } @@ -198,28 +212,39 @@ impl PropertyDeclarationBlock { fn push_common(&mut self, declaration: PropertyDeclaration, importance: Importance, overwrite_more_important: bool) -> bool { - for slot in &mut *self.declarations { - if slot.0.id() == declaration.id() { - match (slot.1, importance) { - (Importance::Normal, Importance::Important) => { - self.important_count += 1; - } - (Importance::Important, Importance::Normal) => { - if overwrite_more_important { - self.important_count -= 1; - } else { - return false + let definitely_new = if let PropertyDeclarationId::Longhand(id) = declaration.id() { + !self.longhands.contains(id) + } else { + false // For custom properties, always scan + }; + + if !definitely_new { + for slot in &mut *self.declarations { + if slot.0.id() == declaration.id() { + match (slot.1, importance) { + (Importance::Normal, Importance::Important) => { + self.important_count += 1; + } + (Importance::Important, Importance::Normal) => { + if overwrite_more_important { + self.important_count -= 1; + } else { + return false + } + } + _ => if slot.0 == declaration { + return false; } } - _ => if slot.0 == declaration { - return false; - } + *slot = (declaration, importance); + return true } - *slot = (declaration, importance); - return true; } } + if let PropertyDeclarationId::Longhand(id) = declaration.id() { + self.longhands.insert(id); + } self.declarations.push((declaration, importance)); if importance.important() { self.important_count += 1; @@ -256,6 +281,11 @@ impl PropertyDeclarationBlock { /// /// Returns whether any declaration was actually removed. pub fn remove_property(&mut self, property: &PropertyId) -> bool { + if let PropertyId::Longhand(id) = *property { + if !self.longhands.contains(id) { + return false + } + } let important_count = &mut self.important_count; let mut removed_at_least_one = false; self.declarations.retain(|&(ref declaration, importance)| { @@ -269,6 +299,10 @@ impl PropertyDeclarationBlock { !remove }); + if let PropertyId::Longhand(id) = *property { + debug_assert!(removed_at_least_one); + self.longhands.remove(id); + } removed_at_least_one } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 849cb74e965..5eb133601fe 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -177,6 +177,7 @@ pub mod animated_properties { } /// A set of longhand properties +#[derive(Clone)] pub struct LonghandIdSet { storage: [u32; (${len(data.longhands)} - 1 + 32) / 32] } @@ -202,6 +203,13 @@ impl LonghandIdSet { self.storage[bit / 32] |= 1 << (bit % 32); } + /// Remove the given property from the set + #[inline] + pub fn remove(&mut self, id: LonghandId) { + let bit = id as usize; + self.storage[bit / 32] &= !(1 << (bit % 32)); + } + /// Set the corresponding bit of TransitionProperty. /// This function will panic if TransitionProperty::All is given. pub fn set_transition_property_bit(&mut self, property: &TransitionProperty) { diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 509e7efe316..06f3c0f3de8 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -738,7 +738,8 @@ pub extern "C" fn Servo_DeclarationBlock_Clone(declarations: RawServoDeclaration pub extern "C" fn Servo_DeclarationBlock_Equals(a: RawServoDeclarationBlockBorrowed, b: RawServoDeclarationBlockBorrowed) -> bool { - *RwLock::::as_arc(&a).read() == *RwLock::::as_arc(&b).read() + *RwLock::::as_arc(&a).read().declarations() == + *RwLock::::as_arc(&b).read().declarations() } #[no_mangle] From 446aaa584533ece729fedc5658cd1920fc850c21 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Mar 2017 12:03:47 +0100 Subject: [PATCH 11/11] Better variable name, remove obsolete comment --- components/style/keyframes.rs | 1 - components/style/properties/properties.mako.rs | 10 +++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs index 31ba401cddb..348bc028ee6 100644 --- a/components/style/keyframes.rs +++ b/components/style/keyframes.rs @@ -393,7 +393,6 @@ impl<'a, 'b> AtRuleParser for KeyframeDeclarationParser<'a, 'b> { } impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> { - /// We parse rules directly into the declarations object type Declaration = ParsedDeclaration; fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result { diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 5eb133601fe..af4943e8d33 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -823,7 +823,7 @@ pub enum ParsedDeclaration { impl ParsedDeclaration { /// Transform this ParsedDeclaration into a sequence of PropertyDeclaration /// by expanding shorthand declarations into their corresponding longhands - pub fn expand(self, mut f: F) where F: FnMut(PropertyDeclaration) { + pub fn expand(self, mut push: F) where F: FnMut(PropertyDeclaration) { match self { % for shorthand in data.shorthands: ParsedDeclaration::${shorthand.camel_case}( @@ -834,7 +834,7 @@ impl ParsedDeclaration { } ) => { % for sub_property in shorthand.sub_properties: - f(PropertyDeclaration::${sub_property.camel_case}( + push(PropertyDeclaration::${sub_property.camel_case}( % if sub_property.boxed: DeclaredValue::Value(Box::new(${sub_property.ident})) % else: @@ -845,7 +845,7 @@ impl ParsedDeclaration { } ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword) => { % for sub_property in shorthand.sub_properties: - f(PropertyDeclaration::${sub_property.camel_case}( + push(PropertyDeclaration::${sub_property.camel_case}( DeclaredValue::CSSWideKeyword(keyword) )); % endfor @@ -856,13 +856,13 @@ impl ParsedDeclaration { Some(ShorthandId::${shorthand.camel_case}) ); % for sub_property in shorthand.sub_properties: - f(PropertyDeclaration::${sub_property.camel_case}( + push(PropertyDeclaration::${sub_property.camel_case}( DeclaredValue::WithVariables(value.clone()) )); % endfor } % endfor - ParsedDeclaration::LonghandOrCustom(declaration) => f(declaration), + ParsedDeclaration::LonghandOrCustom(declaration) => push(declaration), } }