From ba7629a17eb501c33234e3cced781230a2a5b542 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Feb 2018 20:02:54 +0100 Subject: [PATCH 1/5] style: Update a spec link. --- components/style/properties/declaration_block.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 8612c1244c3..c0eab37fe7a 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -306,7 +306,7 @@ impl PropertyDeclarationBlock { /// Find the value of the given property in this block and serialize it /// - /// + /// pub fn property_value_to_css(&self, property: &PropertyId, dest: &mut CssStringWriter) -> fmt::Result { // Step 1.1: done when parsing a string to PropertyId @@ -617,8 +617,7 @@ impl PropertyDeclarationBlock { dest: &mut CssStringWriter, computed_values: Option<&ComputedValues>, custom_properties_block: Option<&PropertyDeclarationBlock>, - ) -> fmt::Result - { + ) -> fmt::Result { match property.as_shorthand() { Err(_longhand_or_custom) => { if self.declarations.len() == 1 { From 5c2ac8cf8bdbea247fb1a1507c87b1bf3ef95103 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Feb 2018 20:06:23 +0100 Subject: [PATCH 2/5] style: Deindent a giant loop guarded by an !is_empty() condition. --- .../style/properties/declaration_block.rs | 249 +++++++++--------- 1 file changed, 123 insertions(+), 126 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index c0eab37fe7a..845eaa8e503 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -764,143 +764,140 @@ impl PropertyDeclarationBlock { } // Step 3.3 - let shorthands = declaration.shorthands(); - if !shorthands.is_empty() { - // Step 3.3.1 is done by checking already_serialized while - // iterating below. + // Step 3.3.1 is done by checking already_serialized while + // iterating below. - // Step 3.3.2 - for &shorthand in shorthands { - let properties = shorthand.longhands(); + // Step 3.3.2 + for &shorthand in declaration.shorthands() { + let properties = shorthand.longhands(); - // Substep 2 & 3 - let mut current_longhands = SmallVec::<[_; 10]>::new(); - let mut important_count = 0; - let mut found_system = None; + // Substep 2 & 3 + let mut current_longhands = SmallVec::<[_; 10]>::new(); + let mut important_count = 0; + let mut found_system = None; - let is_system_font = - shorthand == ShorthandId::Font && - self.declarations.iter().any(|l| { - !already_serialized.contains(l.id()) && - l.get_system().is_some() - }); + let is_system_font = + shorthand == ShorthandId::Font && + self.declarations.iter().any(|l| { + !already_serialized.contains(l.id()) && + l.get_system().is_some() + }); - if is_system_font { - for (longhand, importance) in self.declaration_importance_iter() { - if longhand.get_system().is_some() || longhand.is_default_line_height() { - current_longhands.push(longhand); - if found_system.is_none() { - found_system = longhand.get_system(); - } - if importance.important() { - important_count += 1; - } + if is_system_font { + for (longhand, importance) in self.declaration_importance_iter() { + if longhand.get_system().is_some() || longhand.is_default_line_height() { + current_longhands.push(longhand); + if found_system.is_none() { + found_system = longhand.get_system(); } - } - } else { - for (longhand, importance) in self.declaration_importance_iter() { - if longhand.id().is_longhand_of(shorthand) { - current_longhands.push(longhand); - if importance.important() { - important_count += 1; - } + if importance.important() { + important_count += 1; } } - // Substep 1: - // - // Assuming that the PropertyDeclarationBlock contains no - // duplicate entries, if the current_longhands length is - // equal to the properties length, it means that the - // properties that map to shorthand are present in longhands - if current_longhands.len() != properties.len() { - continue; - } } - - // Substep 4 - let is_important = important_count > 0; - if is_important && important_count != current_longhands.len() { + } else { + for (longhand, importance) in self.declaration_importance_iter() { + if longhand.id().is_longhand_of(shorthand) { + current_longhands.push(longhand); + if importance.important() { + important_count += 1; + } + } + } + // Substep 1: + // + // Assuming that the PropertyDeclarationBlock contains no + // duplicate entries, if the current_longhands length is + // equal to the properties length, it means that the + // properties that map to shorthand are present in longhands + if current_longhands.len() != properties.len() { continue; } - let importance = if is_important { - Importance::Important - } else { - Importance::Normal - }; - - // Substep 5 - Let value be the result of invoking serialize - // a CSS value of current longhands. - let appendable_value = - match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { - None => continue, - Some(appendable_value) => appendable_value, - }; - - // We avoid re-serializing if we're already an - // AppendableValue::Css. - let mut v = CssString::new(); - let value = match (appendable_value, found_system) { - (AppendableValue::Css { css, with_variables }, _) => { - debug_assert!(!css.is_empty()); - AppendableValue::Css { - css: css, - with_variables: with_variables, - } - } - #[cfg(feature = "gecko")] - (_, Some(sys)) => { - sys.to_css(&mut CssWriter::new(&mut v))?; - AppendableValue::Css { - css: CssStringBorrow::from(&v), - with_variables: false, - } - } - (other, _) => { - append_declaration_value(&mut v, other)?; - - // Substep 6 - if v.is_empty() { - continue; - } - - AppendableValue::Css { - css: CssStringBorrow::from(&v), - with_variables: false, - } - } - }; - - // Substeps 7 and 8 - // We need to check the shorthand whether it's an alias property or not. - // If it's an alias property, it should be serialized like its longhand. - if shorthand.flags().contains(PropertyFlags::SHORTHAND_ALIAS_PROPERTY) { - append_serialization::>, _>( - dest, - &property, - value, - importance, - &mut is_first_serialization)?; - } else { - append_serialization::>, _>( - dest, - &shorthand, - value, - importance, - &mut is_first_serialization)?; - } - - for current_longhand in ¤t_longhands { - // Substep 9 - already_serialized.insert(current_longhand.id()); - } - - // FIXME(https://github.com/w3c/csswg-drafts/issues/1774) - // The specification does not include an instruction to abort - // the shorthand loop at this point, but doing so both matches - // Gecko and makes sense since shorthands are checked in - // preferred order. - break; } + + // Substep 4 + let is_important = important_count > 0; + if is_important && important_count != current_longhands.len() { + continue; + } + let importance = if is_important { + Importance::Important + } else { + Importance::Normal + }; + + // Substep 5 - Let value be the result of invoking serialize + // a CSS value of current longhands. + let appendable_value = + match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { + None => continue, + Some(appendable_value) => appendable_value, + }; + + // We avoid re-serializing if we're already an + // AppendableValue::Css. + let mut v = CssString::new(); + let value = match (appendable_value, found_system) { + (AppendableValue::Css { css, with_variables }, _) => { + debug_assert!(!css.is_empty()); + AppendableValue::Css { + css: css, + with_variables: with_variables, + } + } + #[cfg(feature = "gecko")] + (_, Some(sys)) => { + sys.to_css(&mut CssWriter::new(&mut v))?; + AppendableValue::Css { + css: CssStringBorrow::from(&v), + with_variables: false, + } + } + (other, _) => { + append_declaration_value(&mut v, other)?; + + // Substep 6 + if v.is_empty() { + continue; + } + + AppendableValue::Css { + css: CssStringBorrow::from(&v), + with_variables: false, + } + } + }; + + // Substeps 7 and 8 + // We need to check the shorthand whether it's an alias property or not. + // If it's an alias property, it should be serialized like its longhand. + if shorthand.flags().contains(PropertyFlags::SHORTHAND_ALIAS_PROPERTY) { + append_serialization::>, _>( + dest, + &property, + value, + importance, + &mut is_first_serialization)?; + } else { + append_serialization::>, _>( + dest, + &shorthand, + value, + importance, + &mut is_first_serialization)?; + } + + for current_longhand in ¤t_longhands { + // Substep 9 + already_serialized.insert(current_longhand.id()); + } + + // FIXME(https://github.com/w3c/csswg-drafts/issues/1774) + // The specification does not include an instruction to abort + // the shorthand loop at this point, but doing so both matches + // Gecko and makes sense since shorthands are checked in + // preferred order. + break; } // Step 3.3.4 From f3e38aca1bcf671993c4d02cfc8fb3a2a615958a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Feb 2018 20:17:34 +0100 Subject: [PATCH 3/5] style: Move the shorthand serialization code to its own function. --- .../style/properties/declaration_block.rs | 100 ++++++++++-------- 1 file changed, 54 insertions(+), 46 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 845eaa8e503..3a5f7e0a325 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -304,6 +304,50 @@ impl PropertyDeclarationBlock { }) } + fn shorthand_to_css( + &self, + shorthand: ShorthandId, + dest: &mut CssStringWriter, + ) -> fmt::Result { + // Step 1.2.1 of + // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue + let mut list = SmallVec::<[&_; 10]>::new(); + let mut important_count = 0; + + // Step 1.2.2 + for &longhand in shorthand.longhands() { + // Step 1.2.2.1 + let declaration = self.get(PropertyDeclarationId::Longhand(longhand)); + + // Step 1.2.2.2 & 1.2.2.3 + match declaration { + Some((declaration, importance)) => { + list.push(declaration); + if importance.important() { + important_count += 1; + } + }, + None => return Ok(()), + } + } + + // If there is one or more longhand with important, and one or more + // without important, we don't serialize it as a shorthand. + if important_count > 0 && important_count != list.len() { + return Ok(()); + } + + // Step 1.2.3 + // We don't print !important when serializing individual properties, + // so we treat this as a normal-importance property + match shorthand.get_shorthand_appendable_value(list.iter().cloned()) { + Some(appendable_value) => { + append_declaration_value(dest, appendable_value) + } + None => return Ok(()), + } + } + /// Find the value of the given property in this block and serialize it /// /// @@ -311,53 +355,17 @@ impl PropertyDeclarationBlock { // Step 1.1: done when parsing a string to PropertyId // Step 1.2 - match property.as_shorthand() { - Ok(shorthand) => { - // Step 1.2.1 - let mut list = Vec::new(); - let mut important_count = 0; + let longhand_or_custom = match property.as_shorthand() { + Ok(shorthand) => return self.shorthand_to_css(shorthand, dest), + Err(longhand_or_custom) => longhand_or_custom, + }; - // Step 1.2.2 - for &longhand in shorthand.longhands() { - // Step 1.2.2.1 - let declaration = self.get(PropertyDeclarationId::Longhand(longhand)); - - // Step 1.2.2.2 & 1.2.2.3 - match declaration { - Some((declaration, importance)) => { - list.push(declaration); - if importance.important() { - important_count += 1; - } - }, - None => return Ok(()), - } - } - - // If there is one or more longhand with important, and one or more - // without important, we don't serialize it as a shorthand. - if important_count > 0 && important_count != list.len() { - return Ok(()); - } - - // Step 1.2.3 - // We don't print !important when serializing individual properties, - // so we treat this as a normal-importance property - match shorthand.get_shorthand_appendable_value(list) { - Some(appendable_value) => - append_declaration_value(dest, appendable_value), - None => return Ok(()), - } - } - Err(longhand_or_custom) => { - if let Some((value, _importance)) = self.get(longhand_or_custom) { - // Step 2 - value.to_css(dest) - } else { - // Step 3 - Ok(()) - } - } + if let Some((value, _importance)) = self.get(longhand_or_custom) { + // Step 2 + value.to_css(dest) + } else { + // Step 3 + Ok(()) } } From 808102035e26a9613311a97f61cf6ecd11a94488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Feb 2018 20:23:26 +0100 Subject: [PATCH 4/5] style: Deindent and reuse some more code in single_value_to_css. This method is a hack on its own, let's try to scope it a bit... --- .../style/properties/declaration_block.rs | 95 ++++++++----------- 1 file changed, 42 insertions(+), 53 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 3a5f7e0a325..983b2b0a331 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -626,61 +626,50 @@ impl PropertyDeclarationBlock { computed_values: Option<&ComputedValues>, custom_properties_block: Option<&PropertyDeclarationBlock>, ) -> fmt::Result { - match property.as_shorthand() { - Err(_longhand_or_custom) => { - if self.declarations.len() == 1 { - let declaration = &self.declarations[0]; - let custom_properties = if let Some(cv) = computed_values { - // If there are extra custom properties for this - // declaration block, factor them in too. - if let Some(block) = custom_properties_block { - // FIXME(emilio): This is not super-efficient - // here... - block.cascade_custom_properties(cv.custom_properties()) - } else { - cv.custom_properties().cloned() - } - } else { - None - }; + if let Ok(shorthand) = property.as_shorthand() { + return self.shorthand_to_css(shorthand, dest); + } - match (declaration, computed_values) { - // If we have a longhand declaration with variables, those variables will be - // stored as unparsed values. As a temporary measure to produce sensible results - // in Gecko's getKeyframes() implementation for CSS animations, if - // |computed_values| is supplied, we use it to expand such variable - // declarations. This will be fixed properly in Gecko bug 1391537. - ( - &PropertyDeclaration::WithVariables(ref declaration), - Some(ref _computed_values), - ) => { - declaration.value.substitute_variables( - declaration.id, - custom_properties.as_ref(), - QuirksMode::NoQuirks, - ).to_css(dest) - }, - (ref d, _) => d.to_css(dest), - } - } else { - Err(fmt::Error) - } - } - Ok(shorthand) => { - if !self.declarations.iter().all(|decl| decl.shorthands().contains(&shorthand)) { - return Err(fmt::Error) - } - let iter = self.declarations.iter(); - match shorthand.get_shorthand_appendable_value(iter) { - Some(AppendableValue::Css { css, .. }) => { - css.append_to(dest) - }, - Some(AppendableValue::DeclarationsForShorthand(_, decls)) => { - shorthand.longhands_to_css(decls, &mut CssWriter::new(dest)) - } - _ => Ok(()) - } + // FIXME(emilio): Should this assert, or assert that the declaration is + // the property we expect? + let declaration = match self.declarations.get(0) { + Some(d) => d, + None => return Err(fmt::Error), + }; + + let custom_properties = if let Some(cv) = computed_values { + // If there are extra custom properties for this declaration block, + // factor them in too. + if let Some(block) = custom_properties_block { + // FIXME(emilio): This is not super-efficient here, and all this + // feels like a hack anyway... + block.cascade_custom_properties(cv.custom_properties()) + } else { + cv.custom_properties().cloned() } + } else { + None + }; + + match (declaration, computed_values) { + // If we have a longhand declaration with variables, those variables + // will be stored as unparsed values. + // + // As a temporary measure to produce sensible results in Gecko's + // getKeyframes() implementation for CSS animations, if + // |computed_values| is supplied, we use it to expand such variable + // declarations. This will be fixed properly in Gecko bug 1391537. + ( + &PropertyDeclaration::WithVariables(ref declaration), + Some(ref _computed_values), + ) => { + declaration.value.substitute_variables( + declaration.id, + custom_properties.as_ref(), + QuirksMode::NoQuirks, + ).to_css(dest) + }, + (ref d, _) => d.to_css(dest), } } From 502b734a1a4304b7265643e3dc9a7ceb30e25986 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Feb 2018 20:29:40 +0100 Subject: [PATCH 5/5] style: Update a couple other spec links. --- components/style/properties/declaration_block.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 983b2b0a331..2be09150edc 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -369,7 +369,7 @@ impl PropertyDeclarationBlock { } } - /// + /// pub fn property_priority(&self, property: &PropertyId) -> Importance { // Step 1: done when parsing a string to PropertyId @@ -583,7 +583,7 @@ impl PropertyDeclarationBlock { updated_at_least_one } - /// + /// /// /// Returns whether any declaration was actually removed. pub fn remove_property(&mut self, property: &PropertyId) -> bool {