servo: Merge #17831 - Iterate through properties in priority order when computing keyframes (from birtles:property-priorities); r=heycam
authorBrian Birtles <birtles@gmail.com>
Sun, 23 Jul 2017 21:54:12 -0700
changeset 419230 cf34b8de856895e7d61b004dc54d2d4f3c4edea3
parent 419229 e09385879b6e71ab3d61e3aa2f8e5bbef0d10999
child 419231 e89eb57f2e97dc69da10ae56b3fac1c59c4117f9
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs17831, 1371493
milestone56.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 #17831 - Iterate through properties in priority order when computing keyframes (from birtles:property-priorities); r=heycam This is largely just a translation of Gecko's PropertyPriorityIterator[1] into rust with the exception that IDL sort order is only defined for shorthands since that's all we currently require. [1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/animation/KeyframeUtils.cpp#151 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy --stylo` does not report any errors - [x] These changes fix (Gecko bug 1371493)[https://bugzilla.mozilla.org/show_bug.cgi?id=1371493]. - [x] There are tests for these changes OR <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: a47fde038e893d4b76d64b6917085d8e5bc9d8d1
servo/components/style/properties/data.py
servo/components/style/properties/helpers/animated_properties.mako.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/properties/data.py
+++ b/servo/components/style/properties/data.py
@@ -41,16 +41,21 @@ def to_camel_case(ident):
     return re.sub("(^|_|-)([a-z])", lambda m: m.group(2).upper(), ident.strip("_").strip("-"))
 
 
 def to_camel_case_lower(ident):
     camel = to_camel_case(ident)
     return camel[0].lower() + camel[1:]
 
 
+# https://drafts.csswg.org/cssom/#css-property-to-idl-attribute
+def to_idl_name(ident):
+    return re.sub("-([a-z])", lambda m: m.group(1).upper(), ident)
+
+
 def parse_aliases(value):
     aliases = {}
     for pair in value.split():
         [a, v] = pair.split("=")
         aliases[a] = v
     return aliases
 
 
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -1,15 +1,15 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 <%namespace name="helpers" file="/helpers.mako.rs" />
 
-<% from data import SYSTEM_FONT_LONGHANDS %>
+<% from data import to_idl_name, SYSTEM_FONT_LONGHANDS %>
 
 use app_units::Au;
 use cssparser::{Parser, RGBA};
 use euclid::{Point2D, Size2D};
 #[cfg(feature = "gecko")] use gecko_bindings::bindings::RawServoAnimationValueMap;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::RawGeckoGfxMatrix4x4;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::nsCSSPropertyID;
 #[cfg(feature = "gecko")] use gecko_bindings::sugar::ownership::{HasFFI, HasSimpleFFI};
@@ -18,17 +18,18 @@ use properties::{CSSWideKeyword, Propert
 use properties::longhands;
 use properties::longhands::font_weight::computed_value::T as FontWeight;
 use properties::longhands::font_stretch::computed_value::T as FontStretch;
 use properties::longhands::transform::computed_value::ComputedMatrix;
 use properties::longhands::transform::computed_value::ComputedOperation as TransformOperation;
 use properties::longhands::transform::computed_value::T as TransformList;
 use properties::longhands::vertical_align::computed_value::T as VerticalAlign;
 use properties::longhands::visibility::computed_value::T as Visibility;
-#[cfg(feature = "gecko")] use properties::{PropertyDeclarationId, LonghandId};
+#[cfg(feature = "gecko")] use properties::{PropertyId, PropertyDeclarationId, LonghandId};
+#[cfg(feature = "gecko")] use properties::{ShorthandId};
 use selectors::parser::SelectorParseError;
 use smallvec::SmallVec;
 use std::cmp;
 #[cfg(feature = "gecko")] use fnv::FnvHashMap;
 use style_traits::ParseError;
 use super::ComputedValues;
 #[cfg(any(feature = "gecko", feature = "testing"))]
 use values::Auto;
@@ -3199,8 +3200,61 @@ impl Animatable for AnimatedFilterList {
                 from = from_iter.next();
                 to = to_iter.next();
             }
             square_distance += current_square_distance;
         }
         Ok(square_distance.sqrt())
     }
 }
