Auto merge of #17138 - emilio:parsing-decl-block, r=heycam

Bug 1369198: Ensure pushing a declaration to a declaration block for parsing always inserts it at the last position.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17138)
<!-- Reviewable:end -->

1 files changed, 60 insertions, 15 deletions

diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs
index 32b18a3d3cb..7a349db0ff3 100644
--- a/components/style/properties/declaration_block.rs
+++ b/components/style/properties/declaration_block.rs
@@ -257,22 +257,30 @@ impl PropertyDeclarationBlock {
     }
 
     /// 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(_) |
@@ -285,20 +293,32 @@ impl PropertyDeclarationBlock {
 
         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,
+                    );
                 }
             }
         }
@@ -306,28 +326,38 @@ impl PropertyDeclarationBlock {
     }
 
     /// 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
@@ -337,10 +367,25 @@ impl PropertyDeclarationBlock {
                             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() {