Make RuleTree::root return a reference instead of a strong pointer. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 09 Jul 2017 21:02:21 +0200
changeset 606838 cd8e3f97bbde3df4248cbb566007f30dba7bb567
parent 606837 753aad344fb9c98759869438ef36efb797d81500
child 606839 08f3e04e3506e25a732d9c03aa01802ebb2ddc69
child 606874 2c453d62c33b8c41b0ac117f7801d3cb44f1240a
push id67812
push userbmo:emilio+bugs@crisal.io
push dateTue, 11 Jul 2017 13:56:11 +0000
milestone56.0a1
Make RuleTree::root return a reference instead of a strong pointer. There's no reason it wasn't done before. MozReview-Commit-ID: G0lMLWmfHbS
servo/components/style/rule_tree/mod.rs
servo/components/style/stylist.rs
--- a/servo/components/style/rule_tree/mod.rs
+++ b/servo/components/style/rule_tree/mod.rs
@@ -145,18 +145,18 @@ impl RuleTree {
     /// Construct a new rule tree.
     pub fn new() -> Self {
         RuleTree {
             root: StrongRuleNode::new(Box::new(RuleNode::root())),
         }
     }
 
     /// Get the root rule node.
-    pub fn root(&self) -> StrongRuleNode {
-        self.root.clone()
+    pub fn root(&self) -> &StrongRuleNode {
+        &self.root
     }
 
     fn dump<W: Write>(&self, guards: &StylesheetGuards, writer: &mut W) {
         let _ = writeln!(writer, " + RuleTree");
         self.root.get().dump(guards, writer, 0);
     }
 
     /// Dump the rule tree to stdout.
@@ -166,21 +166,23 @@ impl RuleTree {
     }
 
     /// Inserts the given rules, that must be in proper order by specifity, and
     /// returns the corresponding rule node representing the last inserted one.
     ///
     /// !important rules are detected and inserted into the appropriate position
     /// in the rule tree. This allows selector matching to ignore importance,
     /// while still maintaining the appropriate cascade order in the rule tree.
-    pub fn insert_ordered_rules_with_important<'a, I>(&self,
-                                                      iter: I,
-                                                      guards: &StylesheetGuards)
-                                                      -> StrongRuleNode
-        where I: Iterator<Item=(StyleSource, CascadeLevel)>,
+    pub fn insert_ordered_rules_with_important<'a, I>(
+        &self,
+        iter: I,
+        guards: &StylesheetGuards
+    ) -> StrongRuleNode
+    where
+        I: Iterator<Item=(StyleSource, CascadeLevel)>,
     {
         use self::CascadeLevel::*;
         let mut current = self.root.clone();
         let mut last_level = current.get().level;
 
         let mut found_important = false;
         let mut important_style_attr = None;
         let mut important_author = SmallVec::<[StyleSource; 4]>::new();
@@ -252,21 +254,21 @@ impl RuleTree {
             current = current.ensure_child(self.root.downgrade(), source, Transitions);
         }
 
         current
     }
 
     /// Given a list of applicable declarations, insert the rules and return the
     /// corresponding rule node.