+
+/// A comparator to sort PropertyIds such that longhands are sorted before shorthands,
+/// shorthands with fewer components are sorted before shorthands with more components,
+/// and otherwise shorthands are sorted by IDL name as defined by [Web Animations][property-order].
+///
+/// Using this allows us to prioritize values specified by longhands (or smaller
+/// shorthand subsets) when longhands and shorthands are both specified on the one keyframe.
+///
+/// Example orderings that result from this:
+///
+///   margin-left, margin
+///
+/// and:
+///
+///   border-top-color, border-color, border-top, border
+///
+/// [property-order] https://w3c.github.io/web-animations/#calculating-computed-keyframes
+#[cfg(feature = "gecko")]
+pub fn compare_property_priority(a: &PropertyId, b: &PropertyId) -> cmp::Ordering {
+    match (a.as_shorthand(), b.as_shorthand()) {
+        // Within shorthands, sort by the number of subproperties, then by IDL name.
+        (Ok(a), Ok(b)) => {
+            let subprop_count_a = a.longhands().len();
+            let subprop_count_b = b.longhands().len();
+            subprop_count_a.cmp(&subprop_count_b).then_with(
+                || get_idl_name_sort_order(&a).cmp(&get_idl_name_sort_order(&b)))
+        },
+
+        // Longhands go before shorthands.
+        (Ok(_), Err(_)) => cmp::Ordering::Greater,
+        (Err(_), Ok(_)) => cmp::Ordering::Less,
+
+        // Both are longhands or custom properties in which case they don't overlap and should
+        // sort equally.
+        _ => cmp::Ordering::Equal,
+    }
+}
+
+#[cfg(feature = "gecko")]
+fn get_idl_name_sort_order(shorthand: &ShorthandId) -> u32 {
+<%
+# Sort by IDL name.
+sorted_shorthands = sorted(data.shorthands, key=lambda p: to_idl_name(p.ident))
+
+# Annotate with sorted position
+sorted_shorthands = [(p, position) for position, p in enumerate(sorted_shorthands)]
+%>
+    match *shorthand {
+        % for property, position in sorted_shorthands:
+            ShorthandId::${property.camel_case} => ${position},
+        % endfor
+    }
+}
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -78,30 +78,32 @@ use style::gecko_bindings::structs::RawG
 use style::gecko_bindings::structs::RawGeckoPresContextOwned;
 use style::gecko_bindings::structs::ServoElementSnapshotTable;
 use style::gecko_bindings::structs::StyleRuleInclusion;
 use style::gecko_bindings::structs::URLExtraData;
 use style::gecko_bindings::structs::nsCSSValueSharedList;
 use style::gecko_bindings::structs::nsCompatibility;
 use style::gecko_bindings::structs::nsIDocument;
 use style::gecko_bindings::structs::nsStyleTransformMatrix::MatrixTransformOperator;
+use style::gecko_bindings::structs::nsTArray;
 use style::gecko_bindings::structs::nsresult;
 use style::gecko_bindings::sugar::ownership::{FFIArcHelpers, HasFFI, HasArcFFI, HasBoxFFI};
 use style::gecko_bindings::sugar::ownership::{HasSimpleFFI, Strong};
 use style::gecko_bindings::sugar::refptr::RefPtr;
 use style::gecko_properties::{self, style_structs};
 use style::invalidation::element::restyle_hints::{self, RestyleHint};
 use style::media_queries::{MediaList, parse_media_query_list};
 use style::parallel;
 use style::parser::ParserContext;
 use style::properties::{ComputedValues, Importance};
 use style::properties::{IS_FIELDSET_CONTENT, LonghandIdSet};
-use style::properties::{PropertyDeclaration, PropertyDeclarationBlock, PropertyId};
+use style::properties::{PropertyDeclaration, PropertyDeclarationBlock, PropertyId, ShorthandId};
 use style::properties::{SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, SourcePropertyDeclaration, StyleBuilder};
 use style::properties::animated_properties::{Animatable, AnimatableLonghand, AnimationValue};
