servo: Merge #16135 - Shorthand with variable reference should not have extra whitespace after colon for serialization (from emilio:shorthand-reference-variable); r=upsuper
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 29 Mar 2017 06:11:32 -0500
changeset 350407 d5e22d9308739ad39e78908a09c68cf9812ad1c1
parent 350406 e8540acb58bc1a75665b2f6ac1d1f5d2d93a28a1
child 350408 5f8238257a99b380ef8dca08728aac0b5e834cdc
push id88627
push userkwierso@gmail.com
push dateWed, 29 Mar 2017 22:48:01 +0000
treeherdermozilla-inbound@83d25c5fdde7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersupsuper
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #16135 - Shorthand with variable reference should not have extra whitespace after colon for serialization (from emilio:shorthand-reference-variable); r=upsuper This is https://github.com/servo/servo/pull/15422 + a manifest update. Fixes #15295. Source-Repo: https://github.com/servo/servo Source-Revision: 3a3f258f33d7f1c18645df95d3ca72879a770266
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/properties.mako.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -328,17 +328,19 @@ impl PropertyDeclarationBlock {
                     -> &PropertyDeclaration {
                     &dec.0
                 }
                 if !self.declarations.iter().all(|decl| decl.0.shorthands().contains(&shorthand)) {
                     return Err(fmt::Error)
                 }
                 let iter = self.declarations.iter().map(get_declaration as fn(_) -> _);
                 match shorthand.get_shorthand_appendable_value(iter) {
-                    Some(AppendableValue::Css(css)) => dest.write_str(css),
+                    Some(AppendableValue::Css { css, .. }) => {
+                        dest.write_str(css)
+                    },
                     Some(AppendableValue::DeclarationsForShorthand(_, decls)) => {
                         shorthand.longhands_to_css(decls, dest)
                     }
                     _ => Ok(())
                 }
             }
         }
     }
@@ -404,86 +406,110 @@ impl ToCss for PropertyDeclarationBlock 
                         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.is_empty() || current_longhands.len() != properties.len() {
+                    // 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 {
                         Importance::Important
                     } else {
                         Importance::Normal
                     };
 
-                    // Substep 5 - Let value be the result of invoking serialize a CSS
-                    // value of current longhands.
+                    // 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 value = String::new();
-                    match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) {
-                        None => continue,
-                        Some(appendable_value) => {
-                            try!(append_declaration_value(&mut value, appendable_value));
+                    let value = match appendable_value {
+                        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)?;
 
-                    // Substep 6
-                    if value.is_empty() {
-                        continue
-                    }
+                            // Substep 6
+                            if value.is_empty() {
+                                continue;
+                            }
+
+                            AppendableValue::Css {
+                                css: &value,
+                                with_variables: false,
+                            }
+                        }
+                    };
 
                     // Substeps 7 and 8
-                    try!(append_serialization::<W, Cloned<slice::Iter< _>>, _>(
-                             dest, &shorthand, AppendableValue::Css(&value), importance,
-                             &mut is_first_serialization));
+                    append_serialization::<_, Cloned<slice::Iter< _>>, _>(
+                         dest,
+                         &shorthand,
+                         value,
+                         importance,
+                         &mut is_first_serialization)?;
 
-                    for current_longhand in current_longhands {
+                    for current_longhand in &current_longhands {
                         // Substep 9
                         already_serialized.push(current_longhand.id());
-                        let index_to_remove = longhands.iter().position(|l| l.0 == *current_longhand);
+                        let index_to_remove = longhands.iter().position(|l| l.0 == **current_longhand);
                         if let Some(index) = index_to_remove {
                             // Substep 10
                             longhands.remove(index);
                         }
-                     }
-                 }
+                    }
+                }
             }
 
             // Step 3.3.4
             if already_serialized.contains(&property) {
                 continue;
             }
 
             use std::iter::Cloned;
             use std::slice;
 
             // Steps 3.3.5, 3.3.6 & 3.3.7
             // Need to specify an iterator type here even though it’s unused to work around
             // "error: unable to infer enough type information about `_`;
             //  type annotations or generic parameter binding required [E0282]"
             // Use the same type as earlier call to reuse generated code.
