stylo: Cleanup a bit of the Stylist clear setup.

This moves us to clear on rebuild, which allows us to remove yet another place
where we track stylist dirtiness.

Bug: 1390255
Reviewed-by: heycam
MozReview-Commit-ID: nihQbUAbh8
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>

components/style/gecko/data.rs

@@ -17,7 +17,7 @@ use properties::ComputedValues;
 use servo_arc::Arc;
 use shared_lock::{Locked, StylesheetGuards, SharedRwLockReadGuard};
 use stylesheet_set::StylesheetSet;
-use stylesheets::{Origin, PerOrigin, StylesheetContents, StylesheetInDocument};
+use stylesheets::{PerOrigin, StylesheetContents, StylesheetInDocument};
 use stylist::{ExtraStyleData, Stylist};
 
 /// Little wrapper to a Gecko style sheet.
@@ -164,14 +164,14 @@ impl PerDocumentStyleDataImpl {
 
         let author_style_disabled = self.stylesheets.author_style_disabled();
 
-        let iter = self.stylesheets.flush(document_element);
+        let (iter, dirty_origins) = self.stylesheets.flush(document_element);
         self.stylist.rebuild(
             iter,
             &StylesheetGuards::same(guard),
             /* ua_sheets = */ None,
-            /* stylesheets_changed = */ true,
             author_style_disabled,
             &mut self.extra_style_data,
+            dirty_origins,
         );
     }
 
@@ -187,17 +187,6 @@ impl PerDocumentStyleDataImpl {
         self.stylist.device().default_computed_values_arc()
     }
 