+use style::properties::animated_properties::compare_property_priority;
 use style::properties::parse_one_declaration_into;
 use style::rule_tree::StyleSource;
 use style::selector_parser::PseudoElementCascadeType;
 use style::sequential;
 use style::shared_lock::{SharedRwLockReadGuard, StylesheetGuards, ToCssWithGuard, Locked};
 use style::string_cache::Atom;
 use style::style_adjuster::StyleAdjuster;
 use style::stylesheets::{CssRule, CssRules, CssRuleType, CssRulesHelpers, DocumentRule};
@@ -2911,16 +2913,63 @@ fn create_context<'a>(
         ),
         font_metrics_provider: font_metrics_provider,
         cached_system_font: None,
         in_media_query: false,
         quirks_mode: per_doc_data.stylist.quirks_mode(),
     }
 }
 
+struct PropertyAndIndex {
+    property: PropertyId,
+    index: usize,
+}
+
+struct PrioritizedPropertyIter<'a> {
+    properties: &'a nsTArray<PropertyValuePair>,
+    sorted_property_indices: Vec<PropertyAndIndex>,
+    curr: usize,
+}
+
+impl<'a> PrioritizedPropertyIter<'a> {
+    pub fn new(properties: &'a nsTArray<PropertyValuePair>) -> PrioritizedPropertyIter {
+        // If we fail to convert a nsCSSPropertyID into a PropertyId we shouldn't fail outright
+        // but instead by treating that property as the 'all' property we make it sort last.
+        let all = PropertyId::Shorthand(ShorthandId::All);
+
+        let mut sorted_property_indices: Vec<PropertyAndIndex> =
+            properties.iter().enumerate().map(|(index, pair)| {
+                PropertyAndIndex {
+                    property: PropertyId::from_nscsspropertyid(pair.mProperty)
+                              .unwrap_or(all.clone()),
+                    index,
+                }
+            }).collect();
+        sorted_property_indices.sort_by(|a, b| compare_property_priority(&a.property, &b.property));
+
+        PrioritizedPropertyIter {
+            properties,
+            sorted_property_indices,
+            curr: 0,
+        }
+    }
+}
+
+impl<'a> Iterator for PrioritizedPropertyIter<'a> {
+    type Item = &'a PropertyValuePair;
+
+    fn next(&mut self) -> Option<&'a PropertyValuePair> {
+        if self.curr >= self.sorted_property_indices.len() {
+            return None
+        }
+        self.curr += 1;
+        Some(&self.properties[self.sorted_property_indices[self.curr - 1].index])
+    }
+}
+
 #[no_mangle]
 pub extern "C" fn Servo_GetComputedKeyframeValues(keyframes: RawGeckoKeyframeListBorrowed,
                                                   element: RawGeckoElementBorrowed,
                                                   style: ServoStyleContextBorrowed,
                                                   raw_data: RawServoStyleSetBorrowed,
                                                   computed_keyframes: RawGeckoComputedKeyframeValuesListBorrowedMut)
 {
     use std::mem;
@@ -2941,19 +2990,18 @@ pub extern "C" fn Servo_GetComputedKeyfr
     let guard = global_style_data.shared_lock.read();
     let default_values = data.default_computed_values();
 
     for (index, keyframe) in keyframes.iter().enumerate() {
         let ref mut animation_values = computed_keyframes[index];
 
         let mut seen = LonghandIdSet::new();
 
-        let iter = keyframe.mPropertyValues.iter();
         let mut property_index = 0;
-        for property in iter {
+        for property in PrioritizedPropertyIter::new(&keyframe.mPropertyValues) {
             if simulate_compute_values_failure(property) {
                 continue;
             }
 
             let mut maybe_append_animation_value = |property: AnimatableLonghand,
                                                     value: Option<AnimationValue>| {
                 if seen.has_animatable_longhand_bit(&property) {
                     return;