-    pub fn compute_rule_node(&self,
-                             applicable_declarations: &mut ApplicableDeclarationList,
-                             guards: &StylesheetGuards)
-                             -> StrongRuleNode
-    {
+    pub fn compute_rule_node(
+        &self,
+        applicable_declarations: &mut ApplicableDeclarationList,
+        guards: &StylesheetGuards
+    ) -> StrongRuleNode {
         let rules = applicable_declarations.drain().map(|d| d.order_and_level());
         let rule_node = self.insert_ordered_rules_with_important(rules, guards);
         rule_node
     }
 
     /// Insert the given rules, that must be in proper order by specifity, and
     /// return the corresponding rule node representing the last inserted one.
     pub fn insert_ordered_rules<'a, I>(&self, iter: I) -> StrongRuleNode
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -16,17 +16,17 @@ use gecko_bindings::structs::{nsIAtom, S
 use invalidation::element::invalidation_map::InvalidationMap;
 use invalidation::media_queries::{EffectiveMediaQueryResults, ToMediaListKey};
 use matching::CascadeVisitedMode;
 use media_queries::Device;
 use properties::{self, CascadeFlags, ComputedValues};
 use properties::{AnimationRules, PropertyDeclarationBlock};
 #[cfg(feature = "servo")]
 use properties::INHERIT_ALL;
-use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
+use rule_tree::{CascadeLevel, RuleTree, StyleSource};
 use selector_map::{SelectorMap, SelectorMapEntry};
 use selector_parser::{SelectorImpl, PseudoElement};
 use selectors::attr::NamespaceConstraint;
 use selectors::bloom::BloomFilter;
 use selectors::matching::{ElementSelectorFlags, matches_selector, MatchingContext, MatchingMode};
 use selectors::matching::{VisitedHandlingMode, AFFECTED_BY_PRESENTATIONAL_HINTS};
 use selectors::parser::{AncestorHashes, Combinator, Component, Selector, SelectorAndHashes};
 use selectors::parser::{SelectorIter, SelectorMethods};
@@ -602,23 +602,22 @@ impl Stylist {
                                          parent: Option<&Arc<ComputedValues>>,
                                          cascade_flags: CascadeFlags,
                                          font_metrics: &FontMetricsProvider)
                                          -> Arc<ComputedValues> {
         debug_assert!(pseudo.is_precomputed());
 
         let rule_node = match self.precomputed_pseudo_element_decls.get(pseudo) {
             Some(declarations) => {
-                // FIXME(emilio): When we've taken rid of the cascade we can just
-                // use into_iter.
                 self.rule_tree.insert_ordered_rules_with_important(
                     declarations.into_iter().map(|a| (a.source.clone(), a.level())),
-                    guards)
+                    guards
+                )
             }
-            None => self.rule_tree.root(),
+            None => self.rule_tree.root().clone(),
         };
 
         // NOTE(emilio): We skip calculating the proper layout parent style
         // here.
         //
         // It'd be fine to assert that this isn't called with a parent style
         // where display contents is in effect, but in practice this is hard to
         // do for stuff like :-moz-fieldset-content with a
@@ -752,23 +751,17 @@ impl Stylist {
 
             Some(Arc::new(computed))
         } else {
             None
         };
 
         // We may not have non-visited rules, if we only had visited ones.  In
         // that case we want to use the root rulenode for our non-visited rules.
-        let root;
-        let rules = if let Some(rules) = inputs.get_rules() {
-            rules
-        } else {
-            root = self.rule_tree.root();
-            &root
-        };
+        let rules = inputs.get_rules().unwrap_or(self.rule_tree.root());
 
         // Read the comment on `precomputed_values_for_pseudo` to see why it's
         // difficult to assert that display: contents nodes never arrive here
         // (tl;dr: It doesn't apply for replaced elements and such, but the
         // computed value is still "contents").
         let computed =
             properties::cascade(&self.device,
                                 rules,
@@ -834,35 +827,35 @@ impl Stylist {
         };
 
         let mut inputs = CascadeInputs::default();
         let mut declarations = ApplicableDeclarationList::new();
         let mut matching_context =
             MatchingContext::new(MatchingMode::ForStatelessPseudoElement,
                                  None,
                                  self.quirks_mode);
-        self.push_applicable_declarations(element,
-                                          Some(&pseudo),
-                                          None,
-                                          None,
-                                          AnimationRules(None, None),
-                                          rule_inclusion,
-                                          &mut declarations,
-                                          &mut matching_context,
-                                          &mut set_selector_flags);
+
+        self.push_applicable_declarations(
+            element,
+            Some(&pseudo),
+            None,
+            None,
+            AnimationRules(None, None),
+            rule_inclusion,
+            &mut declarations,
+            &mut matching_context,
+            &mut set_selector_flags
+        );
 
         if !declarations.is_empty() {
-            let rule_node = self.rule_tree.insert_ordered_rules_with_important(
-                declarations.into_iter().map(|a| a.order_and_level()),
-                guards);
-            if rule_node != self.rule_tree.root() {
-                inputs.set_rules(VisitedHandlingMode::AllLinksUnvisited,
-                                 rule_node);
-            }
-        };
+            let rule_node =
+                self.rule_tree.compute_rule_node(&mut declarations, guards);
+            debug_assert!(rule_node != *self.rule_tree.root());
+            inputs.set_rules(VisitedHandlingMode::AllLinksUnvisited, rule_node);
+        }
 
         if is_probe && !inputs.has_rules() {
             // When probing, don't compute visited styles if we have no
             // unvisited styles.
             return inputs;
         }
 
         if matching_context.relevant_link_found {
@@ -881,17 +874,17 @@ impl Stylist {
                                               &mut declarations,
                                               &mut matching_context,
                                               &mut set_selector_flags);
             if !declarations.is_empty() {
                 let rule_node =
                     self.rule_tree.insert_ordered_rules_with_important(
                         declarations.into_iter().map(|a| a.order_and_level()),
                         guards);
-                if rule_node != self.rule_tree.root() {
+                if rule_node != *self.rule_tree.root() {
                     inputs.set_rules(VisitedHandlingMode::RelevantLinkVisited,
                                      rule_node);
                 }
             }
         }
 
         inputs
     }
@@ -1290,22 +1283,16 @@ impl Stylist {
     }
 
     /// Returns the map of registered `@keyframes` animations.
     #[inline]
     pub fn animations(&self) -> &FnvHashMap<Atom, KeyframesAnimation> {
         &self.animations
     }
 
-    /// Returns the rule root node.
-    #[inline]
-    pub fn rule_tree_root(&self) -> StrongRuleNode {
-        self.rule_tree.root()
-    }
-
     /// Computes the match results of a given element against the set of
     /// revalidation selectors.
     pub fn match_revalidation_selectors<E, F>(&self,
                                               element: &E,
                                               bloom: Option<&BloomFilter>,
                                               flags_setter: &mut F)
                                               -> BitVec
         where E: TElement,