-    /// Clear the stylist.  This will be a no-op if the stylist is
-    /// already cleared; the stylist handles that.
-    pub fn clear_stylist(&mut self) {
-        self.stylist.clear();
-    }
-
-    /// Clear the stylist's data for the specified origin.
-    pub fn clear_stylist_origin(&mut self, origin: &Origin) {
-        self.stylist.clear_origin(origin);
-    }
-
     /// Returns whether visited links are enabled.
     fn visited_links_enabled(&self) -> bool {
         unsafe { bindings::Gecko_AreVisitedLinksEnabled() }

components/style/gecko/generated/bindings.rs

@@ -1987,9 +1987,6 @@ extern "C" {
     pub fn Servo_StyleSet_Init(pres_context: RawGeckoPresContextOwned)
      -> *mut RawServoStyleSet;
 }
-extern "C" {
-    pub fn Servo_StyleSet_Clear(set: RawServoStyleSetBorrowed);
-}
 extern "C" {
     pub fn Servo_StyleSet_RebuildCachedData(set: RawServoStyleSetBorrowed);
 }

components/style/invalidation/element/invalidation_map.rs

@@ -195,8 +195,8 @@ impl InvalidationMap {
     pub fn clear(&mut self) {
         self.class_to_selector.clear();
         self.id_to_selector.clear();
-        self.state_affecting_selectors = SelectorMap::new();
+        self.state_affecting_selectors.clear();
-        self.other_attribute_affecting_selectors = SelectorMap::new();
+        self.other_attribute_affecting_selectors.clear();
         self.has_id_attribute_selectors = false;
         self.has_class_attribute_selectors = false;
     }

components/style/selector_map.rs

@@ -123,6 +123,15 @@ impl<T> SelectorMap<T> {
         }
     }
 
+    /// Clears the hashmap retaining storage.
+    pub fn clear(&mut self) {
+        self.id_hash.clear();
+        self.class_hash.clear();
+        self.local_name_hash.clear();
+        self.other.clear();
+        self.count = 0;
+    }
+
     /// Returns whether there are any entries in the map.
     pub fn is_empty(&self) -> bool {
         self.count == 0

components/style/stylesheet_set.rs

@@ -8,7 +8,7 @@ use dom::TElement;
 use invalidation::stylesheets::StylesheetInvalidationSet;
 use shared_lock::SharedRwLockReadGuard;
 use std::slice;
-use stylesheets::{Origin, PerOrigin, StylesheetInDocument};
+use stylesheets::{OriginSet, PerOrigin, StylesheetInDocument};
 use stylist::Stylist;
 
 /// Entry for a StylesheetSet. We don't bother creating a constructor, because
@@ -169,21 +169,23 @@ where
     pub fn flush<E>(
         &mut self,
         document_element: Option<E>,
-    ) -> StylesheetIterator<S>
+    ) -> (StylesheetIterator<S>, OriginSet)
     where
         E: TElement,
     {
         debug!("StylesheetSet::flush");
         debug_assert!(self.has_changed());
 
-        for (data, _) in self.invalidation_data.iter_mut_origins() {
+        let mut origins = OriginSet::empty();
+        for (data, origin) in self.invalidation_data.iter_mut_origins() {
             if data.dirty {
                 data.invalidations.flush(document_element);
                 data.dirty = false;
+                origins |= origin;
             }
         }
 
-        self.iter()
+        (self.iter(), origins)
     }
 
     /// Returns an iterator over the current list of stylesheets.
@@ -191,24 +193,15 @@ where
         StylesheetIterator(self.entries.iter())
     }
 
-    /// Mark the stylesheets as dirty, because something external may have
+    /// Mark the stylesheets for the specified origin as dirty, because
-    /// invalidated it.
+    /// something external may have invalidated it.
-    ///
+    pub fn force_dirty(&mut self, origins: OriginSet) {
-    /// FIXME(emilio): Make this more granular.
+        for origin in origins.iter() {
-    pub fn force_dirty(&mut self) {
+            let data = self.invalidation_data.borrow_mut_for_origin(&origin);
-        for (data, _) in self.invalidation_data.iter_mut_origins() {
             data.invalidations.invalidate_fully();
             data.dirty = true;
         }
     }
-
-    /// Mark the stylesheets for the specified origin as dirty, because
-    /// something external may have invalidated it.
-    pub fn force_dirty_origin(&mut self, origin: &Origin) {
-        let data = self.invalidation_data.borrow_mut_for_origin(origin);
-        data.invalidations.invalidate_fully();
-        data.dirty = true;
-    }
 }
 
 struct InvalidationData {

components/style/stylesheets/mod.rs

@@ -44,7 +44,7 @@ pub use self::memory::{MallocSizeOf, MallocSizeOfFn, MallocSizeOfWithGuard};
 #[cfg(feature = "gecko")]
 pub use self::memory::{MallocSizeOfWithRepeats, SizeOfState};
 pub use self::namespace_rule::NamespaceRule;
-pub use self::origin::{Origin, OriginSet, PerOrigin, PerOriginClear};
+pub use self::origin::{Origin, OriginSet, PerOrigin};
 pub use self::page_rule::PageRule;
 pub use self::rule_parser::{State, TopLevelRuleParser};
 pub use self::rule_list::{CssRules, CssRulesHelpers};

components/style/stylesheets/origin.rs

@@ -168,20 +168,6 @@ impl<T> PerOrigin<T> {
     }
 }
 
-/// An object that can be cleared.
-pub trait PerOriginClear {
-    /// Clears the object.
-    fn clear(&mut self);
-}
-
-impl<T> PerOriginClear for PerOrigin<T> where T: PerOriginClear {
-    fn clear(&mut self) {
-        self.user_agent.clear();
-        self.user.clear();
-        self.author.clear();
-    }
-}
-
 /// Iterator over `PerOrigin<T>`, from highest level (author) to lowest
 /// (user agent).
 ///

components/style/stylist.rs

@@ -40,7 +40,7 @@ use style_traits::viewport::ViewportConstraints;
 #[cfg(feature = "gecko")]
 use stylesheets::{CounterStyleRule, FontFaceRule};
 use stylesheets::{CssRule, StyleRule};
-use stylesheets::{StylesheetInDocument, Origin, OriginSet, PerOrigin, PerOriginClear};
+use stylesheets::{StylesheetInDocument, Origin, OriginSet, PerOrigin};
 use stylesheets::UserAgentStylesheets;
 use stylesheets::keyframes_rule::KeyframesAnimation;
 use stylesheets::viewport_rule::{self, MaybeNew, ViewportRule};
@@ -182,52 +182,6 @@ impl Stylist {
         }
     }
 
-    /// 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
-    ///   rule_tree: So we can re-use rule nodes across rebuilds.
-    ///
-    /// 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) {
-        self.cascade_data.clear();
-        self.precomputed_pseudo_element_decls.clear();
-        self.viewport_constraints = None;
-
-        // XXX(heycam) Why do this, if we are preserving the Device?
-        self.is_device_dirty = true;
-    }
-
-    /// Clear the stylist's state for the specified origin.
-    pub fn clear_origin(&mut self, origin: &Origin) {
-        self.cascade_data.borrow_mut_for_origin(origin).clear();
-
-        if *origin == Origin::UserAgent {
-            // We only collect these declarations from UA sheets.
-            self.precomputed_pseudo_element_decls.clear();
-        }
-
-        // The stored `ViewportConstraints` contains data from rules across
-        // all origins.
-        self.viewport_constraints = None;
-
-        // XXX(heycam) Why do this, if we are preserving the Device?
-        self.is_device_dirty = true;
-    }
-
-    /// Returns whether any origin's `CascadeData` has been cleared.
-    fn any_origin_cleared(&self) -> bool {
-        self.cascade_data
-            .iter_origins()
-            .any(|(d, _)| d.is_cleared)
-    }
-
     /// Rebuild the stylist for the given document stylesheets, and optionally
     /// with a set of user agent stylesheets.
     ///