-            try!(append_serialization::<W, Cloned<slice::Iter< &PropertyDeclaration>>, _>(
+            append_serialization::<_, Cloned<slice::Iter<_>>, _>(
                 dest,
                 &property,
                 AppendableValue::Declaration(declaration),
                 importance,
-                &mut is_first_serialization));
+                &mut is_first_serialization)?;
 
             // Step 3.3.8
             already_serialized.push(property);
         }
 
         // Step 4
         Ok(())
     }
@@ -498,54 +524,56 @@ pub enum AppendableValue<'a, I>
     Declaration(&'a PropertyDeclaration),
     /// A set of declarations for a given shorthand.
     ///
     /// FIXME: This needs more docs, where are the shorthands expanded? We print
     /// the property name before-hand, don't we?
     DeclarationsForShorthand(ShorthandId, I),
     /// A raw CSS string, coming for example from a property with CSS variables,
     /// or when storing a serialized shorthand value before appending directly.
-    Css(&'a str)
+    Css {
+        /// The raw CSS string.
+        css: &'a str,
+        /// Whether the original serialization contained variables or not.
+        with_variables: bool,
+    }
 }
 
 /// Potentially appends whitespace after the first (property: value;) pair.
 fn handle_first_serialization<W>(dest: &mut W,
                                  is_first_serialization: &mut bool)
                                  -> fmt::Result
     where W: fmt::Write,
 {
     if !*is_first_serialization {
-        try!(write!(dest, " "));
+        dest.write_str(" ")
     } else {
         *is_first_serialization = false;
+        Ok(())
     }
-
-    Ok(())
 }
 
 /// Append a given kind of appendable value to a serialization.
 pub fn append_declaration_value<'a, W, I>(dest: &mut W,
                                           appendable_value: AppendableValue<'a, I>)
                                           -> fmt::Result
     where W: fmt::Write,
           I: Iterator<Item=&'a PropertyDeclaration>,
 {
     match appendable_value {
-        AppendableValue::Css(css) => {
-            try!(write!(dest, "{}", css))
+        AppendableValue::Css { css, .. } => {
+            dest.write_str(css)
         },
         AppendableValue::Declaration(decl) => {
-            try!(decl.to_css(dest));
+            decl.to_css(dest)
         },
         AppendableValue::DeclarationsForShorthand(shorthand, decls) => {
-            try!(shorthand.longhands_to_css(decls, dest));
+            shorthand.longhands_to_css(decls, dest)
         }
     }
-
-    Ok(())
 }
 
 /// Append a given property and value pair to a serialization.
 pub fn append_serialization<'a, W, I, N>(dest: &mut W,
                                          property_name: &N,
                                          appendable_value: AppendableValue<'a, I>,
                                          importance: Importance,
                                          is_first_serialization: &mut bool)
@@ -555,38 +583,40 @@ pub fn append_serialization<'a, W, I, N>
           N: ToCss,
 {
     try!(handle_first_serialization(dest, is_first_serialization));
 
     try!(property_name.to_css(dest));
     try!(dest.write_char(':'));
 
     // for normal parsed values, add a space between key: and value
-    match &appendable_value {
-        &AppendableValue::Css(_) => {
-            try!(write!(dest, " "))
-        },
-        &AppendableValue::Declaration(decl) => {
+    match appendable_value {
+        AppendableValue::Declaration(decl) => {
             if !decl.value_is_unparsed() {
-                // for normal parsed values, add a space between key: and value
-                try!(write!(dest, " "));
+                // For normal parsed values, add a space between key: and value.
+                dest.write_str(" ")?
             }
         },
+        AppendableValue::Css { with_variables, .. } => {
+            if !with_variables {
+                dest.write_str(" ")?
+            }
+        }
         // Currently append_serialization is only called with a Css or
         // a Declaration AppendableValue.
-        &AppendableValue::DeclarationsForShorthand(..) => unreachable!()
+        AppendableValue::DeclarationsForShorthand(..) => unreachable!(),
     }
 
     try!(append_declaration_value(dest, appendable_value));
 
     if importance.important() {
-        try!(write!(dest, " !important"));
+        try!(dest.write_str(" !important"));
     }
 
-    write!(dest, ";")
+    dest.write_char(';')
 }
 
 /// A helper to parse the style attribute of an element, in order for this to be
 /// shared between Servo and Gecko.
 pub fn parse_style_attribute(input: &str,
                              base_url: &ServoUrl,
                              error_reporter: &ParseErrorReporter,
                              extra_data: ParserContextExtraData)
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -556,18 +556,18 @@ impl ShorthandId {
             % endfor
         }
     }
 
     /// Finds and returns an appendable value for the given declarations.
     ///
     /// Returns the optional appendable value.
     pub fn get_shorthand_appendable_value<'a, I>(self,
-                                             declarations: I)
-                                             -> Option<AppendableValue<'a, I::IntoIter>>
+                                                 declarations: I)
+                                                 -> Option<AppendableValue<'a, I::IntoIter>>
         where I: IntoIterator<Item=&'a PropertyDeclaration>,
               I::IntoIter: Clone,
     {
         let declarations = declarations.into_iter();
 
         // Only cloning iterators (a few pointers each) not declarations.
         let mut declarations2 = declarations.clone();
         let mut declarations3 = declarations.clone();
@@ -575,25 +575,31 @@ impl ShorthandId {
         let first_declaration = match declarations2.next() {
             Some(declaration) => declaration,
             None => return None
         };
 
         // https://drafts.csswg.org/css-variables/#variables-in-shorthands
         if let Some(css) = first_declaration.with_variables_from_shorthand(self) {
             if declarations2.all(|d| d.with_variables_from_shorthand(self) == Some(css)) {
-               return Some(AppendableValue::Css(css));
-           }
-           return None;
+               return Some(AppendableValue::Css {
+                   css: css,
+                   with_variables: true,
+               });
+            }
+            return None;
         }
 
         // Check whether they are all the same CSS-wide keyword.
         if let Some(keyword) = first_declaration.get_css_wide_keyword() {
             if declarations2.all(|d| d.get_css_wide_keyword() == Some(keyword)) {
-                return Some(AppendableValue::Css(keyword.to_str()));
+                return Some(AppendableValue::Css {
+                    css: keyword.to_str(),
+                    with_variables: false,
+                });
             }
             return None;
         }
 
         // Check whether all declarations can be serialized as part of shorthand.
         if declarations3.all(|d| d.may_serialize_as_part_of_shorthand()) {
             return Some(AppendableValue::DeclarationsForShorthand(self, declarations));
         }
@@ -1186,20 +1192,20 @@ impl PropertyDeclaration {
     /// Caller should check `with_variables_from_shorthand()` and whether all
     /// needed declarations has the same CSS-wide keyword first.
     ///
     /// Note that, serialization of a shorthand may still fail because of other
     /// property-specific requirement even when this method returns true for all
     /// the longhand declarations.
     pub fn may_serialize_as_part_of_shorthand(&self) -> bool {
         match *self {
-            PropertyDeclaration::CSSWideKeyword(..) => false,
+            PropertyDeclaration::CSSWideKeyword(..) |
             PropertyDeclaration::WithVariables(..) => false,
             PropertyDeclaration::Custom(..) =>
-                unreachable!("Serialize a custom property as part of shorthand?"),
+                unreachable!("Serializing a custom property as part of shorthand?"),
             _ => true,
         }
     }
 
     /// Return whether the value is stored as it was in the CSS source,
     /// preserving whitespace (as opposed to being parsed into a more abstract
     /// data structure).
     ///