servo: Merge #17138 - Bug 1369198: Ensure pushing a declaration to a declaration block for parsing always inserts it at the last position (from emilio:parsing-decl-block); r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 02 Jun 2017 03:04:26 -0700
changeset 410173 6fc608d6f3fdf0c8e8fa8301f01846a07b39d2c9
parent 410172 bce41e33bffd069768eba7acb4cb121f1613307a
child 410174 17c0e9c9698b4e79bb674bbf2aa2a7c9f83507d2
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs17138, 1369198
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 #17138 - Bug 1369198: Ensure pushing a declaration to a declaration block for parsing always inserts it at the last position (from emilio:parsing-decl-block); r=heycam Source-Repo: https://github.com/servo/servo Source-Revision: fab1a6354f103bb264941fff2619bca931e48a40
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;