Bug 1364286 - Fix serialization of system fonts; r?xidorn draft
authorManish Goregaokar <manishearth@gmail.com>
Fri, 12 May 2017 17:36:05 -0700
changeset 587516 ecaff6ffe8477fe933d25149c713058147d90181
parent 585650 35099b4caec14bf0e3c5e3fed7a17dd3faf51dbe
child 631301 42296967317da7a890f6ae079293d7960bd8c39b
push id61741
push userbmo:manishearth@gmail.com
push dateThu, 01 Jun 2017 07:30:16 +0000
reviewersxidorn
bugs1364286
milestone55.0a1
Bug 1364286 - Fix serialization of system fonts; r?xidorn MozReview-Commit-ID: LnJbj00hT2T
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/properties/shorthand/font.mako.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -494,34 +494,48 @@ impl ToCss for PropertyDeclarationBlock 
 
                 // Step 3.3.2
                 for &shorthand in shorthands {
                     let properties = shorthand.longhands();
 
                     // Substep 2 & 3
                     let mut current_longhands = Vec::new();
                     let mut important_count = 0;
+                    let mut found_system = None;
 
-                    for &&(ref longhand, longhand_importance) in longhands.iter() {
-                        if longhand.id().is_longhand_of(shorthand) {
-                            current_longhands.push(longhand);
-                            if longhand_importance.important() {
-                                important_count += 1;
+                    if shorthand == ShorthandId::Font && longhands.iter().any(|&&(ref l, _)| l.get_system().is_some()) {
+                        for &&(ref longhand, longhand_importance) in longhands.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 longhand_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;
+                    } else {
+                        for &&(ref longhand, longhand_importance) in longhands.iter() {
+                            if longhand.id().is_longhand_of(shorthand) {
+                                current_longhands.push(longhand);
+                                if longhand_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() {
                         continue;
                     }
                     let importance = if is_important {
@@ -535,35 +549,42 @@ impl ToCss for PropertyDeclarationBlock 
                     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 value = String::new();
-                    let value = match appendable_value {
-                        AppendableValue::Css { css, with_variables } => {
+                    let mut v = String::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,
                             }
                         }
-                        other @ _ => {
-                            append_declaration_value(&mut value, other)?;
+                        (_, Some(sys)) => {
+                            sys.to_css(&mut v)?;
+                            AppendableValue::Css {
+                                css: &v,
+                                with_variables: false,
+                            }
+                        }
+                        (other, _) => {
+                            append_declaration_value(&mut v, other)?;
 
                             // Substep 6
-                            if value.is_empty() {
+                            if v.is_empty() {
                                 continue;
                             }
 
                             AppendableValue::Css {
-                                css: &value,
+                                css: &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.
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -27,16 +27,17 @@ use context::QuirksMode;
 use font_metrics::FontMetricsProvider;
 #[cfg(feature = "gecko")] use gecko_bindings::bindings;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::{self, nsCSSPropertyID};
 #[cfg(feature = "servo")] use logical_geometry::{LogicalMargin, PhysicalSide};
 use logical_geometry::WritingMode;
 use media_queries::Device;
 use parser::{PARSING_MODE_DEFAULT, Parse, ParserContext};
 use properties::animated_properties::TransitionProperty;
+#[cfg(feature = "gecko")] use properties::longhands::system_font::SystemFont;
 #[cfg(feature = "servo")] use servo_config::prefs::PREFS;
 use shared_lock::StylesheetGuards;
 use style_traits::{HasViewportPercentage, ToCss};
 use stylesheets::{CssRuleType, Origin, UrlExtraData};
 #[cfg(feature = "servo")] use values::Either;
 use values::computed;
 use cascade_info::CascadeInfo;
 use rule_tree::StrongRuleNode;
@@ -47,17 +48,17 @@ pub use self::declaration_block::*;
 
 #[cfg(feature = "gecko")]
 #[macro_export]
 macro_rules! property_name {
     ($s: tt) => { atom!($s) }
 }
 
 <%!
-    from data import Method, Keyword, to_rust_ident, to_camel_case
+    from data import Method, Keyword, to_rust_ident, to_camel_case, SYSTEM_FONT_LONGHANDS
     import os.path
 %>
 
 #[path="${repr(os.path.join(os.path.dirname(__file__), 'declaration_block.rs'))[1:-1]}"]
 pub mod declaration_block;
 
 /// Conversion with fewer impls than From/Into
 pub trait MaybeBoxed<Out> {
@@ -1276,16 +1277,45 @@ impl PropertyDeclaration {
     /// Returns a CSS-wide keyword if the declaration's value is one.
     pub fn get_css_wide_keyword(&self) -> Option<CSSWideKeyword> {
         match *self {
             PropertyDeclaration::CSSWideKeyword(_, keyword) => Some(keyword),
             _ => None,
         }
     }
 
+    /// Returns whether or not the property is set by a system font
+    #[cfg(feature = "gecko")]
+    pub fn get_system(&self) -> Option<SystemFont> {
+        match *self {
+            % for prop in SYSTEM_FONT_LONGHANDS:
+                PropertyDeclaration::${to_camel_case(prop)}(ref prop) => {
+                    prop.get_system()
+                }
+            % endfor
+            _ => None,
+        }
+    }
+
+    /// Is it the default value of line-height?
+    ///
+    /// (using match because it generates less code than)
+    pub fn is_default_line_height(&self) -> bool {
+        match *self {
+            PropertyDeclaration::LineHeight(longhands::line_height::SpecifiedValue::Normal) => true,
+            _ => false
+        }
+    }
+
+    #[cfg(feature = "servo")]
+    /// Dummy method to avoid cfg()s
+    pub fn get_system(&self, include_line_height_default: bool) -> Option<()> {
+        None
+    }
+
     /// Returns whether the declaration may be serialized as part of a shorthand.
     ///
     /// This method returns false if this declaration contains variable or has a
     /// CSS-wide keyword value, since these values cannot be serialized as part
     /// of a shorthand.
     ///
     /// Caller should check `with_variables_from_shorthand()` and whether all
     /// needed declarations has the same CSS-wide keyword first.
--- a/servo/components/style/properties/shorthand/font.mako.rs
+++ b/servo/components/style/properties/shorthand/font.mako.rs
@@ -17,16 +17,17 @@
                                     ${'font-variant-position' if product == 'gecko' or data.testing else ''}
                                     ${'font-language-override' if product == 'gecko' or data.testing else ''}
                                     ${'font-feature-settings' if product == 'gecko' or data.testing else ''}"
                     spec="https://drafts.csswg.org/css-fonts-3/#propdef-font">
     use properties::longhands::{font_family, font_style, font_weight, font_stretch};
     use properties::longhands::{font_size, line_height, font_variant_caps};
     #[cfg(feature = "gecko")]
     use properties::longhands::system_font::SystemFont;
+
     <%
         gecko_sub_properties = "kerning language_override size_adjust \
                                 variant_alternates variant_east_asian \
                                 variant_ligatures variant_numeric \
                                 variant_position feature_settings".split()
     %>
     % if product == "gecko" or data.testing:
         % for prop in gecko_sub_properties: