Bug 1334036 - Part 6: Trigger restyle if important rules are changed. draft
authorBoris Chiou <boris.chiou@gmail.com>
Fri, 12 May 2017 16:57:28 +0800
changeset 579517 ebf0fec529c31219841322bb09ef52fcc10abf80
parent 579498 cfb76f8a26663cdd25d9c1561c0501f421517c6f
child 579518 ca177529bdd8f2e42e478010c9d678f698325cdf
push id59273
push userbmo:boris.chiou@gmail.com
push dateWed, 17 May 2017 11:18:46 +0000
bugs1334036
milestone55.0a1
Bug 1334036 - Part 6: Trigger restyle if important rules are changed. If we add/remove important rules, we should call MaybeUpdateCascadeResults() to make sure EffectSet::mPropertiesWithImportantRules is correct, and so we can avoid that these important rules are overridden by animations running on compositor. Currently, we call MaybeUpdateCascadeResults only while iterating elements which needs to be restyled, so we should request a restyle on this element whose important rules are changed. Therefore, we need to set a flag if we update the primary rules which includes important ones. Therefore, we can use this flag to check if we should update cascade results on this element. If we have animations on this element, we update its effect properties, and also send a task to update cascade results. Calling get_properties_overriding_animations() might cases some impact on performance because we need to walk the rule tree, so if possible, we could just store this set into TNode to avoid finding the properties for both old and new rules each time. This could be a future work if necessary. MozReview-Commit-ID: 87MBQrirVto
layout/style/ServoBindings.cpp
servo/components/style/data.rs
servo/components/style/matching.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/traversal.rs
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -529,20 +529,32 @@ Gecko_UpdateAnimations(RawGeckoElementBo
     if (aTasks & UpdateAnimationsTasks::CSSTransitions) {
       MOZ_ASSERT(aOldComputedValues);
       const ServoComputedValuesWithParent oldServoValues =
         { aOldComputedValues, nullptr };
       presContext->TransitionManager()->
         UpdateTransitions(const_cast<dom::Element*>(aElement), pseudoType,
                           oldServoValues, servoValues);
     }
+
     if (aTasks & UpdateAnimationsTasks::EffectProperties) {
       presContext->EffectCompositor()->UpdateEffectProperties(
         servoValues, const_cast<dom::Element*>(aElement), pseudoType);
     }
+
+    if (aTasks & UpdateAnimationsTasks::CascadeResults) {
+      // This task will be scheduled if we detected any changes to !important
+      // rules. We post a restyle here so that we can update the cascade
+      // results in the pre-traversal of the next restyle.
+      presContext->EffectCompositor()
+                 ->RequestRestyle(const_cast<Element*>(aElement),
+                                  pseudoType,
+                                  EffectCompositor::RestyleType::Standard,
+                                  EffectCompositor::CascadeLevel::Animations);
+    }
   }
 }
 
 bool
 Gecko_ElementHasAnimations(RawGeckoElementBorrowed aElement)
 {
   nsIAtom* pseudoTag = PseudoTagAndCorrectElementForAnimation(aElement);
   CSSPseudoElementType pseudoType =
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -8,16 +8,17 @@
 
 use context::SharedStyleContext;
 use dom::TElement;
 use properties::ComputedValues;
 use properties::longhands::display::computed_value as display;
 use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_LATER_SIBLINGS, RESTYLE_SELF, RestyleHint};
 use rule_tree::StrongRuleNode;
 use selector_parser::{EAGER_PSEUDO_COUNT, PseudoElement, RestyleDamage};
+use shared_lock::StylesheetGuards;
 #[cfg(feature = "servo")] use std::collections::HashMap;
 use std::fmt;
 #[cfg(feature = "servo")] use std::hash::BuildHasherDefault;
 use stylearc::Arc;
 use traversal::TraversalFlags;
 
 /// The structure that represents the result of style computation. This is
 /// effectively a tuple of rules and computed values, that is, the rule node,
@@ -494,16 +495,34 @@ impl ElementData {
         if self.styles().primary.rules == rules {
             return false;
         }
 
         self.styles_mut().primary.rules = rules;
         true
     }
 
