style: Undo the optimization for grabbing animation rules from the style data. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 09 Jul 2017 21:01:00 +0200
changeset 606837 753aad344fb9c98759869438ef36efb797d81500
parent 606836 e6690861830433f2c21d97d805e13a0e79db6ea5
child 606838 cd8e3f97bbde3df4248cbb566007f30dba7bb567
push id67812
push userbmo:emilio+bugs@crisal.io
push dateTue, 11 Jul 2017 13:56:11 +0000
milestone56.0a1
style: Undo the optimization for grabbing animation rules from the style data. This is unfortunate, but for now it complicates things, I would like not needing a ElementData to get the style of an Element in order to fix all the getDefaultComputedStyle bugs. MozReview-Commit-ID: LZvsdFEqrDE
servo/components/style/data.rs
servo/components/style/dom.rs
servo/components/style/matching.rs
servo/components/style/properties/declaration_block.rs
servo/components/style/rule_tree/mod.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -3,21 +3,21 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! Per-node data used in style calculation.
 
 use arrayvec::ArrayVec;
 use context::SharedStyleContext;
 use dom::TElement;
 use invalidation::element::restyle_hints::RestyleHint;
-use properties::{AnimationRules, ComputedValues, PropertyDeclarationBlock};
+use properties::ComputedValues;
 use properties::longhands::display::computed_value as display;
 use rule_tree::StrongRuleNode;
 use selector_parser::{EAGER_PSEUDO_COUNT, PseudoElement, RestyleDamage};
-use shared_lock::{Locked, StylesheetGuards};
+use shared_lock::StylesheetGuards;
 use std::ops::{Deref, DerefMut};
 use stylearc::Arc;
 
 bitflags! {
     flags RestyleFlags: u8 {
         /// Whether the styles changed for this restyle.
         const WAS_RESTYLED = 1 << 0,
         /// Whether we reframed/reconstructed any ancestor or self.
@@ -397,34 +397,9 @@ impl ElementData {
         let (other_important_rules, _custom) = rules.get_properties_overriding_animations(&guards);
         important_rules != other_important_rules
     }
 
     /// Drops any restyle state from the element.
     pub fn clear_restyle_state(&mut self) {
         self.restyle.clear();
     }
-
-    /// Returns SMIL overriden value if exists.
-    pub fn get_smil_override(&self) -> Option<&Arc<Locked<PropertyDeclarationBlock>>> {
-        if cfg!(feature = "servo") {
-            // Servo has no knowledge of a SMIL rule, so just avoid looking for it.
-            return None;
-        }
-
-        match self.styles.get_primary() {
-            Some(v) => v.rules().get_smil_animation_rule(),
-            None => None,
-        }
-    }
-
-    /// Returns AnimationRules that has processed during animation-only restyles.
-    pub fn get_animation_rules(&self) -> AnimationRules {
-        if cfg!(feature = "servo") {
-            return AnimationRules(None, None)
-        }
-
-        match self.styles.get_primary() {
-            Some(v) => v.rules().get_animation_rules(),
-            None => AnimationRules(None, None),
-        }
-    }
 }
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -10,17 +10,17 @@
 use {Atom, Namespace, LocalName};
 use applicable_declarations::ApplicableDeclarationBlock;
 use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
 #[cfg(feature = "gecko")] use context::UpdateAnimationsTasks;
 use data::ElementData;
 use element_state::ElementState;
 use font_metrics::FontMetricsProvider;
 use media_queries::Device;
-use properties::{ComputedValues, PropertyDeclarationBlock};
+use properties::{AnimationRules, ComputedValues, PropertyDeclarationBlock};
 #[cfg(feature = "gecko")] use properties::animated_properties::AnimationValue;
 #[cfg(feature = "gecko")] use properties::animated_properties::TransitionProperty;
 use rule_tree::CascadeLevel;
 use selector_parser::{AttrValue, ElementExt, PreExistingComputedValues};
 use selector_parser::{PseudoClassStringArg, PseudoElement};
 use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode};
 use selectors::sink::Push;
 use shared_lock::Locked;
