Bug 1369228: Respect `all` shorthand position when parsing property declaration blocks. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 01 Jun 2017 01:18:43 +0200
changeset 587406 bf8cb1e1286493d23daafca841a4516dd6a49da0
parent 587384 106812a7d2643c10c1bbccce47b23fe5b7bf3922
child 587408 0573788632df7d9bbf13f184a547792a33b73028
push id61693
push userbmo:emilio+bugs@crisal.io
push dateThu, 01 Jun 2017 00:01:06 +0000
reviewersheycam
bugs1369228
milestone55.0a1
Bug 1369228: Respect `all` shorthand position when parsing property declaration blocks. r?heycam MozReview-Commit-ID: 457spIBP4df
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
@@ -279,17 +279,17 @@ impl PropertyDeclarationBlock {
             AllShorthand::WithVariables(_) => ShorthandId::All.longhands().len()
         };
         let push_calls_count = drain.declarations.len() + all_shorthand_len;
 
         // With deduplication the actual length increase may be less than this.
         self.declarations.reserve(push_calls_count);
 
         let mut changed = false;
-        for decl in &mut drain.declarations {
+        for decl in (&mut drain.declarations).take(drain.all_shorthand_position) {
             changed |= self.push_common(decl, importance, overwrite_more_important);
         }
         match drain.all_shorthand {
             AllShorthand::NotSet => {}
             AllShorthand::CSSWideKeyword(keyword) => {
                 for &id in ShorthandId::All.longhands() {
                     let decl = PropertyDeclaration::CSSWideKeyword(id, keyword);
                     changed |= self.push_common(decl, importance, overwrite_more_important);
@@ -297,16 +297,19 @@ impl PropertyDeclarationBlock {
             }
             AllShorthand::WithVariables(unparsed) => {
                 for &id in ShorthandId::All.longhands() {
                     let decl = PropertyDeclaration::WithVariables(id, unparsed.clone());
                     changed |= self.push_common(decl, importance, overwrite_more_important);
                 }
             }
         }
+        for decl in drain.declarations {
+            changed |= self.push_common(decl, importance, overwrite_more_important);
+        }
         changed
     }
 
     /// Adds or overrides the declaration for a given property in this block,
     /// **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);
     }
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -208,22 +208,22 @@ pub mod shorthands {
             // definition.
             input.look_for_var_functions();
             let start = input.position();
             while let Ok(_) = input.next() {}  // Look for var()
             if input.seen_var_functions() {
                 input.reset(start);
                 let (first_token_type, css) = try!(
                     ::custom_properties::parse_non_custom_with_var(input));
-                declarations.all_shorthand = AllShorthand::WithVariables(Arc::new(UnparsedValue {
+                declarations.set_all_shorthand(AllShorthand::WithVariables(Arc::new(UnparsedValue {
                     css: css.into_owned(),
                     first_token_type: first_token_type,
                     url_data: context.url_data.clone(),
                     from_shorthand: Some(ShorthandId::All),
-                }));
+                })));
                 Ok(())
             } else {
                 Err(())
             }
         }
     }
 }
 
@@ -1438,17 +1438,17 @@ impl PropertyDeclaration {
                         }
                     % endif
 
                     ${property_pref_check(shorthand)}
 
                     match input.try(|i| CSSWideKeyword::parse(context, i)) {
                         Ok(keyword) => {
                             % if shorthand.name == "all":
-                                declarations.all_shorthand = AllShorthand::CSSWideKeyword(keyword);
+                                declarations.set_all_shorthand(AllShorthand::CSSWideKeyword(keyword));
                             % else:
                                 % for sub_property in shorthand.sub_properties:
                                     declarations.push(PropertyDeclaration::CSSWideKeyword(
                                         LonghandId::${sub_property.camel_case},
                                         keyword,
                                     ));
                                 % endfor
                             % endif
@@ -1475,56 +1475,71 @@ type SourcePropertyDeclarationArray =
 /// A stack-allocated vector of `PropertyDeclaration`
 /// large enough to parse one CSS `key: value` declaration.
 /// (Shorthands expand to multiple `PropertyDeclaration`s.)
 pub struct SourcePropertyDeclaration {
     declarations: ::arrayvec::ArrayVec<SourcePropertyDeclarationArray>,
 
     /// Stored separately to keep MAX_SUB_PROPERTIES_PER_SHORTHAND_EXCEPT_ALL smaller.
     all_shorthand: AllShorthand,
+
+    /// The position all_shorthand would be in `declarations`.
+    all_shorthand_position: usize,
 }
 
 impl SourcePropertyDeclaration {
     /// Create one. It’s big, try not to move it around.
     #[inline]
     pub fn new() -> Self {
         SourcePropertyDeclaration {
             declarations: ::arrayvec::ArrayVec::new(),
             all_shorthand: AllShorthand::NotSet,
+            all_shorthand_position: 0,
         }
     }
 
     /// Similar to Vec::drain: leaves this empty when the return value is dropped.
     pub fn drain(&mut self) -> SourcePropertyDeclarationDrain {
         SourcePropertyDeclarationDrain {
             declarations: self.declarations.drain(..),
             all_shorthand: mem::replace(&mut self.all_shorthand, AllShorthand::NotSet),
+            all_shorthand_position: self.all_shorthand_position,
         }
     }
 
+    fn set_all_shorthand(&mut self, shorthand: AllShorthand) {
+        debug_assert!(!matches!(shorthand, AllShorthand::NotSet));
+
+        let pos = self.declarations.len();
+        self.all_shorthand = shorthand;
+        self.all_shorthand_position = pos;
+    }
+
     /// Reset to initial state
     pub fn clear(&mut self) {
         self.declarations.clear();
         self.all_shorthand = AllShorthand::NotSet;
+        self.all_shorthand_position = 0;
     }
 
     fn is_empty(&self) -> bool {
         self.declarations.is_empty() && matches!(self.all_shorthand, AllShorthand::NotSet)
     }
 
     fn push(&mut self, declaration: PropertyDeclaration) {
         let over_capacity = self.declarations.push(declaration).is_some();
         debug_assert!(!over_capacity);
     }
 }
 
 /// Return type of SourcePropertyDeclaration::drain
 pub struct SourcePropertyDeclarationDrain<'a> {
     declarations: ::arrayvec::Drain<'a, SourcePropertyDeclarationArray>,
     all_shorthand: AllShorthand,
+    all_shorthand_position: usize,
 }
 
 enum AllShorthand {
     NotSet,
     CSSWideKeyword(CSSWideKeyword),
     WithVariables(Arc<UnparsedValue>)
 }