Bug 1361843 part 1. Split up Stylist's update() method into separate clear() and rebuild() methods. r?emilio draft
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 10 May 2017 12:09:43 -0400
changeset 575549 1b1754312288ed3da880686c72891c11eda3cf79
parent 575133 120d8562d4a53e4f78bd86c6f5076f6db265e5a3
child 575550 85870882035698a7e48954c23e376684d3c5269c
push id58095
push userbzbarsky@mozilla.com
push dateWed, 10 May 2017 16:11:09 +0000
reviewersemilio
bugs1361843
milestone55.0a1
Bug 1361843 part 1. Split up Stylist's update() method into separate clear() and rebuild() methods. r?emilio Note that we used to both reset self.animations to Default::default() and then also clear() it. I stuck with the clear() call, but maybe resetting to Default::default() is better. MozReview-Commit-ID: 7v2TdyFZCCF
servo/components/style/stylist.rs
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -80,16 +80,20 @@ pub struct Stylist {
     viewport_constraints: Option<ViewportConstraints>,
 
     /// If true, the quirks-mode stylesheet is applied.
     quirks_mode: QuirksMode,
 
     /// If true, the device has changed, and the stylist needs to be updated.
     is_device_dirty: bool,
 
+    /// If true, the stylist is in a cleared state (e.g. just-constructed, or
+    /// had clear() called on it with no following rebuild()).
+    is_cleared: bool,
+
     /// The current selector maps, after evaluating media
     /// rules against the current device.
     element_map: PerPseudoElementSelectorMap,
 
     /// The rule tree, that stores the results of selector matching.
     ///
     /// FIXME(emilio): Not `pub`!
     pub rule_tree: RuleTree,
@@ -158,23 +162,25 @@ impl<'a> ExtraStyleData<'a> {
 
 #[cfg(feature = "servo")]
 impl<'a> ExtraStyleData<'a> {
     fn clear_font_faces(&mut self) {}
     fn add_font_face(&mut self, _: &Arc<Locked<FontFaceRule>>, _: Origin) {}
 }
 
 impl Stylist {
-    /// Construct a new `Stylist`, using a given `Device`.
+    /// Construct a new `Stylist`, using a given `Device`.  If more members are
+    /// added here, think about whether they should be reset in clear().
     #[inline]
     pub fn new(device: Device) -> Self {
         let mut stylist = Stylist {
             viewport_constraints: None,
             device: Arc::new(device),
             is_device_dirty: true,
+            is_cleared: true,
             quirks_mode: QuirksMode::NoQuirks,
 
             element_map: PerPseudoElementSelectorMap::new(),
             pseudos_map: Default::default(),
             animations: Default::default(),
             precomputed_pseudo_element_decls: Default::default(),
             rules_source_order: 0,
             rule_tree: RuleTree::new(),
@@ -214,63 +220,91 @@ impl Stylist {
         self.dependencies.len()
     }
 
     /// Returns the number of revalidation_selectors.
     pub fn num_revalidation_selectors(&self) -> usize {
         self.selectors_for_cache_revalidation.len()
     }
 
-    /// Update the stylist for the given document stylesheets, and optionally
+    /// Clear the stylist's state, effectively resetting it to more or less
+    /// the state Stylist::new creates.
+    ///
+    /// We preserve the state of the following members:
+    ///   device: Someone might have set this on us.
+    ///   quirks_mode: Again, someone might have set this on us.
+    ///   num_rebuilds: clear() followed by rebuild() should just increment this
+    ///
+    /// We don't just use struct update syntax with Stylist::new(self.device)
+    /// beause for some of our members we can clear them instead of creating new
+    /// objects.  This does cause unfortunate code duplication with
+    /// Stylist::new.
+    pub fn clear(&mut self) {
+        if self.is_cleared {
+            return
+        }
+
+        self.viewport_constraints = None;
+        // preserve current device
+        self.is_device_dirty = true;
+        self.is_cleared = true;
+        // preserve current quirks_mode value
+        self.element_map = PerPseudoElementSelectorMap::new();
+        self.pseudos_map = Default::default();
+        self.animations.clear(); // Or set to Default::default()?
+        self.precomputed_pseudo_element_decls = Default::default();
+        self.rules_source_order = 0;
+        // We want to keep rule_tree around across stylist rebuilds.
+        self.dependencies.clear();
+        self.selectors_for_cache_revalidation = SelectorMap::new();
+        self.num_selectors = 0;
+        self.num_declarations = 0;
+        // preserve num_rebuilds value, since it should stay across
+        // clear()/rebuild() cycles.
+    }
+
+    /// rebuild the stylist for the given document stylesheets, and optionally
     /// with a set of user agent stylesheets.
     ///
     /// This method resets all the style data each time the stylesheets change
     /// (which is indicated by the `stylesheets_changed` parameter), or the
     /// device is dirty, which means we need to re-evaluate media queries.
-    pub fn update<'a>(&mut self,
+    pub fn rebuild<'a>(&mut self,
                       doc_stylesheets: &[Arc<Stylesheet>],
                       guards: &StylesheetGuards,
                       ua_stylesheets: Option<&UserAgentStylesheets>,
                       stylesheets_changed: bool,
                       author_style_disabled: bool,
                       extra_data: &mut ExtraStyleData<'a>) -> bool {
+        self.is_cleared = false;
+
         if !(self.is_device_dirty || stylesheets_changed) {
             return false;
         }
+
         self.num_rebuilds += 1;
 
         let cascaded_rule = ViewportRule {
             declarations: viewport::Cascade::from_stylesheets(
                 doc_stylesheets, guards.author, &self.device
             ).finish(),
         };
 
         self.viewport_constraints =
             ViewportConstraints::maybe_new(&self.device, &cascaded_rule, self.quirks_mode);
 
         if let Some(ref constraints) = self.viewport_constraints {
             Arc::get_mut(&mut self.device).unwrap()
                 .account_for_viewport_rule(constraints);
         }
 
-        self.element_map = PerPseudoElementSelectorMap::new();
-        self.pseudos_map = Default::default();
-        self.animations = Default::default();
         SelectorImpl::each_eagerly_cascaded_pseudo_element(|pseudo| {
             self.pseudos_map.insert(pseudo, PerPseudoElementSelectorMap::new());
         });
 
-        self.precomputed_pseudo_element_decls = Default::default();
-        self.rules_source_order = 0;
-        self.dependencies.clear();
-        self.animations.clear();
-        self.selectors_for_cache_revalidation = SelectorMap::new();
-        self.num_selectors = 0;
-        self.num_declarations = 0;
-
         extra_data.clear_font_faces();
 
         if let Some(ua_stylesheets) = ua_stylesheets {
             for stylesheet in &ua_stylesheets.user_or_user_agent_stylesheets {
                 self.add_stylesheet(&stylesheet, guards.ua_or_user, extra_data);
             }
 
             if self.quirks_mode != QuirksMode::NoQuirks {
@@ -297,16 +331,35 @@ impl Stylist {
                 self.precomputed_pseudo_element_decls.insert(pseudo, declarations);
             }
         });
 
         self.is_device_dirty = false;
         true
     }
 
+    /// clear the stylist and then rebuild it.  Chances are, you want to use
+    /// either clear() or rebuild(), with the latter done lazily, instead.
+    pub fn update<'a>(&mut self,
+                      doc_stylesheets: &[Arc<Stylesheet>],
+                      guards: &StylesheetGuards,
+                      ua_stylesheets: Option<&UserAgentStylesheets>,
+                      stylesheets_changed: bool,
+                      author_style_disabled: bool,
+                      extra_data: &mut ExtraStyleData<'a>) -> bool {
+        // We have to do a dirtiness check before clearing, because if
+        // we're not actually dirty we need to no-op here.
+        if !(self.is_device_dirty || stylesheets_changed) {
+            return false;
+        }
+        self.clear();
+        self.rebuild(doc_stylesheets, guards, ua_stylesheets, stylesheets_changed,
+                     author_style_disabled, extra_data)
+    }
+
     fn add_stylesheet<'a>(&mut self, stylesheet: &Stylesheet, guard: &SharedRwLockReadGuard,
                           extra_data: &mut ExtraStyleData<'a>) {
         if stylesheet.disabled() || !stylesheet.is_effective_for_device(&self.device, guard) {
             return;
         }
 
         // Cheap `Arc` clone so that the closure below can borrow `&mut Stylist`.
         let device = self.device.clone();