@@ -374,16 +374,28 @@ pub trait TElement : Eq + PartialEq + De
 
     /// Get this element's animation rule by the cascade level.
     fn get_animation_rule_by_cascade(&self,
                                      _cascade_level: CascadeLevel)
                                      -> Option<Arc<Locked<PropertyDeclarationBlock>>> {
         None
     }
 
+    /// Get the combined animation and transition rules.
+    fn get_animation_rules(&self) -> AnimationRules {
+        if !self.may_have_animations() {
+            return AnimationRules(None, None)
+        }
+
+        AnimationRules(
+            self.get_animation_rule(),
+            self.get_transition_rule(),
+        )
+    }
+
     /// Get this element's animation rule.
     fn get_animation_rule(&self)
                           -> Option<Arc<Locked<PropertyDeclarationBlock>>> {
         None
     }
 
     /// Get this element's transition rule.
     fn get_transition_rule(&self)
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -232,17 +232,17 @@ impl CascadeVisitedMode {
         match *self {
             CascadeVisitedMode::Unvisited => styles.pseudos.take(pseudo),
             CascadeVisitedMode::Visited => inputs.take_visited_values(),
         }
     }
 
     /// Returns whether there might be visited values that should be inserted
     /// within the regular computed values based on the cascade mode.
-    fn visited_values_for_insertion(&self) -> bool {
+    pub fn visited_values_for_insertion(&self) -> bool {
         *self == CascadeVisitedMode::Unvisited
     }
 
     /// Returns whether animations should be processed based on the cascade
     /// mode.  At the moment, it appears we don't need to support animating
     /// visited styles.
     fn should_process_animations(&self) -> bool {
         *self == CascadeVisitedMode::Unvisited
@@ -253,17 +253,17 @@ impl CascadeVisitedMode {
     /// styles.  TODO: Verify this is correct as part of
     /// https://bugzilla.mozilla.org/show_bug.cgi?id=1364484.
     fn should_accumulate_damage(&self) -> bool {
         *self == CascadeVisitedMode::Unvisited
     }
 
     /// Returns whether the cascade should filter to only visited dependent
     /// properties based on the cascade mode.
-    fn visited_dependent_only(&self) -> bool {
+    pub fn visited_dependent_only(&self) -> bool {
         *self == CascadeVisitedMode::Visited
     }
 }
 
 trait PrivateMatchMethods: TElement {
     /// Returns the closest parent element that doesn't have a display: contents
     /// style (and thus generates a box).
     ///
@@ -441,17 +441,17 @@ trait PrivateMatchMethods: TElement {
             // computing default styles, we aren't guaranteed the parent
             // will have eagerly computed our styles, so we just handled it
             // below like a lazy pseudo.
             let only_default_rules = context.shared.traversal_flags.for_default_styles();
             if pseudo.is_eager() && !only_default_rules {
                 debug_assert!(pseudo.is_before_or_after());
                 let parent = self.parent_element().unwrap();
                 if !parent.may_have_animations() ||
-                   primary_inputs.rules().get_animation_rules().is_empty() {
+                   self.get_animation_rules().is_empty() {
                     let parent_data = parent.borrow_data().unwrap();
                     let pseudo_style =
                         parent_data.styles.pseudos.get(&pseudo).unwrap();
                     let values = cascade_visited.values(pseudo_style);
                     return values.clone()
                 }
             }
         }
@@ -989,22 +989,22 @@ impl MatchingResults {
         }
     }
 }
 
 /// The public API that elements expose for selector matching.
 pub trait MatchMethods : TElement {
     /// Performs selector matching and property cascading on an element and its
     /// eager pseudos.
-    fn match_and_cascade(&self,
-                         context: &mut StyleContext<Self>,
-                         data: &mut ElementData,
-                         sharing: StyleSharingBehavior)
-                         -> ChildCascadeRequirement
-    {
+    fn match_and_cascade(
+        &self,
+        context: &mut StyleContext<Self>,
+        data: &mut ElementData,
+        sharing: StyleSharingBehavior
+    ) -> ChildCascadeRequirement {
         debug!("Match and cascade for {:?}", self);
 
         // Perform selector matching for the primary style.
         let primary_results =
             self.match_primary(context, data, VisitedHandlingMode::AllLinksUnvisited);
         let important_rules_changed =
             primary_results.important_rules_overriding_animation_changed;
 
@@ -1016,26 +1016,34 @@ pub trait MatchMethods : TElement {
         if relevant_link_found {
             self.match_primary(context, data, VisitedHandlingMode::RelevantLinkVisited);
         }
 
         // Even if there is no relevant link, we need to cascade visited styles
         // if our parent has visited styles.
         let parent_and_styles = self.get_inherited_style_and_parent();
         if relevant_link_found || parent_and_styles.has_visited_style() {
-            self.cascade_primary(context, data, important_rules_changed,
-                                 &parent_and_styles,
-                                 CascadeVisitedMode::Visited);
+            self.cascade_primary(
+                context,
+                data,
+                important_rules_changed,
+                &parent_and_styles,
+                CascadeVisitedMode::Visited
+            );
         }
 
         // Cascade properties and compute primary values.
         let child_cascade_requirement =
-            self.cascade_primary(context, data, important_rules_changed,
-                                 &parent_and_styles,
-                                 CascadeVisitedMode::Unvisited);
+            self.cascade_primary(
+                context,
+                data,
+                important_rules_changed,
+                &parent_and_styles,
+                CascadeVisitedMode::Unvisited
+            );
 
         // Match and cascade eager pseudo-elements.
         if !data.styles.is_display_none() {
             self.match_pseudos(context, data, VisitedHandlingMode::AllLinksUnvisited);
 
             // If there's a relevant link involved, match and cascade eager
             // pseudo-element styles as if the link is visited as well.
             // This runs after matching for regular styles because matching adds
@@ -1104,22 +1112,22 @@ pub trait MatchMethods : TElement {
         child_cascade_requirement
     }
 
     /// Runs selector matching to (re)compute the primary rule node for this
     /// element.
     ///
     /// Returns `MatchingResults` with the new rules and other associated data
     /// from the matching process.
-    fn match_primary(&self,
-                     context: &mut StyleContext<Self>,
-                     data: &mut ElementData,
-                     visited_handling: VisitedHandlingMode)
-                     -> MatchingResults
-    {
+    fn match_primary(
+        &self,
+        context: &mut StyleContext<Self>,
+        data: &mut ElementData,
+        visited_handling: VisitedHandlingMode
+    ) -> MatchingResults {
         debug!("Match primary for {:?}, visited: {:?}", self, visited_handling);
 
         let mut primary_inputs = context.thread_local.current_element_info
                                         .as_mut().unwrap()
                                         .cascade_inputs.ensure_primary();
 
         let only_default_rules = context.shared.traversal_flags.for_default_styles();
         let implemented_pseudo = self.implemented_pseudo_element();
@@ -1140,17 +1148,17 @@ pub trait MatchMethods : TElement {
                 // bothered to store pseudo styles there.  In this case, we just
                 // treat it like a lazily computed pseudo.
                 let parent = self.parent_element().unwrap();
                 let parent_data = parent.borrow_data().unwrap();
                 let pseudo_style =
                     parent_data.styles.pseudos.get(&pseudo).unwrap();
                 let mut rules = pseudo_style.rules().clone();
                 if parent.may_have_animations() {
-                    let animation_rules = data.get_animation_rules();
+                    let animation_rules = self.get_animation_rules();
 
                     // Handle animations here.
                     if let Some(animation_rule) = animation_rules.0 {
                         let animation_rule_node =
                             context.shared.stylist.rule_tree()
                                 .update_rule_at_level(CascadeLevel::Animations,
                                                       Some(&animation_rule),
                                                       &mut rules,
@@ -1203,22 +1211,18 @@ pub trait MatchMethods : TElement {
         let bloom_filter = context.thread_local.bloom_filter.filter();
         let mut matching_context =
             MatchingContext::new_for_visited(MatchingMode::Normal,
                                              Some(bloom_filter),
                                              visited_handling,
                                              context.shared.quirks_mode);
 
         {
-            let smil_override = data.get_smil_override();
-            let animation_rules = if self.may_have_animations() {
-                data.get_animation_rules()
-            } else {
-                AnimationRules(None, None)
-            };
+            let smil_override = self.get_smil_override();
+            let animation_rules = self.get_animation_rules();
 
             // Compute the primary rule node.
             stylist.push_applicable_declarations(self,
                                                  implemented_pseudo.as_ref(),
                                                  style_attribute,
                                                  smil_override,
                                                  animation_rules,
                                                  rule_inclusion,
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -23,20 +23,20 @@ use stylesheets::{MallocSizeOf, MallocSi
 use super::*;
 use values::computed::Context;
 #[cfg(feature = "gecko")] use properties::animated_properties::AnimationValueMap;
 
 /// The animation rules.
 ///
 /// The first one is for Animation cascade level, and the second one is for
 /// Transition cascade level.
-pub struct AnimationRules<'a>(pub Option<&'a Arc<Locked<PropertyDeclarationBlock>>>,
-                              pub Option<&'a Arc<Locked<PropertyDeclarationBlock>>>);
+pub struct AnimationRules(pub Option<Arc<Locked<PropertyDeclarationBlock>>>,
+                          pub Option<Arc<Locked<PropertyDeclarationBlock>>>);
 
-impl<'a> AnimationRules<'a> {
+impl AnimationRules {
     /// Returns whether these animation rules represents an actual rule or not.
     pub fn is_empty(&self) -> bool {
         self.0.is_none() && self.1.is_none()
     }
 }
 
 /// A declaration [importance][importance].
 ///
--- a/servo/components/style/rule_tree/mod.rs
+++ b/servo/components/style/rule_tree/mod.rs
@@ -4,17 +4,17 @@
 
 #![allow(unsafe_code)]
 
 //! The rule tree.
 
 use applicable_declarations::ApplicableDeclarationList;
 #[cfg(feature = "servo")]
 use heapsize::HeapSizeOf;
-use properties::{AnimationRules, Importance, LonghandIdSet, PropertyDeclarationBlock};
+use properties::{Importance, LonghandIdSet, PropertyDeclarationBlock};
 use shared_lock::{Locked, StylesheetGuards, SharedRwLockReadGuard};
 use smallvec::SmallVec;
 use std::io::{self, Write};
 use std::mem;
 use std::ptr;
 use std::sync::atomic::{AtomicPtr, AtomicUsize, Ordering};
 use stylearc::{Arc, NonZeroPtrMut};
 use stylesheets::StyleRule;
@@ -1316,42 +1316,16 @@ impl StrongRuleNode {
             return None;
         }
 
         self.self_and_ancestors()
             .take_while(|node| node.cascade_level() >= CascadeLevel::SMILOverride)
             .find(|node| node.cascade_level() == CascadeLevel::SMILOverride)
             .map(|node| node.get_animation_style())
     }
-
-    /// Returns AnimationRules that has processed during animation-only restyles.
-    pub fn get_animation_rules(&self) -> AnimationRules {
-        if cfg!(feature = "servo") {
-            return AnimationRules(None, None);
-        }
-
-        let mut animation = None;
-        let mut transition = None;
-
-        for node in self.self_and_ancestors()
-                        .take_while(|node| node.cascade_level() >= CascadeLevel::Animations) {
-            match node.cascade_level() {
-                CascadeLevel::Animations => {
-                    debug_assert!(animation.is_none());
-                    animation = Some(node.get_animation_style())
-                },
-                CascadeLevel::Transitions => {
-                    debug_assert!(transition.is_none());
-                    transition = Some(node.get_animation_style())
-                },
-                _ => {},
-            }
-        }
-        AnimationRules(animation, transition)
-    }
 }
 
 /// An iterator over a rule node and its ancestors.
 #[derive(Clone)]
 pub struct SelfAndAncestors<'a> {
     current: Option<&'a StrongRuleNode>,
 }