@@ -239,40 +193,17 @@ impl Stylist {
         doc_stylesheets: I,
         guards: &StylesheetGuards,
         ua_stylesheets: Option<&UserAgentStylesheets>,
-        stylesheets_changed: bool,
         author_style_disabled: bool,
-        extra_data: &mut PerOrigin<ExtraStyleData>
+        extra_data: &mut PerOrigin<ExtraStyleData>,
+        mut origins_to_rebuild: OriginSet,
     ) -> bool
     where
         I: Iterator<Item = &'a S> + Clone,
         S: StylesheetInDocument + ToMediaListKey + 'static,
     {
-        debug_assert!(!self.any_origin_cleared() || self.is_device_dirty);
+        debug_assert!(!origins_to_rebuild.is_empty() || self.is_device_dirty);
-
+        if self.is_device_dirty {
-        // Determine the origins that actually need updating.
+            origins_to_rebuild = OriginSet::all();
-        //
-        // XXX(heycam): What is the relationship between `stylesheets_changed`
-        // and the `is_cleared` fields on each origin's `CascadeData`?  Can
-        // we avoid passing in `stylesheets_changed`?
-        let mut to_update: PerOrigin<bool> = Default::default();
-
-        // If we're provided with a list of UA and user style sheets, then
-        // we must update those cascade levels. (Servo does this, but Gecko
-        // just includes the UA and User sheets in `doc_stylesheets`.)
-        if ua_stylesheets.is_some() {
-            to_update.user_agent = true;
-            to_update.user = true;
-        }
-
-        for (data, origin) in self.cascade_data.iter_mut_origins() {
-            if data.is_cleared {
-                data.is_cleared = false;
-                *to_update.borrow_mut_for_origin(&origin) = true;
-            }
-        }
-
-        if !(self.is_device_dirty || stylesheets_changed) {
-            return false;
         }
 
         self.num_rebuilds += 1;
@@ -307,13 +238,13 @@ impl Stylist {
             }
         }
 
-        // XXX(heycam): We should probably just move the `extra_data` to be
+        for origin in origins_to_rebuild.iter() {
-        // stored on the `Stylist` instead of Gecko's `PerDocumentStyleData`.
+            extra_data.borrow_mut_for_origin(&origin).clear();
-        // That would let us clear it inside `clear()` and `clear_origin()`.
+            self.cascade_data.borrow_mut_for_origin(&origin).clear();
-        for (update, origin) in to_update.iter_origins() {
+        }
-            if *update {
+
-                extra_data.borrow_mut_for_origin(&origin).clear();
+        if origins_to_rebuild.contains(Origin::UserAgent.into()) {
-            }
+            self.precomputed_pseudo_element_decls.clear();
         }
 
         if let Some(ua_stylesheets) = ua_stylesheets {
@@ -337,11 +268,10 @@ impl Stylist {
         // Only add stylesheets for origins we are updating, and only add
         // Author level sheets if author style is not disabled.
         let sheets_to_add = doc_stylesheets.filter(|s| {
-            match s.contents(guards.author).origin {
+            let sheet_origin = s.contents(guards.author).origin;
-                Origin::UserAgent => to_update.user_agent,
+
-                Origin::Author => to_update.author && !author_style_disabled,
+            origins_to_rebuild.contains(sheet_origin.into()) &&
-                Origin::User => to_update.user,
+                (!matches!(sheet_origin, Origin::Author) || !author_style_disabled)
-            }
         });
 
         for stylesheet in sheets_to_add {
@@ -367,16 +297,19 @@ impl Stylist {
         I: Iterator<Item = &'a S> + Clone,
         S: StylesheetInDocument + ToMediaListKey + 'static,
     {
-        debug_assert!(!self.any_origin_cleared() || self.is_device_dirty);
-
         // 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(
-        self.rebuild(doc_stylesheets, guards, ua_stylesheets, stylesheets_changed,
+            doc_stylesheets,
-                     author_style_disabled, extra_data)
+            guards,
+            ua_stylesheets,
+            author_style_disabled,
+            extra_data,
+            OriginSet::all(),
+        )
     }
 
     fn add_stylesheet<S>(
@@ -529,11 +462,7 @@ impl Stylist {
     pub fn might_have_attribute_dependency(&self,
                                            local_name: &LocalName)
                                            -> bool {
-        if self.any_origin_cleared() || self.is_device_dirty {
+        if *local_name == local_name!("style") {
-            // We can't tell what attributes are in our style rules until
-            // we rebuild.
-            true
-        } else if *local_name == local_name!("style") {
             self.cascade_data
                 .iter_origins()
                 .any(|(d, _)| d.style_attribute_dependency)
@@ -550,13 +479,7 @@ impl Stylist {
     /// Returns whether the given ElementState bit might be relied upon by a
     /// selector of some rule in the stylist.
     pub fn might_have_state_dependency(&self, state: ElementState) -> bool {
-        if self.any_origin_cleared() || self.is_device_dirty {
+        self.has_state_dependency(state)
-            // We can't tell what states our style rules rely on until
-            // we rebuild.
-            true
-        } else {
-            self.has_state_dependency(state)
-        }
     }
 
     /// Returns whether the given ElementState bit is relied upon by a selector
@@ -1440,14 +1363,17 @@ impl ExtraStyleData {
     }
 
     /// Add the given @counter-style rule.
-    fn add_counter_style(&mut self, guard: &SharedRwLockReadGuard,
+    fn add_counter_style(
-                         rule: &Arc<Locked<CounterStyleRule>>) {
+        &mut self,
+        guard: &SharedRwLockReadGuard,
+        rule: &Arc<Locked<CounterStyleRule>>,
+    ) {
         let name = rule.read_with(guard).mName.raw::<nsIAtom>().into();
         self.counter_styles.insert(name, rule.clone());
     }
 }
 
-impl PerOriginClear for ExtraStyleData {
+impl ExtraStyleData {
     fn clear(&mut self) {
         #[cfg(feature = "gecko")]
         {
@@ -1689,11 +1615,6 @@ struct CascadeData {
 
     /// The total number of declarations.
     num_declarations: usize,
-
-    /// If true, the `CascadeData` is in a cleared state (e.g. just-constructed,
-    /// or had `clear()` called on it with no following `rebuild()` on the
-    /// `Stylist`).
-    is_cleared: bool,
 }
 
 impl CascadeData {
@@ -1712,7 +1633,6 @@ impl CascadeData {
             rules_source_order: 0,
             num_selectors: 0,
             num_declarations: 0,
-            is_cleared: true,
         }
     }
 
@@ -1727,28 +1647,21 @@ impl CascadeData {
     fn has_rules_for_pseudo(&self, pseudo: &PseudoElement) -> bool {
         self.pseudos_map.get(pseudo).is_some()
     }
-}
 
-impl PerOriginClear for CascadeData {
     fn clear(&mut self) {
-        if self.is_cleared {
+        self.element_map.clear();
-            return;
+        self.pseudos_map.clear();
-        }
+        self.animations.clear();
-
-        self.element_map = SelectorMap::new();
-        self.pseudos_map = Default::default();
-        self.animations = Default::default();
         self.invalidation_map.clear();
         self.attribute_dependencies.clear();
         self.style_attribute_dependency = false;
         self.state_dependencies = ElementState::empty();
         self.mapped_ids.clear();
-        self.selectors_for_cache_revalidation = SelectorMap::new();
+        self.selectors_for_cache_revalidation.clear();
         self.effective_media_query_results.clear();
         self.rules_source_order = 0;
         self.num_selectors = 0;
         self.num_declarations = 0;
-        self.is_cleared = true;
     }
 }
 

ports/geckolib/glue.rs

@@ -117,7 +117,7 @@ use style::style_adjuster::StyleAdjuster;
 use style::stylesheets::{CssRule, CssRules, CssRuleType, CssRulesHelpers, DocumentRule};
 use style::stylesheets::{FontFeatureValuesRule, ImportRule, KeyframesRule, MallocSizeOfWithGuard};
 use style::stylesheets::{MediaRule, NamespaceRule, Origin, OriginSet, PageRule, SizeOfState, StyleRule};
-use style::stylesheets::{StylesheetContents, StylesheetInDocument, SupportsRule};
+use style::stylesheets::{StylesheetContents, SupportsRule};
 use style::stylesheets::StylesheetLoader as StyleStylesheetLoader;
 use style::stylesheets::keyframes_rule::{Keyframe, KeyframeSelector, KeyframesStepValue};
 use style::stylesheets::supports_rule::parse_condition_or_declaration;
@@ -891,9 +891,7 @@ pub extern "C" fn Servo_StyleSet_AppendStyleSheet(
     let mut data = &mut *data;
     let guard = global_style_data.shared_lock.read();
     let sheet = unsafe { GeckoStyleSheet::new(sheet) };
-    let origin = sheet.contents(&guard).origin;
     data.stylesheets.append_stylesheet(&data.stylist, sheet, &guard);
-    data.clear_stylist_origin(&origin);
 }
 
 #[no_mangle]
@@ -942,9 +940,7 @@ pub extern "C" fn Servo_StyleSet_PrependStyleSheet(
     let mut data = &mut *data;
     let guard = global_style_data.shared_lock.read();
     let sheet = unsafe { GeckoStyleSheet::new(sheet) };
-    let origin = sheet.contents(&guard).origin;
     data.stylesheets.prepend_stylesheet(&data.stylist, sheet, &guard);
-    data.clear_stylist_origin(&origin);
 }
 
 #[no_mangle]
@@ -958,14 +954,12 @@ pub extern "C" fn Servo_StyleSet_InsertStyleSheetBefore(
     let mut data = &mut *data;
     let guard = global_style_data.shared_lock.read();
     let sheet = unsafe { GeckoStyleSheet::new(sheet) };
-    let origin = sheet.contents(&guard).origin;
     data.stylesheets.insert_stylesheet_before(
         &data.stylist,
         sheet,
         unsafe { GeckoStyleSheet::new(before_sheet) },
         &guard,
     );
-    data.clear_stylist_origin(&origin);
 }
 
 #[no_mangle]
@@ -978,9 +972,7 @@ pub extern "C" fn Servo_StyleSet_RemoveStyleSheet(
     let mut data = &mut *data;
     let guard = global_style_data.shared_lock.read();
     let sheet = unsafe { GeckoStyleSheet::new(sheet) };
-    let origin = sheet.contents(&guard).origin;
     data.stylesheets.remove_stylesheet(&data.stylist, sheet, &guard);
-    data.clear_stylist_origin(&origin);
 }
 
 #[no_mangle]
@@ -1002,10 +994,7 @@ pub extern "C" fn Servo_StyleSet_NoteStyleSheetsChanged(
     changed_origins: OriginFlags,
 ) {
     let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
-    for origin in OriginSet::from(changed_origins).iter() {
+    data.stylesheets.force_dirty(OriginSet::from(changed_origins));
-        data.stylesheets.force_dirty_origin(&origin);
-        data.clear_stylist_origin(&origin);
-    }
     data.stylesheets.set_author_style_disabled(author_style_disabled);
 }
 
@@ -1939,12 +1928,6 @@ pub extern "C" fn Servo_StyleSet_RebuildCachedData(raw_data: RawServoStyleSetBor
     data.stylist.device_mut().rebuild_cached_data();
 }
 
-#[no_mangle]
-pub extern "C" fn Servo_StyleSet_Clear(raw_data: RawServoStyleSetBorrowed) {
-    let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
-    data.clear_stylist();
-}
-
 #[no_mangle]
 pub extern "C" fn Servo_StyleSet_Drop(data: RawServoStyleSetOwned) {
     let _ = data.into_box::<PerDocumentStyleData>();