+    /// Return true if important rules are different.
+    /// We use this to make sure the cascade of off-main thread animations is correct.
+    /// Note: Ignore custom properties for now because we only support opacity and transform
+    ///       properties for animations running on compositor. Actually, we only care about opacity
+    ///       and transform for now, but it's fine to compare all properties and let the user
+    ///       the check which properties do they want.
+    ///       If it costs too much, get_properties_overriding_animations() should return a set
+    ///       containing only opacity and transform properties.
+    pub fn important_rules_are_different(&self,
+                                         rules: &StrongRuleNode,
+                                         guards: &StylesheetGuards) -> bool {
+        debug_assert!(self.has_styles());
+        let (important_rules, _custom) =
+            self.styles().primary.rules.get_properties_overriding_animations(&guards);
+        let (other_important_rules, _custom) = rules.get_properties_overriding_animations(&guards);
+        important_rules != other_important_rules
+    }
+
     /// Returns true if the Element has a RestyleData.
     pub fn has_restyle(&self) -> bool {
         self.restyle.is_some()
     }
 
     /// Drops any RestyleData.
     pub fn clear_restyle(&mut self) {
         self.restyle = None;
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -511,17 +511,18 @@ trait PrivateMatchMethods: TElement {
                                 inherit_mode)
     }
 
     /// Computes values and damage for the primary or pseudo style of an element,
     /// setting them on the ElementData.
     fn cascade_primary_or_pseudo(&self,
                                  context: &mut StyleContext<Self>,
                                  data: &mut ElementData,
-                                 pseudo: Option<&PseudoElement>) {
+                                 pseudo: Option<&PseudoElement>,
+                                 important_rules_changed: bool) {
         debug_assert!(pseudo.is_none() || self.implemented_pseudo_element().is_none(),
                       "Pseudo-element-implementing elements can't have pseudos!");
         // Collect some values.
         let (mut styles, restyle) = data.styles_and_restyle_mut();
         let mut primary_style = &mut styles.primary;
         let pseudos = &mut styles.pseudos;
         let mut pseudo_style = match pseudo {
             Some(p) => {
@@ -574,17 +575,18 @@ trait PrivateMatchMethods: TElement {
 
         // NB: Animations for pseudo-elements in Gecko are handled while
         // traversing the pseudo-elements themselves.
         if pseudo.is_none() &&
            !context.shared.traversal_flags.for_animation_only() {
             self.process_animations(context,
                                     &mut old_values,
                                     &mut new_values,
-                                    primary_style);
+                                    primary_style,
+                                    important_rules_changed);
         }
 
         // Accumulate restyle damage.
         if let Some(old) = old_values {
             // ::before and ::after are element-backed in Gecko, so they do
             // the damage calculation for themselves.
             //
             // FIXME(emilio): We have more element-backed stuff, and this is
@@ -652,18 +654,19 @@ trait PrivateMatchMethods: TElement {
         })
     }
 
     #[cfg(feature = "gecko")]
     fn process_animations(&self,
                           context: &mut StyleContext<Self>,
                           old_values: &mut Option<Arc<ComputedValues>>,
                           new_values: &mut Arc<ComputedValues>,
-                          primary_style: &ComputedStyle) {
-        use context::{CSS_ANIMATIONS, CSS_TRANSITIONS, EFFECT_PROPERTIES};
+                          primary_style: &ComputedStyle,
+                          important_rules_changed: bool) {
+        use context::{CASCADE_RESULTS, CSS_ANIMATIONS, CSS_TRANSITIONS, EFFECT_PROPERTIES};
         use context::UpdateAnimationsTasks;
 
         let mut tasks = UpdateAnimationsTasks::empty();
         if self.needs_animations_update(old_values.as_ref(), new_values) {
             tasks.insert(CSS_ANIMATIONS);
         }
 
         let before_change_style = if self.might_need_transitions_update(old_values.as_ref().map(|s| &**s),
@@ -699,32 +702,36 @@ trait PrivateMatchMethods: TElement {
                 None
             }
         } else {
             None
         };
 
         if self.has_animations() {
             tasks.insert(EFFECT_PROPERTIES);
+            if important_rules_changed {
+                tasks.insert(CASCADE_RESULTS);
+            }
         }
 
         if !tasks.is_empty() {
             let task = ::context::SequentialTask::update_animations(*self,
                                                                     before_change_style,
                                                                     tasks);
             context.thread_local.tasks.push(task);
         }
     }
 
     #[cfg(feature = "servo")]
     fn process_animations(&self,
                           context: &mut StyleContext<Self>,
                           old_values: &mut Option<Arc<ComputedValues>>,
                           new_values: &mut Arc<ComputedValues>,
-                          _primary_style: &ComputedStyle) {
+                          _primary_style: &ComputedStyle,
+                          _important_rules_changed: bool) {
         use animation;
 
         let possibly_expired_animations =
             &mut context.thread_local.current_element_info.as_mut().unwrap()
                         .possibly_expired_animations;
         let shared_context = context.shared;
         if let Some(ref mut old) = *old_values {
             self.update_animations_for_cascade(shared_context, old,
@@ -880,36 +887,68 @@ impl<E: TElement> PrivateMatchMethods fo
 #[derive(Clone, Copy, PartialEq)]
 pub enum StyleSharingBehavior {
     /// Style sharing allowed.
     Allow,
     /// Style sharing disallowed.
     Disallow,
 }
 
+/// The result status for match primary rules.
+#[derive(Debug)]
+pub struct RulesMatchedResult {
+    /// Indicate that the rule nodes are changed.
+    rule_nodes_changed: bool,
+    /// Indicate that there are any changes of important rules overriding animations.
+    important_rules_overriding_animation_changed: bool,
+}
+
+bitflags! {
+    /// Flags that represent the result of replace_rules.
+    pub flags RulesChangedFlags: u8 {
+        /// Normal rules are changed.
+        const NORMAL_RULES_CHANGED = 0x01,
+        /// Important rules are changed.
+        const IMPORTANT_RULES_CHANGED = 0x02,
+    }
+}
+
+impl RulesChangedFlags {
+    /// Return true if there are any normal rules changed.
+    #[inline]
+    pub fn normal_rules_changed(&self) -> bool {
+        self.contains(NORMAL_RULES_CHANGED)
+    }
+
+    /// Return true if there are any important rules changed.
+    #[inline]
+    pub fn important_rules_changed(&self) -> bool {
+        self.contains(IMPORTANT_RULES_CHANGED)
+    }
+}
+
 /// 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)
     {
         // Perform selector matching for the primary style.
         let mut primary_relations = StyleRelations::empty();
-        let _rule_node_changed = self.match_primary(context, data, &mut primary_relations);
+        let result = self.match_primary(context, data, &mut primary_relations);
 
         // Cascade properties and compute primary values.
-        self.cascade_primary(context, data);
+        self.cascade_primary(context, data, result.important_rules_overriding_animation_changed);
 
         // Match and cascade eager pseudo-elements.
         if !data.styles().is_display_none() {
-            let _pseudo_rule_nodes_changed =
-                self.match_pseudos(context, data);
+            let _pseudo_rule_nodes_changed = self.match_pseudos(context, data);
             self.cascade_pseudos(context, data);
         }
 
         // If we have any pseudo elements, indicate so in the primary StyleRelations.
         if !data.styles().pseudos.is_empty() {
             primary_relations |= AFFECTED_BY_PSEUDO_ELEMENTS;
         }
 
@@ -933,30 +972,32 @@ pub trait MatchMethods : TElement {
                                        primary_relations,
                                        revalidation_match_results);
         }
     }
 
     /// Performs the cascade, without matching.
     fn cascade_primary_and_pseudos(&self,
                                    context: &mut StyleContext<Self>,
-                                   mut data: &mut ElementData)
+                                   mut data: &mut ElementData,
+                                   important_rules_changed: bool)
     {
-        self.cascade_primary(context, &mut data);
+        self.cascade_primary(context, &mut data, important_rules_changed);
         self.cascade_pseudos(context, &mut data);
     }
 
     /// Runs selector matching to (re)compute the primary rule node for this element.
     ///
-    /// Returns whether the primary rule node changed.
+    /// Returns whether the primary rule node changed and whether the change includes important
+    /// rules.
     fn match_primary(&self,
                      context: &mut StyleContext<Self>,
                      data: &mut ElementData,
                      relations: &mut StyleRelations)
-                     -> bool
+                     -> RulesMatchedResult
     {
         let implemented_pseudo = self.implemented_pseudo_element();
         if let Some(ref pseudo) = implemented_pseudo {
             if pseudo.is_eager() {
                 // If it's an eager element-backed pseudo, just grab the matched
                 // rules from the parent, and update animations.
                 let parent = self.parent_element().unwrap();
                 let parent_data = parent.borrow_data().unwrap();
@@ -985,17 +1026,20 @@ pub trait MatchMethods : TElement {
                                                   Some(&animation_rule),
                                                   &mut rules,
                                                   &context.shared.guards);
                     if let Some(node) = animation_rule_node {
                         rules = node;
                     }
                 }
 
-                return data.set_primary_rules(rules);
+                return RulesMatchedResult {
+                    rule_nodes_changed: data.set_primary_rules(rules),
+                    important_rules_overriding_animation_changed: false,
+                };
             }
         }
 
         let mut applicable_declarations =
             Vec::<ApplicableDeclarationBlock>::with_capacity(16);
 
         let stylist = &context.shared.stylist;
         let style_attribute = self.style_attribute();
@@ -1026,17 +1070,25 @@ pub trait MatchMethods : TElement {
                                                           &mut applicable_declarations,
                                                           &mut set_selector_flags);
 
         let primary_rule_node =
             compute_rule_node::<Self>(&stylist.rule_tree,
                                       &mut applicable_declarations,
                                       &context.shared.guards);
 
-        return data.set_primary_rules(primary_rule_node);
+        let important_rules_changed = self.has_animations() &&
+                                      data.has_styles() &&
+                                      data.important_rules_are_different(&primary_rule_node,
+                                                                         &context.shared.guards);
+
+        RulesMatchedResult {
+            rule_nodes_changed: data.set_primary_rules(primary_rule_node),
+            important_rules_overriding_animation_changed: important_rules_changed,
+        }
     }
 
     /// Runs selector matching to (re)compute eager pseudo-element rule nodes for this
     /// element.
     ///
     /// Returns whether any of the pseudo rule nodes changed (including, but not
     /// limited to, cases where we match different pseudos altogether).
     fn match_pseudos(&self,
@@ -1163,38 +1215,43 @@ pub trait MatchMethods : TElement {
                 if !p.has_selector_flags(parent_flags) {
                     map.insert_flags(p, parent_flags);
                 }
             }
         }
     }
 
     /// Updates the rule nodes without re-running selector matching, using just
-    /// the rule tree. Returns true if the rule nodes changed.
+    /// the rule tree. Returns two booleans, the first true if the rule nodes changed,
+    /// and second true if the important rules changed.
     fn replace_rules(&self,
                      hint: RestyleHint,
                      context: &StyleContext<Self>,
                      data: &mut AtomicRefMut<ElementData>)
-                     -> bool {
+                     -> RulesChangedFlags {
         use properties::PropertyDeclarationBlock;
         use shared_lock::Locked;
 
         let element_styles = &mut data.styles_mut();
         let primary_rules = &mut element_styles.primary.rules;
-        let mut rule_node_changed = false;
+        let mut result: RulesChangedFlags = RulesChangedFlags::empty();
 
         {
             let mut replace_rule_node = |level: CascadeLevel,
                                          pdb: Option<&Arc<Locked<PropertyDeclarationBlock>>>,
                                          path: &mut StrongRuleNode| {
                 let new_node = context.shared.stylist.rule_tree
                     .update_rule_at_level(level, pdb, path, &context.shared.guards);
                 if let Some(n) = new_node {
                     *path = n;
-                    rule_node_changed = true;
+                    if level.is_important() {
+                        result.insert(IMPORTANT_RULES_CHANGED);
+                    } else {
+                        result.insert(NORMAL_RULES_CHANGED);
+                    }
                 }
             };
 
             // Animation restyle hints are processed prior to other restyle
             // hints in the animation-only traversal.
             //
             // Non-animation restyle hints will be processed in a subsequent
             // normal traversal.
@@ -1232,17 +1289,17 @@ pub trait MatchMethods : TElement {
                                   style_attribute,
                                   primary_rules);
                 replace_rule_node(CascadeLevel::StyleAttributeImportant,
                                   style_attribute,
                                   primary_rules);
             }
         }
 
-        rule_node_changed
+        result
     }
 
     /// Attempts to share a style with another node. This method is unsafe
     /// because it depends on the `style_sharing_candidate_cache` having only
     /// live nodes in it, and we have no way to guarantee that at the type
     /// system level yet.
     unsafe fn share_style_if_possible(&self,
                                       context: &mut StyleContext<Self>,
@@ -1415,34 +1472,35 @@ pub trait MatchMethods : TElement {
                 }
             }
         }
     }
 
     /// Performs the cascade for the element's primary style.
     fn cascade_primary(&self,
                        context: &mut StyleContext<Self>,
-                       mut data: &mut ElementData)
+                       mut data: &mut ElementData,
+                       important_rules_changed: bool)
     {
-        self.cascade_primary_or_pseudo(context, &mut data, None);
+        self.cascade_primary_or_pseudo(context, &mut data, None, important_rules_changed);
     }
 
     /// Performs the cascade for the element's eager pseudos.
     fn cascade_pseudos(&self,
                        context: &mut StyleContext<Self>,
                        mut data: &mut ElementData)
     {
         // Note that we've already set up the map of matching pseudo-elements
         // in match_pseudos (and handled the damage implications of changing
         // which pseudos match), so now we can just iterate what we have. This
         // does mean collecting owned pseudos, so that the borrow checker will
         // let us pass the mutable |data| to the cascade function.
         let matched_pseudos = data.styles().pseudos.keys();
         for pseudo in matched_pseudos {
-            self.cascade_primary_or_pseudo(context, data, Some(&pseudo));
+            self.cascade_primary_or_pseudo(context, data, Some(&pseudo), false);
         }
     }
 
     /// Returns computed values without animation and transition rules.
     fn get_base_style(&self,
                       shared_context: &SharedStyleContext,
                       font_metrics_provider: &FontMetricsProvider,
                       primary_style: &ComputedStyle,
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -213,17 +213,17 @@ pub mod shorthands {
 ///
 /// This needs to be "included" by mako at least after all longhand modules,
 /// given they populate the global data.
 pub mod animated_properties {
     <%include file="/helpers/animated_properties.mako.rs" />
 }
 
 /// A set of longhand properties
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 pub struct LonghandIdSet {
     storage: [u32; (${len(data.longhands)} - 1 + 32) / 32]
 }
 
 impl LonghandIdSet {
     /// Create an empty set
     #[inline]
     pub fn new() -> LonghandIdSet {
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -742,22 +742,22 @@ fn compute_style<E, D>(_traversal: &D,
 
             context.thread_local.bloom_filter.assert_complete(element);
             context.thread_local.statistics.elements_matched += 1;
 
             // Perform the matching and cascading.
             element.match_and_cascade(context, &mut data, StyleSharingBehavior::Allow);
         }
         CascadeWithReplacements(hint) => {
-            let _rule_nodes_changed =
-                element.replace_rules(hint, context, &mut data);
-            element.cascade_primary_and_pseudos(context, &mut data);
+            let rules_changed = element.replace_rules(hint, context, &mut data);
+            element.cascade_primary_and_pseudos(context, &mut data,
+                                                rules_changed.important_rules_changed());
         }
         CascadeOnly => {
-            element.cascade_primary_and_pseudos(context, &mut data);
+            element.cascade_primary_and_pseudos(context, &mut data, false);
         }
     };
 }
 
 fn preprocess_children<E, D>(traversal: &D,
                              element: E,
                              mut propagated_hint: StoredRestyleHint,
                              damage_handled: RestyleDamage,