Auto merge of #17973 - ferjm:ordered-map, r=heycam

stylo: ensure consistent order for custom properties computed values

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

From https://bugzilla.mozilla.org/show_bug.cgi?id=1379577

<!-- 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/17973)
<!-- Reviewable:end -->

components/style/custom_properties.rs

@@ -13,9 +13,10 @@ use properties::{CSSWideKeyword, DeclaredValue};
 use selectors::parser::SelectorParseError;
 use servo_arc::Arc;
 use std::ascii::AsciiExt;
-use std::borrow::Cow;
+use std::borrow::{Borrow, Cow};
-use std::collections::{HashMap, hash_map, HashSet};
+use std::collections::{HashMap, HashSet};
 use std::fmt;
+use std::hash::Hash;
 use style_traits::{HasViewportPercentage, ToCss, StyleParseError, ParseError};
 
 /// A custom property name is just an `Atom`.
@@ -97,52 +98,112 @@ impl ToCss for ComputedValue {
 /// DOM. CSSDeclarations expose property names as indexed properties, which
 /// need to be stable. So we keep an array of property names which order is
 /// determined on the order that they are added to the name-value map.
+pub type CustomPropertiesMap = OrderedMap<Name, ComputedValue>;
+
+/// A map that preserves order for the keys, and that is easily indexable.
 #[derive(Clone, Debug, Eq, PartialEq)]
-pub struct CustomPropertiesMap {
+pub struct OrderedMap<K, V>
-    /// Custom property name index.
+where
-    index: Vec<Name>,
+    K: Eq + Hash + Clone,
-    /// Computed values indexed by custom property name.
+{
-    values: HashMap<Name, ComputedValue>,
+    /// Key index.
+    index: Vec<K>,
+    /// Key-value map.
+    values: HashMap<K, V>,
 }
 
-impl CustomPropertiesMap {
+impl<K, V> OrderedMap<K, V>
-    /// Creates a new custom properties map.
+where
+    K: Eq + Hash + Clone,
+{
+    /// Creates a new ordered map.
     pub fn new() -> Self {
-        CustomPropertiesMap {
+        OrderedMap {
             index: Vec::new(),
             values: HashMap::new(),
         }
     }
 
-    /// Insert a computed value if it has not previously been inserted.
+    /// Insert a new key-value pair.
-    pub fn insert(&mut self, name: &Name, value: ComputedValue) {
+    pub fn insert(&mut self, key: K, value: V) {
-        debug_assert!(!self.index.contains(name));
+        if !self.values.contains_key(&key) {
-        self.index.push(name.clone());
+            self.index.push(key.clone());
-        self.values.insert(name.clone(), value);
+        }
+        self.values.insert(key, value);
     }
 
-    /// Custom property computed value getter by name.
+    /// Get a value given its key.
-    pub fn get_computed_value(&self, name: &Name) -> Option<&ComputedValue> {
+    pub fn get(&self, key: &K) -> Option<&V> {
-        let value = self.values.get(name);
+        let value = self.values.get(key);
-        debug_assert_eq!(value.is_some(), self.index.contains(name));
+        debug_assert_eq!(value.is_some(), self.index.contains(key));
         value
     }
 
-    /// Get the name of a custom property given its list index.
+    /// Get the key located at the given index.
-    pub fn get_name_at(&self, index: u32) -> Option<&Name> {
+    pub fn get_key_at(&self, index: u32) -> Option<&K> {
         self.index.get(index as usize)
     }
 
-    /// Get an iterator for custom properties computed values.
+    /// Get an ordered map iterator.
-    pub fn iter(&self) -> hash_map::Iter<Name, ComputedValue> {
+    pub fn iter<'a>(&'a self) -> OrderedMapIterator<'a, K, V> {
-        self.values.iter()
+        OrderedMapIterator {
+            inner: self,
+            pos: 0,
+        }
     }
 
-    /// Get the count of custom properties computed values.
+    /// Get the count of items in the map.
     pub fn len(&self) -> usize {
         debug_assert_eq!(self.values.len(), self.index.len());
         self.values.len()
     }
+
+    /// Remove an item given its key.
+    fn remove<Q: ?Sized>(&mut self, key: &Q) -> Option<V>
+    where
+        K: Borrow<Q>,
+        Q: Hash + Eq,
+    {
+        let index = match self.index.iter().position(|k| k.borrow() == key) {
+            Some(p) => p,
+            None => return None,
+        };
+        self.index.remove(index);
+        self.values.remove(key)
+    }
+}
+
+/// An iterator for OrderedMap.
+///
+/// The iteration order is determined by the order that the values are
+/// added to the key-value map.
+pub struct OrderedMapIterator<'a, K, V>
+where
+    K: 'a + Eq + Hash + Clone, V: 'a,
+{
+    /// The OrderedMap itself.
+    inner: &'a OrderedMap<K, V>,
+    /// The position of the iterator.
+    pos: usize,
+}
+
+impl<'a, K, V> Iterator for OrderedMapIterator<'a, K, V>
+where
+    K: Eq + Hash + Clone,
+{
+    type Item = (&'a K, &'a V);
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let ref index = self.inner.index;
+        if self.pos >= index.len() {
+            return None;
+        }
+
+        let ref key = index[index.len() - self.pos - 1];
+        self.pos += 1;
+        let value = self.inner.values.get(key).unwrap();
+        Some((key, value))
+    }
 }
 
 impl ComputedValue {
@@ -387,7 +448,7 @@ fn parse_var_function<'i, 't>(input: &mut Parser<'i, 't>,
 
 /// Add one custom property declaration to a map, unless another with the same
 /// name was already there.
-pub fn cascade<'a>(custom_properties: &mut Option<HashMap<&'a Name, BorrowedSpecifiedValue<'a>>>,
+pub fn cascade<'a>(custom_properties: &mut Option<OrderedMap<&'a Name, BorrowedSpecifiedValue<'a>>>,
                    inherited: &'a Option<Arc<CustomPropertiesMap>>,
                    seen: &mut HashSet<&'a Name>,
                    name: &'a Name,
@@ -400,17 +461,19 @@ pub fn cascade<'a>(custom_properties: &mut Option<HashMap<&'a Name, BorrowedSpec
     let map = match *custom_properties {
         Some(ref mut map) => map,
         None => {
-            *custom_properties = Some(match *inherited {
+            let mut map = OrderedMap::new();
-                Some(ref inherited) => inherited.iter().map(|(key, inherited_value)| {
+            if let Some(ref inherited) = *inherited {
-                    (key, BorrowedSpecifiedValue {
+                for name in &inherited.index {
+                    let inherited_value = inherited.get(name).unwrap();
+                    map.insert(name, BorrowedSpecifiedValue {
                         css: &inherited_value.css,
                         first_token_type: inherited_value.first_token_type,
                         last_token_type: inherited_value.last_token_type,
                         references: None
                     })
-                }).collect(),
+                }
-                None => HashMap::new(),
+            }
-            });
+            *custom_properties = Some(map);
             custom_properties.as_mut().unwrap()
         }
     };
@@ -440,7 +503,7 @@ pub fn cascade<'a>(custom_properties: &mut Option<HashMap<&'a Name, BorrowedSpec
 /// to remove any potential cycles, and wrap it in an arc.
 ///
 /// Otherwise, just use the inherited custom properties map.
-pub fn finish_cascade(specified_values_map: Option<HashMap<&Name, BorrowedSpecifiedValue>>,
+pub fn finish_cascade(specified_values_map: Option<OrderedMap<&Name, BorrowedSpecifiedValue>>,
                       inherited: &Option<Arc<CustomPropertiesMap>>)
                       -> Option<Arc<CustomPropertiesMap>> {
     if let Some(mut map) = specified_values_map {
@@ -455,15 +518,15 @@ pub fn finish_cascade(specified_values_map: Option<HashMap<&Name, BorrowedSpecif
 ///
 /// The initial value of a custom property is represented by this property not
 /// being in the map.
-fn remove_cycles(map: &mut HashMap<&Name, BorrowedSpecifiedValue>) {
+fn remove_cycles(map: &mut OrderedMap<&Name, BorrowedSpecifiedValue>) {
     let mut to_remove = HashSet::new();
     {
         let mut visited = HashSet::new();
         let mut stack = Vec::new();
-        for name in map.keys() {
+        for name in &map.index {
             walk(map, name, &mut stack, &mut visited, &mut to_remove);
 
-            fn walk<'a>(map: &HashMap<&'a Name, BorrowedSpecifiedValue<'a>>,
+            fn walk<'a>(map: &OrderedMap<&'a Name, BorrowedSpecifiedValue<'a>>,
                         name: &'a Name,
                         stack: &mut Vec<&'a Name>,
                         visited: &mut HashSet<&'a Name>,
@@ -472,7 +535,7 @@ fn remove_cycles(map: &mut HashMap<&Name, BorrowedSpecifiedValue>) {
                 if already_visited_before {
                     return
                 }
-                if let Some(value) = map.get(name) {
+                if let Some(value) = map.get(&name) {
                     if let Some(references) = value.references {
                         stack.push(name);
                         for next in references {
@@ -491,18 +554,20 @@ fn remove_cycles(map: &mut HashMap<&Name, BorrowedSpecifiedValue>) {
             }
         }
     }
-    for name in &to_remove {
+    for name in to_remove {
-        map.remove(name);
+        map.remove(&name);
     }
 }
 
 /// Replace `var()` functions for all custom properties.
-fn substitute_all(specified_values_map: HashMap<&Name, BorrowedSpecifiedValue>,
+fn substitute_all(specified_values_map: OrderedMap<&Name, BorrowedSpecifiedValue>,
                   inherited: &Option<Arc<CustomPropertiesMap>>)
                   -> CustomPropertiesMap {
     let mut custom_properties_map = CustomPropertiesMap::new();
     let mut invalid = HashSet::new();
-    for (&name, value) in &specified_values_map {
+    for name in &specified_values_map.index {
+        let value = specified_values_map.get(name).unwrap();
+
         // If this value is invalid at computed-time it won’t be inserted in computed_values_map.
         // Nothing else to do.
         let _ = substitute_one(
@@ -519,13 +584,13 @@ fn substitute_all(specified_values_map: HashMap<&Name, BorrowedSpecifiedValue>,
 /// or `Ok(last_token_type that was pushed to partial_computed_value)` otherwise.
 fn substitute_one(name: &Name,
                   specified_value: &BorrowedSpecifiedValue,
-                  specified_values_map: &HashMap<&Name, BorrowedSpecifiedValue>,
+                  specified_values_map: &OrderedMap<&Name, BorrowedSpecifiedValue>,
                   inherited: &Option<Arc<CustomPropertiesMap>>,
                   partial_computed_value: Option<&mut ComputedValue>,
                   custom_properties_map: &mut CustomPropertiesMap,
                   invalid: &mut HashSet<Name>)
                   -> Result<TokenSerializationType, ()> {
-    if let Some(computed_value) = custom_properties_map.get_computed_value(&name) {
+    if let Some(computed_value) = custom_properties_map.get(&name) {
         if let Some(partial_computed_value) = partial_computed_value {
             partial_computed_value.push_variable(computed_value)
         }
@@ -543,7 +608,7 @@ fn substitute_one(name: &Name,
         let result = substitute_block(
             &mut input, &mut position, &mut partial_computed_value,
             &mut |name, partial_computed_value| {
-                if let Some(other_specified_value) = specified_values_map.get(name) {
+                if let Some(other_specified_value) = specified_values_map.get(&name) {
                     substitute_one(name, other_specified_value, specified_values_map, inherited,
                                    Some(partial_computed_value), custom_properties_map, invalid)
                 } else {
@@ -575,7 +640,7 @@ fn substitute_one(name: &Name,
         partial_computed_value.push_variable(&computed_value)
     }
     let last_token_type = computed_value.last_token_type;
-    custom_properties_map.insert(name, computed_value);
+    custom_properties_map.insert(name.clone(), computed_value);
     Ok(last_token_type)
 }
 
@@ -679,7 +744,7 @@ pub fn substitute<'i>(input: &'i str, first_token_type: TokenSerializationType,
     let mut position = (input.position(), first_token_type);
     let last_token_type = substitute_block(
         &mut input, &mut position, &mut substituted, &mut |name, substituted| {
-            if let Some(value) = computed_values_map.as_ref().and_then(|map| map.get_computed_value(name)) {
+            if let Some(value) = computed_values_map.as_ref().and_then(|map| map.get(name)) {
                 substituted.push_variable(value);
                 Ok(value.last_token_type)
             } else {

components/style/properties/properties.mako.rs

@@ -2314,7 +2314,7 @@ impl ComputedValuesInner {
             PropertyDeclarationId::Custom(name) => {
                 self.custom_properties
                     .as_ref()
-                    .and_then(|map| map.get_computed_value(name))
+                    .and_then(|map| map.get(name))
                     .map(|value| value.to_css_string())
                     .unwrap_or(String::new())
             }

ports/geckolib/glue.rs

@@ -3519,7 +3519,7 @@ pub extern "C" fn Servo_GetCustomPropertyValue(computed_values: ServoStyleContex
     };
 
     let name = unsafe { Atom::from((&*name)) };
-    let computed_value = match custom_properties.get_computed_value(&name) {
+    let computed_value = match custom_properties.get(&name) {
         Some(v) => v,
         None => return false,
     };
@@ -3545,7 +3545,7 @@ pub extern "C" fn Servo_GetCustomPropertyNameAt(computed_values: ServoStyleConte
         None => return false,
     };
 
-    let property_name = match custom_properties.get_name_at(index) {
+    let property_name = match custom_properties.get_key_at(index) {
         Some(n) => n,
         None => return false,
     };