From 4c80cccbd2aa79281a648c8e2f785f05aab8ea0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 14 Aug 2017 16:27:00 +0200 Subject: [PATCH] stylo: Cleanup a bit of the Stylist clear setup. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- components/style/gecko/data.rs | 17 +- components/style/gecko/generated/bindings.rs | 3 - .../invalidation/element/invalidation_map.rs | 4 +- components/style/selector_map.rs | 9 + components/style/stylesheet_set.rs | 29 ++-- components/style/stylesheets/mod.rs | 2 +- components/style/stylesheets/origin.rs | 14 -- components/style/stylist.rs | 161 ++++-------------- ports/geckolib/glue.rs | 21 +-- 9 files changed, 65 insertions(+), 195 deletions(-) diff --git a/components/style/gecko/data.rs b/components/style/gecko/data.rs index 1ac69f2f29e..7d99e4af89f 100644 --- a/components/style/gecko/data.rs +++ b/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() } diff --git a/components/style/gecko/generated/bindings.rs b/components/style/gecko/generated/bindings.rs index 8541250224a..91a2d0f0ae7 100644 --- a/components/style/gecko/generated/bindings.rs +++ b/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); } diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index 81f966a5df3..3b0601a3f1d 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/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.other_attribute_affecting_selectors = SelectorMap::new(); + self.state_affecting_selectors.clear(); + self.other_attribute_affecting_selectors.clear(); self.has_id_attribute_selectors = false; self.has_class_attribute_selectors = false; } diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index a76da79e457..139a2fd95f6 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -123,6 +123,15 @@ impl SelectorMap { } } + /// 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 diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 60f35dd2788..76d6b592a86 100644 --- a/components/style/stylesheet_set.rs +++ b/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( &mut self, document_element: Option, - ) -> StylesheetIterator + ) -> (StylesheetIterator, 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 - /// invalidated it. - /// - /// FIXME(emilio): Make this more granular. - pub fn force_dirty(&mut self) { - for (data, _) in self.invalidation_data.iter_mut_origins() { + /// Mark the stylesheets for the specified origin as dirty, because + /// something external may have invalidated it. + pub fn force_dirty(&mut self, origins: OriginSet) { + for origin in origins.iter() { + let data = self.invalidation_data.borrow_mut_for_origin(&origin); 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 { diff --git a/components/style/stylesheets/mod.rs b/components/style/stylesheets/mod.rs index e52fcf17887..0f87efc2963 100644 --- a/components/style/stylesheets/mod.rs +++ b/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}; diff --git a/components/style/stylesheets/origin.rs b/components/style/stylesheets/origin.rs index 2e178d00e8b..1eb3c4932f1 100644 --- a/components/style/stylesheets/origin.rs +++ b/components/style/stylesheets/origin.rs @@ -168,20 +168,6 @@ impl PerOrigin { } } -/// An object that can be cleared. -pub trait PerOriginClear { - /// Clears the object. - fn clear(&mut self); -} - -impl PerOriginClear for PerOrigin where T: PerOriginClear { - fn clear(&mut self) { - self.user_agent.clear(); - self.user.clear(); - self.author.clear(); - } -} - /// Iterator over `PerOrigin`, from highest level (author) to lowest /// (user agent). /// diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 760f820c967..b0332d71097 100644 --- a/components/style/stylist.rs +++ b/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 + extra_data: &mut PerOrigin, + mut origins_to_rebuild: OriginSet, ) -> bool where I: Iterator + Clone, S: StylesheetInDocument + ToMediaListKey + 'static, { - debug_assert!(!self.any_origin_cleared() || self.is_device_dirty); - - // Determine the origins that actually need updating. - // - // 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 = 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; + debug_assert!(!origins_to_rebuild.is_empty() || self.is_device_dirty); + if self.is_device_dirty { + origins_to_rebuild = OriginSet::all(); } self.num_rebuilds += 1; @@ -307,13 +238,13 @@ impl Stylist { } } - // XXX(heycam): We should probably just move the `extra_data` to be - // stored on the `Stylist` instead of Gecko's `PerDocumentStyleData`. - // That would let us clear it inside `clear()` and `clear_origin()`. - for (update, origin) in to_update.iter_origins() { - if *update { - extra_data.borrow_mut_for_origin(&origin).clear(); - } + for origin in origins_to_rebuild.iter() { + extra_data.borrow_mut_for_origin(&origin).clear(); + self.cascade_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 { - Origin::UserAgent => to_update.user_agent, - Origin::Author => to_update.author && !author_style_disabled, - Origin::User => to_update.user, - } + let sheet_origin = s.contents(guards.author).origin; + + origins_to_rebuild.contains(sheet_origin.into()) && + (!matches!(sheet_origin, Origin::Author) || !author_style_disabled) }); for stylesheet in sheets_to_add { @@ -367,16 +297,19 @@ impl Stylist { I: Iterator + 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(doc_stylesheets, guards, ua_stylesheets, stylesheets_changed, - author_style_disabled, extra_data) + self.rebuild( + doc_stylesheets, + guards, + ua_stylesheets, + author_style_disabled, + extra_data, + OriginSet::all(), + ) } fn add_stylesheet( @@ -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 { - // We can't tell what attributes are in our style rules until - // we rebuild. - true - } else if *local_name == local_name!("style") { + 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 { - // We can't tell what states our style rules rely on until - // we rebuild. - true - } else { - self.has_state_dependency(state) - } + 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, - rule: &Arc>) { + fn add_counter_style( + &mut self, + guard: &SharedRwLockReadGuard, + rule: &Arc>, + ) { let name = rule.read_with(guard).mName.raw::().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 { - return; - } - - self.element_map = SelectorMap::new(); - self.pseudos_map = Default::default(); - self.animations = Default::default(); + self.element_map.clear(); + self.pseudos_map.clear(); + self.animations.clear(); 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; } } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 9c620b35e70..15fea3f5d63 100644 --- a/ports/geckolib/glue.rs +++ b/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_origin(&origin); - data.clear_stylist_origin(&origin); - } + data.stylesheets.force_dirty(OriginSet::from(changed_origins)); 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::();