Bug 1369198: Ensure pushing a declaration to a declaration block for parsing always inserts it at the last position. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 01 Jun 2017 02:24:58 +0200
changeset 587586 3cedbc97b616b02895296008b6b20728c789cda8
parent 587384 106812a7d2643c10c1bbccce47b23fe5b7bf3922
child 631322 9263895df8f982c5fcad48c7fcc139d408f1e08b
push id61763
push userbmo:emilio+bugs@crisal.io
push dateThu, 01 Jun 2017 11:28:21 +0000
reviewersheycam
bugs1369198
milestone55.0a1
Bug 1369198: Ensure pushing a declaration to a declaration block for parsing always inserts it at the last position. r?heycam MozReview-Commit-ID: LM9mK6ngZ4e
servo/components/style/properties/declaration_block.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -252,100 +252,145 @@ impl PropertyDeclarationBlock {
             Err(longhand_or_custom) => {
                 // Step 3
                 self.get(longhand_or_custom).map_or(Importance::Normal, |&(_, importance)| importance)
             }
         }
     }
 
     /// Adds or overrides the declaration for a given property in this block,
-    /// **except** if an existing declaration for the same property is more important.
+    /// **except** if an existing declaration for the same property is more
+    /// important.
+    ///
+    /// Always ensures that the property declaration is at the end.
     pub fn extend(&mut self, drain: SourcePropertyDeclarationDrain, importance: Importance) {
         self.extend_common(drain, importance, false);
     }
 
     /// Adds or overrides the declaration for a given property in this block,
-    /// **even** if an existing declaration for the same property is more important.
+    /// **even** if an existing declaration for the same property is more
+    /// important, and reuses the same position in the block.
     ///
-    /// Return whether anything changed.
+    /// Returns whether anything changed.
     pub fn extend_reset(&mut self, drain: SourcePropertyDeclarationDrain,
                         importance: Importance) -> bool {
         self.extend_common(drain, importance, true)
     }
 
-    fn extend_common(&mut self, mut drain: SourcePropertyDeclarationDrain,
-                     importance: Importance, overwrite_more_important: bool) -> bool {
+    fn extend_common(
+        &mut self,
+        mut drain: SourcePropertyDeclarationDrain,
+        importance: Importance,
+        overwrite_more_important_and_reuse_slot: bool,
+    ) -> bool {
         let all_shorthand_len = match drain.all_shorthand {
             AllShorthand::NotSet => 0,
             AllShorthand::CSSWideKeyword(_) |
             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 {
-            changed |= self.push_common(decl, importance, overwrite_more_important);
+            changed |= self.push_common(
+                decl,
+                importance,
+                overwrite_more_important_and_reuse_slot,
+            );
         }
         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);
+                    changed |= self.push_common(
+                        decl,
+                        importance,
+                        overwrite_more_important_and_reuse_slot,
+                    );
                 }
             }
             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);
+                    changed |= self.push_common(
+                        decl,
+                        importance,
+                        overwrite_more_important_and_reuse_slot,
+                    );
                 }
             }
         }
         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.
+    /// **except** if an existing declaration for the same property is more
+    /// important.
+    ///
+    /// Ensures that, if inserted, it's inserted at the end of the declaration
+    /// block.
     pub fn push(&mut self, declaration: PropertyDeclaration, importance: Importance) {
         self.push_common(declaration, importance, false);
     }
 
-    fn push_common(&mut self, declaration: PropertyDeclaration, importance: Importance,
-                   overwrite_more_important: bool) -> bool {
+    fn push_common(
+        &mut self,
+        declaration: PropertyDeclaration,
+        importance: Importance,
+        overwrite_more_important_and_reuse_slot: bool
+    ) -> bool {
         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 {
+            let mut index_to_remove = None;
+            for (i, slot) in self.declarations.iter_mut().enumerate() {
                 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 {
+                            if overwrite_more_important_and_reuse_slot {
                                 self.important_count -= 1;
                             } else {
                                 return false
                             }
                         }
                         _ => if slot.0 == declaration {
                             return false;
                         }
                     }
-                    *slot = (declaration, importance);
-                    return true
+
+                    if overwrite_more_important_and_reuse_slot {
+                        *slot = (declaration, importance);
+                        return true;
+                    }
+
+                    // NOTE(emilio): We could avoid this and just override for
+                    // properties not affected by logical props, but it's not
+                    // clear it's worth it given the `definitely_new` check.
+                    index_to_remove = Some(i);
+                    break;
                 }
             }
+
+            if let Some(index) = index_to_remove {
+                self.declarations.remove(index);
+                self.declarations.push((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;