From 605974a1122d34903e56358127c2d1d71509c143 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 19 May 2017 15:40:44 +0200 Subject: [PATCH 1/8] Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content. r=heycam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: BHSxMJd0G0O Signed-off-by: Emilio Cobos Álvarez --- components/style/matching.rs | 76 +++++++++++++------ components/style/properties/gecko.mako.rs | 11 +++ .../style/properties/properties.mako.rs | 13 ++++ 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 0c4e53cfc4c..9b1711f3d48 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -614,9 +614,21 @@ trait PrivateMatchMethods: TElement { self.cascade_internal(context, &styles.primary, Some(pseudo_style)); if let Some(old) = old_values { - // ::before and ::after are element-backed in Gecko, so they do - // the damage calculation for themselves. - if cfg!(feature = "servo") || !pseudo.is_before_or_after() { + // ::before and ::after are element-backed in Gecko, so they do the + // damage calculation for themselves, when there's an actual pseudo. + // + // NOTE(emilio): An alternative to this check is to just remove the + // pseudo-style entry here if new_values is display: none or has + // an ineffective content property... + // + // I think it's nice to handle it here instead for symmetry with how + // we handle display: none elements, but the other approach may be + // ok too? + let is_unexisting_before_or_after = + pseudo.is_before_or_after() && + self.existing_style_for_restyle_damage(&old, Some(pseudo)).is_none(); + + if cfg!(feature = "servo") || is_unexisting_before_or_after { self.accumulate_damage(&context.shared, restyle.unwrap(), &old, @@ -802,12 +814,12 @@ trait PrivateMatchMethods: TElement { return; } - // Add restyle damage, but only the bits that aren't redundant with respect - // to damage applied on our ancestors. + // Add restyle damage, but only the bits that aren't redundant with + // respect to damage applied on our ancestors. // // See https://bugzilla.mozilla.org/show_bug.cgi?id=1301258#c12 - // for followup work to make the optimization here more optimal by considering - // each bit individually. + // for followup work to make the optimization here more optimal by + // considering each bit individually. if !restyle.damage.contains(RestyleDamage::reconstruct()) { let new_damage = self.compute_restyle_damage(&old_values, &new_values, @@ -1446,23 +1458,41 @@ pub trait MatchMethods : TElement { pseudo: Option<&PseudoElement>) -> RestyleDamage { - match self.existing_style_for_restyle_damage(old_values, pseudo) { - Some(ref source) => RestyleDamage::compute(source, new_values), - None => { - // If there's no style source, that likely means that Gecko - // couldn't find a style context. This happens with display:none - // elements, and probably a number of other edge cases that - // we don't handle well yet (like display:contents). - if new_values.get_box().clone_display() == display::T::none && - old_values.get_box().clone_display() == display::T::none { - // The style remains display:none. No need for damage. - RestyleDamage::empty() - } else { - // Something else. Be conservative for now. - RestyleDamage::reconstruct() - } - } + if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) { + return RestyleDamage::compute(source, new_values); } + + let new_style_is_display_none = + new_values.get_box().clone_display() == display::T::none; + let old_style_is_display_none = + old_values.get_box().clone_display() == display::T::none; + + // If there's no style source, that likely means that Gecko couldn't + // find a style context. + // + // This happens with display:none elements, and not-yet-existing + // pseudo-elements. + if new_style_is_display_none && old_style_is_display_none { + // The style remains display:none. No need for damage. + return RestyleDamage::empty() + } + + if pseudo.map_or(false, |p| p.is_before_or_after()) { + if (old_style_is_display_none || + old_values.ineffective_content_property()) && + (new_style_is_display_none || + new_values.ineffective_content_property()) { + // The pseudo-element will remain undisplayed, so just avoid + // triggering any change. + return RestyleDamage::empty() + } + return RestyleDamage::reconstruct() + } + + // Something else. Be conservative for now. + warn!("Reframing due to lack of old style source: {:?}, pseudo: {:?}", + self, pseudo); + RestyleDamage::reconstruct() } /// Cascade the eager pseudo-elements of this element. diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 8ac9bfd3861..0736d34e97e 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -141,6 +141,13 @@ impl ComputedValues { self.get_box().clone_display() == longhands::display::computed_value::T::contents } + /// Returns true if the value of the `content` property would make a + /// pseudo-element not rendered. + #[inline] + pub fn ineffective_content_property(&self) -> bool { + self.get_counters().ineffective_content_property() + } + % for style_struct in data.style_structs: #[inline] pub fn clone_${style_struct.name_lower}(&self) -> Arc { @@ -4204,6 +4211,10 @@ clip-path <%self:impl_trait style_struct_name="Counters" skip_longhands="content counter-increment counter-reset"> + pub fn ineffective_content_property(&self) -> bool { + self.gecko.mContents.is_empty() + } + pub fn set_content(&mut self, v: longhands::content::computed_value::T) { use properties::longhands::content::computed_value::T; use properties::longhands::content::computed_value::ContentItem; diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 47022d30a6a..0f4ee1b6843 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1813,6 +1813,19 @@ impl ComputedValues { /// Since this isn't supported in Servo, this is always false for Servo. pub fn is_display_contents(&self) -> bool { false } + #[inline] + /// Returns whether the "content" property for the given style is completely + /// ineffective, and would yield an empty `::before` or `::after` + /// pseudo-element. + pub fn ineffective_content_property(&self) -> bool { + use properties::longhands::content::computed_value::T; + match self.get_counters().content { + T::normal | + T::none => true, + T::Content(ref items) => items.is_empty(), + } + } + /// Whether the current style is multicolumn. #[inline] pub fn is_multicol(&self) -> bool { From 715d18d377fd85fb7a5960d6ac7ada72f29a5c05 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Sat, 20 May 2017 11:17:57 +0800 Subject: [PATCH 2/8] style: Add an outparam to Gecko_CalcStyleDifference that returns whether any style data changed. --- components/style/gecko/generated/bindings.rs | 3 ++- components/style/gecko/restyle_damage.rs | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/components/style/gecko/generated/bindings.rs b/components/style/gecko/generated/bindings.rs index a07176ee208..101e9c89cad 100644 --- a/components/style/gecko/generated/bindings.rs +++ b/components/style/gecko/generated/bindings.rs @@ -961,7 +961,8 @@ extern "C" { } extern "C" { pub fn Gecko_CalcStyleDifference(oldstyle: *mut nsStyleContext, - newstyle: ServoComputedValuesBorrowed) + newstyle: ServoComputedValuesBorrowed, + any_style_changed: *mut bool) -> nsChangeHint; } extern "C" { diff --git a/components/style/gecko/restyle_damage.rs b/components/style/gecko/restyle_damage.rs index 39fdafebd5d..7cb66470403 100644 --- a/components/style/gecko/restyle_damage.rs +++ b/components/style/gecko/restyle_damage.rs @@ -49,9 +49,11 @@ impl GeckoRestyleDamage { new_style: &Arc) -> Self { // TODO(emilio): Const-ify this? let context = source as *const nsStyleContext as *mut nsStyleContext; + let mut any_style_changed: bool = false; let hint = unsafe { bindings::Gecko_CalcStyleDifference(context, - new_style.as_borrowed_opt().unwrap()) + new_style.as_borrowed_opt().unwrap(), + &mut any_style_changed) }; GeckoRestyleDamage(hint) } From 8b7a414b1cb61ba7bf4fc42e393abfab0d7bdcf8 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Fri, 19 May 2017 17:39:15 +0800 Subject: [PATCH 3/8] style: Use RestyleDamage to determine whether we must continue cascading style changes to children. --- components/layout/animation.rs | 4 +- components/style/gecko/restyle_damage.rs | 14 +- components/style/matching.rs | 254 +++++++++++++++-------- components/style/servo/restyle_damage.rs | 15 +- components/style/traversal.rs | 43 ++-- 5 files changed, 221 insertions(+), 109 deletions(-) diff --git a/components/layout/animation.rs b/components/layout/animation.rs index d1673ac44a1..a054927190c 100644 --- a/components/layout/animation.rs +++ b/components/layout/animation.rs @@ -160,7 +160,9 @@ pub fn recalc_style_for_animations(context: &LayoutContext, animation, &mut fragment.style, &ServoMetricsProvider); - damage |= RestyleDamage::compute(&old_style, &fragment.style); + let difference = + RestyleDamage::compute_style_difference(&old_style, &fragment.style); + damage |= difference.damage; } } }); diff --git a/components/style/gecko/restyle_damage.rs b/components/style/gecko/restyle_damage.rs index 7cb66470403..9d443b23baf 100644 --- a/components/style/gecko/restyle_damage.rs +++ b/components/style/gecko/restyle_damage.rs @@ -8,6 +8,7 @@ use gecko_bindings::bindings; use gecko_bindings::structs; use gecko_bindings::structs::{nsChangeHint, nsStyleContext}; use gecko_bindings::sugar::ownership::FFIArcHelpers; +use matching::{StyleChange, StyleDifference}; use properties::ComputedValues; use std::ops::{BitAnd, BitOr, BitOrAssign, Not}; use stylearc::Arc; @@ -38,15 +39,17 @@ impl GeckoRestyleDamage { self.0 == nsChangeHint(0) } - /// Computes a change hint given an old style (in the form of a - /// `nsStyleContext`, and a new style (in the form of `ComputedValues`). + /// Computes the `StyleDifference` (including the appropriate change hint) + /// given an old style (in the form of a `nsStyleContext`, and a new style + /// (in the form of `ComputedValues`). /// /// Note that we could in theory just get two `ComputedValues` here and diff /// them, but Gecko has an interesting optimization when they mark accessed /// structs, so they effectively only diff structs that have ever been /// accessed from layout. - pub fn compute(source: &nsStyleContext, - new_style: &Arc) -> Self { + pub fn compute_style_difference(source: &nsStyleContext, + new_style: &Arc) + -> StyleDifference { // TODO(emilio): Const-ify this? let context = source as *const nsStyleContext as *mut nsStyleContext; let mut any_style_changed: bool = false; @@ -55,7 +58,8 @@ impl GeckoRestyleDamage { new_style.as_borrowed_opt().unwrap(), &mut any_style_changed) }; - GeckoRestyleDamage(hint) + let change = if any_style_changed { StyleChange::Changed } else { StyleChange::Unchanged }; + StyleDifference::new(GeckoRestyleDamage(hint), change) } /// Returns true if this restyle damage contains all the damage of |other|. diff --git a/components/style/matching.rs b/components/style/matching.rs index 9b1711f3d48..0af85c7d0e3 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -51,6 +51,34 @@ fn relations_are_shareable(relations: &StyleRelations) -> bool { AFFECTED_BY_PRESENTATIONAL_HINTS) } +/// Represents the result of comparing an element's old and new style. +pub struct StyleDifference { + /// The resulting damage. + pub damage: RestyleDamage, + + /// Whether any styles changed. + pub change: StyleChange, +} + +impl StyleDifference { + /// Creates a new `StyleDifference`. + pub fn new(damage: RestyleDamage, change: StyleChange) -> Self { + StyleDifference { + change: change, + damage: damage, + } + } +} + +/// Represents whether or not the style of an element has changed. +#[derive(Copy, Clone)] +pub enum StyleChange { + /// The style hasn't changed. + Unchanged, + /// The style has changed. + Changed, +} + /// Information regarding a style sharing candidate. /// /// Note that this information is stored in TLS and cleared after the traversal, @@ -375,8 +403,37 @@ pub enum StyleSharingResult { /// We didn't find anybody to share the style with. CannotShare, /// The node's style can be shared. The integer specifies the index in the - /// LRU cache that was hit and the damage that was done. - StyleWasShared(usize), + /// LRU cache that was hit and the damage that was done. The + /// `ChildCascadeRequirement` indicates whether style changes due to using + /// the shared style mean we need to recascade to children. + StyleWasShared(usize, ChildCascadeRequirement), +} + +/// Whether or not newly computed values for an element need to be cascade +/// to children. +pub enum ChildCascadeRequirement { + /// Old and new computed values were the same, or we otherwise know that + /// we won't bother recomputing style for children, so we can skip cascading + /// the new values into child elements. + CanSkipCascade, + /// Old and new computed values were different, so we must cascade the + /// new values to children. + /// + /// FIXME(heycam) Although this is "must" cascade, in the future we should + /// track whether child elements rely specifically on inheriting particular + /// property values. When we do that, we can treat `MustCascade` as "must + /// cascade unless we know that changes to these properties can be + /// ignored". + MustCascade, +} + +impl From for ChildCascadeRequirement { + fn from(change: StyleChange) -> ChildCascadeRequirement { + match change { + StyleChange::Unchanged => ChildCascadeRequirement::CanSkipCascade, + StyleChange::Changed => ChildCascadeRequirement::MustCascade, + } + } } /// The result status for match primary rules. @@ -570,7 +627,8 @@ trait PrivateMatchMethods: TElement { fn cascade_primary(&self, context: &mut StyleContext, data: &mut ElementData, - important_rules_changed: bool) { + important_rules_changed: bool) + -> ChildCascadeRequirement { // Collect some values. let (mut styles, restyle) = data.styles_and_restyle_mut(); let mut primary_style = &mut styles.primary; @@ -589,16 +647,19 @@ trait PrivateMatchMethods: TElement { important_rules_changed); } - if let Some(old) = old_values { + let child_cascade_requirement = self.accumulate_damage(&context.shared, - restyle.unwrap(), - &old, + restyle, + old_values.as_ref().map(|v| v.as_ref()), &new_values, None); - } // Set the new computed values. primary_style.values = Some(new_values); + + // Return whether the damage indicates we must cascade new inherited + // values into children. + child_cascade_requirement } fn cascade_eager_pseudo(&self, @@ -613,29 +674,11 @@ trait PrivateMatchMethods: TElement { let new_values = self.cascade_internal(context, &styles.primary, Some(pseudo_style)); - if let Some(old) = old_values { - // ::before and ::after are element-backed in Gecko, so they do the - // damage calculation for themselves, when there's an actual pseudo. - // - // NOTE(emilio): An alternative to this check is to just remove the - // pseudo-style entry here if new_values is display: none or has - // an ineffective content property... - // - // I think it's nice to handle it here instead for symmetry with how - // we handle display: none elements, but the other approach may be - // ok too? - let is_unexisting_before_or_after = - pseudo.is_before_or_after() && - self.existing_style_for_restyle_damage(&old, Some(pseudo)).is_none(); - - if cfg!(feature = "servo") || is_unexisting_before_or_after { - self.accumulate_damage(&context.shared, - restyle.unwrap(), - &old, - &new_values, - Some(pseudo)); - } - } + self.accumulate_damage(&context.shared, + restyle, + old_values.as_ref().map(|v| &**v), + &new_values, + Some(pseudo)); pseudo_style.values = Some(new_values) } @@ -794,54 +837,89 @@ trait PrivateMatchMethods: TElement { } } - /// Computes and applies non-redundant damage. - #[cfg(feature = "gecko")] + /// Computes and applies restyle damage. fn accumulate_damage(&self, shared_context: &SharedStyleContext, - restyle: &mut RestyleData, - old_values: &ComputedValues, + restyle: Option<&mut RestyleData>, + old_values: Option<&ComputedValues>, new_values: &Arc, - pseudo: Option<&PseudoElement>) { + pseudo: Option<&PseudoElement>) + -> ChildCascadeRequirement { + let restyle = match restyle { + Some(r) => r, + None => return ChildCascadeRequirement::MustCascade, + }; + + let old_values = match old_values { + Some(v) => v, + None => return ChildCascadeRequirement::MustCascade, + }; + + // ::before and ::after are element-backed in Gecko, so they do the + // damage calculation for themselves, when there's an actual pseudo. + let is_existing_before_or_after = + cfg!(feature = "gecko") && + pseudo.map_or(false, |p| p.is_before_or_after()) && + self.existing_style_for_restyle_damage(old_values, pseudo) + .is_some(); + + if is_existing_before_or_after { + return ChildCascadeRequirement::CanSkipCascade; + } + + self.accumulate_damage_for(shared_context, + restyle, + old_values, + new_values, + pseudo) + } + + /// Computes and applies non-redundant damage. + #[cfg(feature = "gecko")] + fn accumulate_damage_for(&self, + shared_context: &SharedStyleContext, + restyle: &mut RestyleData, + old_values: &ComputedValues, + new_values: &Arc, + pseudo: Option<&PseudoElement>) + -> ChildCascadeRequirement { // Don't accumulate damage if we're in a restyle for reconstruction. if shared_context.traversal_flags.for_reconstruct() { - return; + return ChildCascadeRequirement::MustCascade; } // If an ancestor is already getting reconstructed by Gecko's top-down - // frame constructor, no need to apply damage. - if restyle.damage_handled.contains(RestyleDamage::reconstruct()) { - restyle.damage = RestyleDamage::empty(); - return; - } - - // Add restyle damage, but only the bits that aren't redundant with - // respect to damage applied on our ancestors. + // frame constructor, no need to apply damage. Similarly if we already + // have an explicitly stored ReconstructFrame hint. // // See https://bugzilla.mozilla.org/show_bug.cgi?id=1301258#c12 - // for followup work to make the optimization here more optimal by - // considering each bit individually. - if !restyle.damage.contains(RestyleDamage::reconstruct()) { - let new_damage = self.compute_restyle_damage(&old_values, - &new_values, - pseudo); - if !restyle.damage_handled.contains(new_damage) { - restyle.damage |= new_damage; - } + // for followup work to make the optimization here more optimal by considering + // each bit individually. + let skip_applying_damage = + restyle.damage_handled.contains(RestyleDamage::reconstruct()) || + restyle.damage.contains(RestyleDamage::reconstruct()); + + let difference = self.compute_style_difference(&old_values, + &new_values, + pseudo); + if !skip_applying_damage { + restyle.damage |= difference.damage; } + difference.change.into() } /// Computes and applies restyle damage unless we've already maxed it out. #[cfg(feature = "servo")] - fn accumulate_damage(&self, - _shared_context: &SharedStyleContext, - restyle: &mut RestyleData, - old_values: &ComputedValues, - new_values: &Arc, - pseudo: Option<&PseudoElement>) { - if restyle.damage != RestyleDamage::rebuild_and_reflow() { - restyle.damage |= - self.compute_restyle_damage(&old_values, &new_values, pseudo); - } + fn accumulate_damage_for(&self, + _shared_context: &SharedStyleContext, + restyle: &mut RestyleData, + old_values: &ComputedValues, + new_values: &Arc, + pseudo: Option<&PseudoElement>) + -> ChildCascadeRequirement { + let difference = self.compute_style_difference(&old_values, &new_values, pseudo); + restyle.damage |= difference.damage; + difference.change.into() } #[cfg(feature = "servo")] @@ -929,14 +1007,19 @@ pub trait MatchMethods : TElement { fn match_and_cascade(&self, context: &mut StyleContext, data: &mut ElementData, - sharing: StyleSharingBehavior) + sharing: StyleSharingBehavior) -> ChildCascadeRequirement { // Perform selector matching for the primary style. let mut relations = StyleRelations::empty(); let result = self.match_primary(context, data, &mut relations); // Cascade properties and compute primary values. - self.cascade_primary(context, data, result.important_rules_overriding_animation_changed); + let child_cascade_requirement = + self.cascade_primary( + context, + data, + result.important_rules_overriding_animation_changed + ); // Match and cascade eager pseudo-elements. if !data.styles().is_display_none() { @@ -969,6 +1052,8 @@ pub trait MatchMethods : TElement { relations, revalidation_match_results); } + + child_cascade_requirement } /// Performs the cascade, without matching. @@ -976,9 +1061,12 @@ pub trait MatchMethods : TElement { context: &mut StyleContext, mut data: &mut ElementData, important_rules_changed: bool) + -> ChildCascadeRequirement { - self.cascade_primary(context, &mut data, important_rules_changed); + let child_cascade_requirement = + self.cascade_primary(context, &mut data, important_rules_changed); self.cascade_pseudos(context, &mut data); + child_cascade_requirement } /// Runs selector matching to (re)compute the primary rule node for this element. @@ -1354,11 +1442,12 @@ pub trait MatchMethods : TElement { debug_assert_eq!(data.has_styles(), data.has_restyle()); let old_values = data.get_styles_mut() .and_then(|s| s.primary.values.take()); - if let Some(old) = old_values { + let child_cascade_requirement = self.accumulate_damage(&context.shared, - data.restyle_mut(), &old, - shared_style.values(), None); - } + data.get_restyle_mut(), + old_values.as_ref().map(|v| v.as_ref()), + shared_style.values(), + None); // We never put elements with pseudo style into the style // sharing cache, so we can just mint an ElementStyles @@ -1368,7 +1457,7 @@ pub trait MatchMethods : TElement { let styles = ElementStyles::new(shared_style); data.set_styles(styles); - return StyleSharingResult::StyleWasShared(i) + return StyleSharingResult::StyleWasShared(i, child_cascade_requirement) } Err(miss) => { debug!("Cache miss: {:?}", miss); @@ -1452,14 +1541,14 @@ pub trait MatchMethods : TElement { /// Given the old and new style of this element, and whether it's a /// pseudo-element, compute the restyle damage used to determine which /// kind of layout or painting operations we'll need. - fn compute_restyle_damage(&self, - old_values: &ComputedValues, - new_values: &Arc, - pseudo: Option<&PseudoElement>) - -> RestyleDamage + fn compute_style_difference(&self, + old_values: &ComputedValues, + new_values: &Arc, + pseudo: Option<&PseudoElement>) + -> StyleDifference { if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) { - return RestyleDamage::compute(source, new_values); + return RestyleDamage::compute_style_difference(source, new_values) } let new_style_is_display_none = @@ -1474,7 +1563,7 @@ pub trait MatchMethods : TElement { // pseudo-elements. if new_style_is_display_none && old_style_is_display_none { // The style remains display:none. No need for damage. - return RestyleDamage::empty() + return StyleDifference::new(RestyleDamage::empty(), StyleChange::Unchanged) } if pseudo.map_or(false, |p| p.is_before_or_after()) { @@ -1484,18 +1573,19 @@ pub trait MatchMethods : TElement { new_values.ineffective_content_property()) { // The pseudo-element will remain undisplayed, so just avoid // triggering any change. - return RestyleDamage::empty() + return StyleDifference::new(RestyleDamage::empty(), StyleChange::Unchanged) } - return RestyleDamage::reconstruct() + return StyleDifference::new(RestyleDamage::reconstruct(), StyleChange::Changed) } // Something else. Be conservative for now. warn!("Reframing due to lack of old style source: {:?}, pseudo: {:?}", self, pseudo); - RestyleDamage::reconstruct() + // Something else. Be conservative for now. + StyleDifference::new(RestyleDamage::reconstruct(), StyleChange::Changed) } - /// Cascade the eager pseudo-elements of this element. + /// Performs the cascade for the element's eager pseudos. fn cascade_pseudos(&self, context: &mut StyleContext, mut data: &mut ElementData) diff --git a/components/style/servo/restyle_damage.rs b/components/style/servo/restyle_damage.rs index 69199d6e982..59a03a3f085 100644 --- a/components/style/servo/restyle_damage.rs +++ b/components/style/servo/restyle_damage.rs @@ -9,6 +9,7 @@ use computed_values::display; use heapsize::HeapSizeOf; +use matching::{StyleChange, StyleDifference}; use properties::ServoComputedValues; use std::fmt; @@ -57,12 +58,14 @@ impl HeapSizeOf for ServoRestyleDamage { } impl ServoRestyleDamage { - /// Compute the appropriate restyle damage for a given style change between - /// `old` and `new`. - pub fn compute(old: &ServoComputedValues, - new: &ServoComputedValues) - -> ServoRestyleDamage { - compute_damage(old, new) + /// Compute the `StyleDifference` (including the appropriate restyle damage) + /// for a given style change between `old` and `new`. + pub fn compute_style_difference(old: &ServoComputedValues, + new: &ServoComputedValues) + -> StyleDifference { + let damage = compute_damage(old, new); + let change = if damage.is_empty() { StyleChange::Unchanged } else { StyleChange::Changed }; + StyleDifference::new(damage, change) } /// Returns a bitmask that represents a flow that needs to be rebuilt and diff --git a/components/style/traversal.rs b/components/style/traversal.rs index d91b81149fb..ab12776eeb8 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -8,7 +8,7 @@ use atomic_refcell::{AtomicRefCell, AtomicRefMut}; use context::{SharedStyleContext, StyleContext, ThreadLocalStyleContext}; use data::{ElementData, ElementStyles, StoredRestyleHint}; use dom::{DirtyDescendants, NodeInfo, OpaqueNode, TElement, TNode}; -use matching::{MatchMethods, StyleSharingBehavior}; +use matching::{ChildCascadeRequirement, MatchMethods, StyleSharingBehavior}; use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_SELF}; use selector_parser::RestyleDamage; #[cfg(feature = "servo")] use servo_config::opts; @@ -611,7 +611,12 @@ pub fn recalc_style_at(traversal: &D, // Compute style for this element if necessary. if compute_self { - compute_style(traversal, traversal_data, context, element, &mut data); + match compute_style(traversal, traversal_data, context, element, &mut data) { + ChildCascadeRequirement::MustCascade => { + inherited_style_changed = true; + } + ChildCascadeRequirement::CanSkipCascade => {} + }; // If we're restyling this element to display:none, throw away all style // data in the subtree, notify the caller to early-return. @@ -620,9 +625,6 @@ pub fn recalc_style_at(traversal: &D, element); clear_descendant_data(element, &|e| unsafe { D::clear_element_data(&e) }); } - - // FIXME(bholley): Compute this accurately from the call to CalcStyleDifference. - inherited_style_changed = true; } // Now that matching and cascading is done, clear the bits corresponding to @@ -711,7 +713,7 @@ fn compute_style(_traversal: &D, traversal_data: &PerLevelTraversalData, context: &mut StyleContext, element: E, - mut data: &mut AtomicRefMut) + mut data: &mut AtomicRefMut) -> ChildCascadeRequirement where E: TElement, D: DomTraversal, { @@ -727,10 +729,10 @@ fn compute_style(_traversal: &D, let sharing_result = unsafe { element.share_style_if_possible(context, &mut data) }; - if let StyleWasShared(index) = sharing_result { + if let StyleWasShared(index, had_damage) = sharing_result { context.thread_local.statistics.styles_shared += 1; context.thread_local.style_sharing_candidate_cache.touch(index); - return; + return had_damage; } } @@ -744,17 +746,28 @@ fn compute_style(_traversal: &D, context.thread_local.statistics.elements_matched += 1; // Perform the matching and cascading. - element.match_and_cascade(context, &mut data, StyleSharingBehavior::Allow); + element.match_and_cascade( + context, + &mut data, + StyleSharingBehavior::Allow + ) } - CascadeWithReplacements(hint) => { - let rules_changed = element.replace_rules(hint, context, &mut data); - element.cascade_primary_and_pseudos(context, &mut data, - rules_changed.important_rules_changed()); + CascadeWithReplacements(flags) => { + let rules_changed = element.replace_rules(flags, 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, false); + element.cascade_primary_and_pseudos( + context, + &mut data, + /* important_rules_changed = */ false + ) } - }; + } } fn preprocess_children(traversal: &D, From c13be5cd13d33c509422b908f93f5ec70524119b Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Fri, 19 May 2017 17:39:15 +0800 Subject: [PATCH 4/8] style: List the matched rules with trace-level logging is enabled. --- components/style/matching.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 0af85c7d0e3..2581bbbf6d0 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -16,6 +16,7 @@ use context::{CurrentElementInfo, SelectorFlagsMap, SharedStyleContext, StyleCon use data::{ComputedStyle, ElementData, ElementStyles, RestyleData}; use dom::{AnimationRules, SendElement, TElement, TNode}; use font_metrics::FontMetricsProvider; +use log::LogLevel::Trace; use properties::{CascadeFlags, ComputedValues, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, cascade}; use properties::longhands::display::computed_value as display; use restyle_hints::{RESTYLE_CSS_ANIMATIONS, RESTYLE_CSS_TRANSITIONS, RestyleHint}; @@ -1163,10 +1164,22 @@ pub trait MatchMethods : TElement { &mut applicable_declarations, &context.shared.guards); - let important_rules_changed = self.has_animations() && - data.has_styles() && - data.important_rules_are_different(&primary_rule_node, - &context.shared.guards); + if log_enabled!(Trace) { + trace!("Matched rules:"); + for rn in primary_rule_node.self_and_ancestors() { + if let Some(source) = rn.style_source() { + trace!(" > {:?}", source); + } + } + } + + 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), From a397590838cb573fc72d417527db04f7049068c1 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Fri, 19 May 2017 17:39:15 +0800 Subject: [PATCH 5/8] style: Refactor RestyleHint to be a struct. Later PRs will add additional data to it that is not so easy to represent using bitflags. --- components/layout_thread/lib.rs | 6 +- components/script/dom/document.rs | 6 +- components/script/dom/element.rs | 4 +- components/style/data.rs | 49 ++--- components/style/matching.rs | 14 +- components/style/restyle_hints.rs | 308 ++++++++++++++++++++++++------ components/style/traversal.rs | 8 +- ports/geckolib/glue.rs | 8 +- tests/unit/style/restyle_hints.rs | 2 +- 9 files changed, 294 insertions(+), 111 deletions(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 8185aa6e409..9f5b60b0045 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -1100,7 +1100,7 @@ impl LayoutThread { let el = node.as_element().unwrap(); if let Some(mut d) = element.mutate_data() { if d.has_styles() { - d.ensure_restyle().hint.insert(&StoredRestyleHint::subtree()); + d.ensure_restyle().hint.insert(StoredRestyleHint::subtree()); } } if let Some(p) = el.parent_element() { @@ -1136,7 +1136,7 @@ impl LayoutThread { if needs_dirtying { if let Some(mut d) = element.mutate_data() { if d.has_styles() { - d.ensure_restyle().hint.insert(&StoredRestyleHint::subtree()); + d.ensure_restyle().hint.insert(StoredRestyleHint::subtree()); } } } @@ -1184,7 +1184,7 @@ impl LayoutThread { let mut restyle_data = style_data.ensure_restyle(); // Stash the data on the element for processing by the style system. - restyle_data.hint.insert(&restyle.hint.into()); + restyle_data.hint.insert(restyle.hint.into()); restyle_data.damage = restyle.damage; debug!("Noting restyle for {:?}: {:?}", el, restyle_data); } diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 81ea073ab41..3bca1dcee9e 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -131,7 +131,7 @@ use std::rc::Rc; use std::time::{Duration, Instant}; use style::attr::AttrValue; use style::context::{QuirksMode, ReflowGoal}; -use style::restyle_hints::{RestyleHint, RESTYLE_SELF, RESTYLE_STYLE_ATTRIBUTE}; +use style::restyle_hints::{RestyleHint, RestyleReplacements, RESTYLE_STYLE_ATTRIBUTE}; use style::selector_parser::{RestyleDamage, Snapshot}; use style::shared_lock::SharedRwLock as StyleSharedRwLock; use style::str::{HTML_SPACE_CHARACTERS, split_html_space_chars, str_join}; @@ -2361,14 +2361,14 @@ impl Document { entry.snapshot = Some(Snapshot::new(el.html_element_in_html_document())); } if attr.local_name() == &local_name!("style") { - entry.hint |= RESTYLE_STYLE_ATTRIBUTE; + entry.hint.insert(RestyleHint::for_replacements(RESTYLE_STYLE_ATTRIBUTE)); } // FIXME(emilio): This should become something like // element.is_attribute_mapped(attr.local_name()). if attr.local_name() == &local_name!("width") || attr.local_name() == &local_name!("height") { - entry.hint |= RESTYLE_SELF; + entry.hint.insert(RestyleHint::for_self()); } let mut snapshot = entry.snapshot.as_mut().unwrap(); diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 55175fd45c6..0dc3db0ebe5 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -102,7 +102,7 @@ use style::context::{QuirksMode, ReflowGoal}; use style::element_state::*; use style::properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock, parse_style_attribute}; use style::properties::longhands::{self, background_image, border_spacing, font_family, font_size, overflow_x}; -use style::restyle_hints::RESTYLE_SELF; +use style::restyle_hints::RestyleHint; use style::rule_tree::CascadeLevel; use style::selector_parser::{NonTSPseudoClass, PseudoElement, RestyleDamage, SelectorImpl, SelectorParser}; use style::shared_lock::{SharedRwLock, Locked}; @@ -245,7 +245,7 @@ impl Element { // FIXME(bholley): I think we should probably only do this for // NodeStyleDamaged, but I'm preserving existing behavior. - restyle.hint |= RESTYLE_SELF; + restyle.hint.insert(RestyleHint::for_self()); if damage == NodeDamage::OtherNodeDamage { restyle.damage = RestyleDamage::rebuild_and_reflow(); diff --git a/components/style/data.rs b/components/style/data.rs index c1ea0865317..e7248cf94d2 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -10,7 +10,7 @@ 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 restyle_hints::{RestyleReplacements, RestyleHint}; use rule_tree::StrongRuleNode; use selector_parser::{EAGER_PSEUDO_COUNT, PseudoElement, RestyleDamage}; use shared_lock::StylesheetGuards; @@ -198,21 +198,18 @@ impl StoredRestyleHint { // In the middle of an animation only restyle, we don't need to // propagate any restyle hints, and we need to remove ourselves. if traversal_flags.for_animation_only() { - self.0.remove(RestyleHint::for_animations()); + self.0.remove_animation_hints(); return Self::empty(); } - debug_assert!(!self.0.intersects(RestyleHint::for_animations()), + debug_assert!(!self.0.has_animation_hint(), "There should not be any animation restyle hints \ during normal traversal"); // Else we should clear ourselves, and return the propagated hint. - let hint = mem::replace(&mut self.0, RestyleHint::empty()); - StoredRestyleHint(if hint.contains(RESTYLE_DESCENDANTS) { - RESTYLE_SELF | RESTYLE_DESCENDANTS - } else { - RestyleHint::empty() - }) + let new_hint = mem::replace(&mut self.0, RestyleHint::empty()) + .propagate_for_non_animation_restyle(); + StoredRestyleHint(new_hint) } /// Creates an empty `StoredRestyleHint`. @@ -223,25 +220,25 @@ impl StoredRestyleHint { /// Creates a restyle hint that forces the whole subtree to be restyled, /// including the element. pub fn subtree() -> Self { - StoredRestyleHint(RESTYLE_SELF | RESTYLE_DESCENDANTS) + StoredRestyleHint(RestyleHint::subtree()) } /// Creates a restyle hint that forces the element and all its later /// siblings to have their whole subtrees restyled, including the elements /// themselves. pub fn subtree_and_later_siblings() -> Self { - StoredRestyleHint(RESTYLE_SELF | RESTYLE_DESCENDANTS | RESTYLE_LATER_SIBLINGS) + StoredRestyleHint(RestyleHint::subtree_and_later_siblings()) } /// Returns true if the hint indicates that our style may be invalidated. pub fn has_self_invalidations(&self) -> bool { - self.0.intersects(RestyleHint::for_self()) + self.0.affects_self() } /// Returns true if the hint indicates that our sibling's style may be /// invalidated. pub fn has_sibling_invalidations(&self) -> bool { - self.0.intersects(RESTYLE_LATER_SIBLINGS) + self.0.affects_later_siblings() } /// Whether the restyle hint is empty (nothing requires to be restyled). @@ -250,13 +247,18 @@ impl StoredRestyleHint { } /// Insert another restyle hint, effectively resulting in the union of both. - pub fn insert(&mut self, other: &Self) { - self.0 |= other.0 + pub fn insert(&mut self, other: Self) { + self.0.insert(other.0) + } + + /// Insert another restyle hint, effectively resulting in the union of both. + pub fn insert_from(&mut self, other: &Self) { + self.0.insert_from(&other.0) } /// Returns true if the hint has animation-only restyle. pub fn has_animation_hint(&self) -> bool { - self.0.intersects(RestyleHint::for_animations()) + self.0.has_animation_hint() } } @@ -356,7 +358,7 @@ pub enum RestyleKind { MatchAndCascade, /// We need to recascade with some replacement rule, such as the style /// attribute, or animation rules. - CascadeWithReplacements(RestyleHint), + CascadeWithReplacements(RestyleReplacements), /// We only need to recascade, for example, because only inherited /// properties in the parent changed. CascadeOnly, @@ -381,7 +383,7 @@ impl ElementData { context.traversal_flags); let mut hint = match self.get_restyle() { - Some(r) => r.hint.0, + Some(r) => r.hint.0.clone(), None => RestyleHint::empty(), }; @@ -393,7 +395,7 @@ impl ElementData { element.implemented_pseudo_element()); if element.has_snapshot() && !element.handled_snapshot() { - hint |= context.stylist.compute_restyle_hint(&element, context.snapshot_map); + hint.insert(context.stylist.compute_restyle_hint(&element, context.snapshot_map)); unsafe { element.set_handled_snapshot() } debug_assert!(element.handled_snapshot()); } @@ -402,8 +404,7 @@ impl ElementData { // If the hint includes a directive for later siblings, strip it out and // notify the caller to modify the base hint for future siblings. - let later_siblings = hint.contains(RESTYLE_LATER_SIBLINGS); - hint.remove(RESTYLE_LATER_SIBLINGS); + let later_siblings = hint.remove_later_siblings_hint(); // Insert the hint, overriding the previous hint. This effectively takes // care of removing the later siblings restyle hint. @@ -445,13 +446,13 @@ impl ElementData { debug_assert!(self.restyle.is_some()); let restyle_data = self.restyle.as_ref().unwrap(); - let hint = restyle_data.hint.0; - if hint.contains(RESTYLE_SELF) { + let hint = &restyle_data.hint.0; + if hint.match_self() { return RestyleKind::MatchAndCascade; } if !hint.is_empty() { - return RestyleKind::CascadeWithReplacements(hint); + return RestyleKind::CascadeWithReplacements(hint.replacements); } debug_assert!(restyle_data.recascade, diff --git a/components/style/matching.rs b/components/style/matching.rs index 2581bbbf6d0..a0598dd2567 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -19,7 +19,7 @@ use font_metrics::FontMetricsProvider; use log::LogLevel::Trace; use properties::{CascadeFlags, ComputedValues, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, cascade}; use properties::longhands::display::computed_value as display; -use restyle_hints::{RESTYLE_CSS_ANIMATIONS, RESTYLE_CSS_TRANSITIONS, RestyleHint}; +use restyle_hints::{RESTYLE_CSS_ANIMATIONS, RESTYLE_CSS_TRANSITIONS, RestyleReplacements}; use restyle_hints::{RESTYLE_STYLE_ATTRIBUTE, RESTYLE_SMIL}; use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode}; use selector_parser::{PseudoElement, RestyleDamage, SelectorImpl}; @@ -1327,7 +1327,7 @@ pub trait MatchMethods : TElement { /// the rule tree. Returns RulesChanged which indicates whether the rule nodes changed /// and whether the important rules changed. fn replace_rules(&self, - hint: RestyleHint, + replacements: RestyleReplacements, context: &StyleContext, data: &mut AtomicRefMut) -> RulesChanged { @@ -1359,10 +1359,10 @@ pub trait MatchMethods : TElement { // // Non-animation restyle hints will be processed in a subsequent // normal traversal. - if hint.intersects(RestyleHint::for_animations()) { + if replacements.intersects(RestyleReplacements::for_animations()) { debug_assert!(context.shared.traversal_flags.for_animation_only()); - if hint.contains(RESTYLE_SMIL) { + if replacements.contains(RESTYLE_SMIL) { replace_rule_node(CascadeLevel::SMILOverride, self.get_smil_override(), primary_rules); @@ -1378,16 +1378,16 @@ pub trait MatchMethods : TElement { // Apply Transition rules and Animation rules if the corresponding restyle hint // is contained. - if hint.contains(RESTYLE_CSS_TRANSITIONS) { + if replacements.contains(RESTYLE_CSS_TRANSITIONS) { replace_rule_node_for_animation(CascadeLevel::Transitions, primary_rules); } - if hint.contains(RESTYLE_CSS_ANIMATIONS) { + if replacements.contains(RESTYLE_CSS_ANIMATIONS) { replace_rule_node_for_animation(CascadeLevel::Animations, primary_rules); } - } else if hint.contains(RESTYLE_STYLE_ATTRIBUTE) { + } else if replacements.contains(RESTYLE_STYLE_ATTRIBUTE) { let style_attribute = self.style_attribute(); replace_rule_node(CascadeLevel::StyleAttributeNormal, style_attribute, diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 73bd39ec1b3..33ec686320d 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -28,32 +28,38 @@ use std::cell::Cell; use std::clone::Clone; use stylist::SelectorMap; +/// When the ElementState of an element (like IN_HOVER_STATE) changes, +/// certain pseudo-classes (like :hover) may require us to restyle that +/// element, its siblings, and/or its descendants. Similarly, when various +/// attributes of an element change, we may also need to restyle things with +/// id, class, and attribute selectors. Doing this conservatively is +/// expensive, and so we use RestyleHints to short-circuit work we know is +/// unnecessary. +#[derive(Debug, Clone, PartialEq)] +pub struct RestyleHint { + /// Rerun selector matching on the element. + match_self: bool, + + /// Rerun selector matching on all of the element's descendants. + match_descendants: bool, + + /// Rerun selector matching on all later siblings of the element and all + /// of their descendants. + match_later_siblings: bool, + + /// Levels of the cascade whose rule nodes should be recomputed and + /// replaced. + pub replacements: RestyleReplacements, +} + bitflags! { - /// When the ElementState of an element (like IN_HOVER_STATE) changes, - /// certain pseudo-classes (like :hover) may require us to restyle that - /// element, its siblings, and/or its descendants. Similarly, when various - /// attributes of an element change, we may also need to restyle things with - /// id, class, and attribute selectors. Doing this conservatively is - /// expensive, and so we use RestyleHints to short-circuit work we know is - /// unnecessary. + /// Cascade levels that can be updated for an element by simply replacing + /// their rule node. /// /// Note that the bit values here must be kept in sync with the Gecko /// nsRestyleHint values. If you add more bits with matching values, /// please add assertions to assert_restyle_hints_match below. - pub flags RestyleHint: u32 { - /// Rerun selector matching on the element. - const RESTYLE_SELF = 0x01, - - /// Rerun selector matching on all of the element's descendants. - /// - /// NB: In Gecko, we have RESTYLE_SUBTREE which is inclusive of self, - /// but heycam isn't aware of a good reason for that. - const RESTYLE_DESCENDANTS = 0x04, - - /// Rerun selector matching on all later siblings of the element and all - /// of their descendants. - const RESTYLE_LATER_SIBLINGS = 0x08, - + pub flags RestyleReplacements: u8 { /// Replace the style data coming from CSS transitions without updating /// any other style data. This hint is only processed in animation-only /// traversal which is prior to normal traversal. @@ -76,7 +82,7 @@ bitflags! { } } -/// Asserts that all RestyleHint flags have a matching nsRestyleHint value. +/// Asserts that all RestyleReplacements have a matching nsRestyleHint value. #[cfg(feature = "gecko")] #[inline] pub fn assert_restyle_hints_match() { @@ -85,24 +91,18 @@ pub fn assert_restyle_hints_match() { macro_rules! check_restyle_hints { ( $( $a:ident => $b:ident ),*, ) => { if cfg!(debug_assertions) { - let mut hints = RestyleHint::all(); + let mut replacements = RestyleReplacements::all(); $( assert_eq!(structs::$a.0 as usize, $b.bits() as usize, stringify!($b)); - hints.remove($b); + replacements.remove($b); )* - assert_eq!(hints, RestyleHint::empty(), "all RestyleHint bits should have an assertion"); + assert_eq!(replacements, RestyleReplacements::empty(), + "all RestyleReplacements bits should have an assertion"); } } } check_restyle_hints! { - nsRestyleHint_eRestyle_Self => RESTYLE_SELF, - // Note that eRestyle_Subtree means "self and descendants", while - // RESTYLE_DESCENDANTS means descendants only. The From impl - // below handles converting eRestyle_Subtree into - // (RESTYLE_SELF | RESTYLE_DESCENDANTS). - nsRestyleHint_eRestyle_Subtree => RESTYLE_DESCENDANTS, - nsRestyleHint_eRestyle_LaterSiblings => RESTYLE_LATER_SIBLINGS, nsRestyleHint_eRestyle_CSSTransitions => RESTYLE_CSS_TRANSITIONS, nsRestyleHint_eRestyle_CSSAnimations => RESTYLE_CSS_ANIMATIONS, nsRestyleHint_eRestyle_StyleAttribute => RESTYLE_STYLE_ATTRIBUTE, @@ -111,39 +111,220 @@ pub fn assert_restyle_hints_match() { } impl RestyleHint { - /// The subset hints that affect the styling of a single element during the - /// traversal. + /// Creates a new, empty `RestyleHint`, which represents no restyling work + /// needs to be done. #[inline] - pub fn for_self() -> Self { - RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE | Self::for_animations() + pub fn empty() -> Self { + RestyleHint { + match_self: false, + match_descendants: false, + match_later_siblings: false, + replacements: RestyleReplacements::empty(), + } } - /// The subset hints that are used for animation restyle. + /// Creates a new `RestyleHint` that indicates selector matching must be + /// re-run on the element. + #[inline] + pub fn for_self() -> Self { + RestyleHint { + match_self: true, + match_descendants: false, + match_later_siblings: false, + replacements: RestyleReplacements::empty(), + } + } + + /// Creates a new `RestyleHint` that indicates selector matching must be + /// re-run on all of the element's descendants. + #[inline] + pub fn descendants() -> Self { + RestyleHint { + match_self: false, + match_descendants: true, + match_later_siblings: false, + replacements: RestyleReplacements::empty(), + } + } + + /// Creates a new `RestyleHint` that indicates selector matching must be + /// re-run on all of the element's later siblings and their descendants. + #[inline] + pub fn later_siblings() -> Self { + RestyleHint { + match_self: false, + match_descendants: false, + match_later_siblings: true, + replacements: RestyleReplacements::empty(), + } + } + + /// Creates a new `RestyleHint` that indicates selector matching must be + /// re-run on the element and all of its descendants. + #[inline] + pub fn subtree() -> Self { + RestyleHint { + match_self: true, + match_descendants: true, + match_later_siblings: false, + replacements: RestyleReplacements::empty(), + } + } + + /// Creates a new `RestyleHint` that indicates selector matching must be + /// re-run on the element, its descendants, its later siblings, and + /// their descendants. + #[inline] + pub fn subtree_and_later_siblings() -> Self { + RestyleHint { + match_self: true, + match_descendants: true, + match_later_siblings: true, + replacements: RestyleReplacements::empty(), + } + } + + /// Creates a new `RestyleHint` that indicates the specified rule node + /// replacements must be performed on the element. + #[inline] + pub fn for_replacements(replacements: RestyleReplacements) -> Self { + RestyleHint { + match_self: false, + match_descendants: false, + match_later_siblings: false, + replacements: replacements, + } + } + + /// Returns whether this `RestyleHint` represents no needed restyle work. + #[inline] + pub fn is_empty(&self) -> bool { + *self == RestyleHint::empty() + } + + /// Returns whether this `RestyleHint` represents the maximum possible + /// restyle work, and thus any `insert()` calls will have no effect. + #[inline] + pub fn is_maximum(&self) -> bool { + self.match_self && self.match_descendants && self.match_later_siblings && self.replacements.is_all() + } + + /// Returns whether the hint specifies that some work must be performed on + /// the current element. + #[inline] + pub fn affects_self(&self) -> bool { + self.match_self || !self.replacements.is_empty() + } + + /// Returns whether the hint specifies that later siblings must be restyled. + #[inline] + pub fn affects_later_siblings(&self) -> bool { + self.match_later_siblings + } + + /// Returns whether the hint specifies that an animation cascade level must + /// be replaced. + #[inline] + pub fn has_animation_hint(&self) -> bool { + self.replacements.intersects(RestyleReplacements::for_animations()) + } + + /// Returns whether the hint specifies some restyle work other than an + /// animation cascade level replacement. + #[inline] + pub fn has_non_animation_hint(&self) -> bool { + self.match_self || self.match_descendants || self.match_later_siblings || + self.replacements.contains(RESTYLE_STYLE_ATTRIBUTE) + } + + /// Returns whether the hint specifies that selector matching must be re-run + /// for the element. + #[inline] + pub fn match_self(&self) -> bool { + self.match_self + } + + /// Returns a new `RestyleHint` appropriate for children of the current + /// element. + #[inline] + pub fn propagate_for_non_animation_restyle(&self) -> Self { + if self.match_descendants { + Self::subtree() + } else { + Self::empty() + } + } + + /// Removes all of the animation-related hints. + #[inline] + pub fn remove_animation_hints(&mut self) { + self.replacements.remove(RestyleReplacements::for_animations()); + } + + /// Removes the later siblings hint, and returns whether it was present. + pub fn remove_later_siblings_hint(&mut self) -> bool { + let later_siblings = self.match_later_siblings; + self.match_later_siblings = false; + later_siblings + } + + /// Unions the specified `RestyleHint` into this one. + #[inline] + pub fn insert_from(&mut self, other: &Self) { + self.match_self |= other.match_self; + self.match_descendants |= other.match_descendants; + self.match_later_siblings |= other.match_later_siblings; + self.replacements.insert(other.replacements); + } + + /// Unions the specified `RestyleHint` into this one. + #[inline] + pub fn insert(&mut self, other: Self) { + // A later patch should make it worthwhile to have an insert() function + // that consumes its argument. + self.insert_from(&other) + } + + /// Returns whether this `RestyleHint` represents at least as much restyle + /// work as the specified one. + #[inline] + pub fn contains(&mut self, other: &Self) -> bool { + !(other.match_self && !self.match_self) && + !(other.match_descendants && !self.match_descendants) && + !(other.match_later_siblings && !self.match_later_siblings) && + self.replacements.contains(other.replacements) + } +} + +impl RestyleReplacements { + /// The replacements for the animation cascade levels. #[inline] pub fn for_animations() -> Self { RESTYLE_SMIL | RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS } } +#[cfg(feature = "gecko")] +impl From for RestyleReplacements { + fn from(raw: nsRestyleHint) -> Self { + Self::from_bits_truncate(raw.0 as u8) + } +} + #[cfg(feature = "gecko")] impl From for RestyleHint { fn from(raw: nsRestyleHint) -> Self { - let raw_bits: u32 = raw.0; + use gecko_bindings::structs::nsRestyleHint_eRestyle_LaterSiblings as eRestyle_LaterSiblings; + use gecko_bindings::structs::nsRestyleHint_eRestyle_Self as eRestyle_Self; + use gecko_bindings::structs::nsRestyleHint_eRestyle_SomeDescendants as eRestyle_SomeDescendants; + use gecko_bindings::structs::nsRestyleHint_eRestyle_Subtree as eRestyle_Subtree; - // FIXME(bholley): Finish aligning the binary representations here and - // then .expect() the result of the checked version. - if Self::from_bits(raw_bits).is_none() { - warn!("stylo: dropping unsupported restyle hint bits"); + RestyleHint { + match_self: (raw.0 & (eRestyle_Self.0 | eRestyle_Subtree.0)) != 0, + match_descendants: (raw.0 & (eRestyle_Subtree.0 | eRestyle_SomeDescendants.0)) != 0, + match_later_siblings: (raw.0 & eRestyle_LaterSiblings.0) != 0, + replacements: raw.into(), } - - let mut bits = Self::from_bits_truncate(raw_bits); - - // eRestyle_Subtree converts to (RESTYLE_SELF | RESTYLE_DESCENDANTS). - if bits.contains(RESTYLE_DESCENDANTS) { - bits |= RESTYLE_SELF; - } - - bits } } @@ -437,15 +618,16 @@ fn is_attr_selector(sel: &Component) -> bool { fn combinator_to_restyle_hint(combinator: Option) -> RestyleHint { match combinator { - None => RESTYLE_SELF, + None => RestyleHint::for_self(), Some(c) => match c { - // NB: RESTYLE_SELF is needed to handle properly eager pseudos, - // otherwise we may leave a stale style on the parent. - Combinator::PseudoElement => RESTYLE_SELF | RESTYLE_DESCENDANTS, - Combinator::Child => RESTYLE_DESCENDANTS, - Combinator::Descendant => RESTYLE_DESCENDANTS, - Combinator::NextSibling => RESTYLE_LATER_SIBLINGS, - Combinator::LaterSibling => RESTYLE_LATER_SIBLINGS, + // NB: RestyleHint::subtree() and not RestyleHint::descendants() is + // needed to handle properly eager pseudos, otherwise we may leave + // a stale style on the parent. + Combinator::PseudoElement => RestyleHint::subtree(), + Combinator::Child => RestyleHint::descendants(), + Combinator::Descendant => RestyleHint::descendants(), + Combinator::NextSibling => RestyleHint::later_siblings(), + Combinator::LaterSibling => RestyleHint::later_siblings(), } } } @@ -700,12 +882,12 @@ impl DependencySet { return true; } - if hint.contains(dep.hint) { + if hint.contains(&dep.hint) { trace!(" > hint was already there"); return true; } - // We can ignore the selector flags, since they would have already + // We can ignore the selector replacements, since they would have already // been set during original matching for any element that might // change its matching behavior here. let matched_then = @@ -717,10 +899,10 @@ impl DependencySet { &mut matching_context, &mut |_, _| {}); if matched_then != matches_now { - hint.insert(dep.hint); + hint.insert_from(&dep.hint); } - !hint.is_all() + !hint.is_maximum() }); debug!("Calculated restyle hint: {:?} for {:?}. (State={:?}, {} Deps)", diff --git a/components/style/traversal.rs b/components/style/traversal.rs index ab12776eeb8..542c5611f9b 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -9,7 +9,7 @@ use context::{SharedStyleContext, StyleContext, ThreadLocalStyleContext}; use data::{ElementData, ElementStyles, StoredRestyleHint}; use dom::{DirtyDescendants, NodeInfo, OpaqueNode, TElement, TNode}; use matching::{ChildCascadeRequirement, MatchMethods, StyleSharingBehavior}; -use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_SELF}; +use restyle_hints::RestyleHint; use selector_parser::RestyleDamage; #[cfg(feature = "servo")] use servo_config::opts; use std::borrow::BorrowMut; @@ -232,7 +232,7 @@ pub trait DomTraversal : Sync { if let Some(next) = root.next_sibling_element() { if let Some(mut next_data) = next.mutate_data() { let hint = StoredRestyleHint::subtree_and_later_siblings(); - next_data.ensure_restyle().hint.insert(&hint); + next_data.ensure_restyle().hint.insert(hint); } } } @@ -819,11 +819,11 @@ fn preprocess_children(traversal: &D, // Propagate the parent and sibling restyle hint. if !propagated_hint.is_empty() { - restyle_data.hint.insert(&propagated_hint); + restyle_data.hint.insert_from(&propagated_hint); } if later_siblings { - propagated_hint.insert(&(RESTYLE_SELF | RESTYLE_DESCENDANTS).into()); + propagated_hint.insert(RestyleHint::subtree().into()); } // Store the damage already handled by ancestors. diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index a08068723f7..a78a43ef395 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -2116,16 +2116,16 @@ pub extern "C" fn Servo_NoteExplicitHints(element: RawGeckoElementBorrowed, element, restyle_hint, change_hint); let restyle_hint: RestyleHint = restyle_hint.into(); - debug_assert!(RestyleHint::for_animations().contains(restyle_hint) || - !RestyleHint::for_animations().intersects(restyle_hint), + debug_assert!(!(restyle_hint.has_animation_hint() && + restyle_hint.has_non_animation_hint()), "Animation restyle hints should not appear with non-animation restyle hints"); let mut maybe_data = element.mutate_data(); let maybe_restyle_data = maybe_data.as_mut().and_then(|d| unsafe { - maybe_restyle(d, element, restyle_hint.intersects(RestyleHint::for_animations())) + maybe_restyle(d, element, restyle_hint.has_animation_hint()) }); if let Some(restyle_data) = maybe_restyle_data { - restyle_data.hint.insert(&restyle_hint.into()); + restyle_data.hint.insert(restyle_hint.into()); restyle_data.damage |= damage; } else { debug!("(Element not styled, discarding hints)"); diff --git a/tests/unit/style/restyle_hints.rs b/tests/unit/style/restyle_hints.rs index b2e2cf3e999..c50cfcc87bb 100644 --- a/tests/unit/style/restyle_hints.rs +++ b/tests/unit/style/restyle_hints.rs @@ -6,7 +6,7 @@ fn smoke_restyle_hints() { use cssparser::Parser; use selectors::parser::SelectorList; - use style::restyle_hints::{DependencySet, RESTYLE_LATER_SIBLINGS}; + use style::restyle_hints::DependencySet; use style::selector_parser::SelectorParser; use style::stylesheets::{Origin, Namespaces}; let namespaces = Namespaces::default(); From 178939d8cee656bb72eb3ff322ca4c76c588e207 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Fri, 19 May 2017 17:39:16 +0800 Subject: [PATCH 6/8] style: Record in RestyleHints how far down the tree selector matching must be re-run. --- components/style/restyle_hints.rs | 134 ++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 34 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 33ec686320d..ce38c3f89ef 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -37,11 +37,8 @@ use stylist::SelectorMap; /// unnecessary. #[derive(Debug, Clone, PartialEq)] pub struct RestyleHint { - /// Rerun selector matching on the element. - match_self: bool, - - /// Rerun selector matching on all of the element's descendants. - match_descendants: bool, + /// Depths at which selector matching must be re-run. + match_under_self: RestyleDepths, /// Rerun selector matching on all later siblings of the element and all /// of their descendants. @@ -82,6 +79,75 @@ bitflags! { } } +/// Eight bit wide bitfield representing depths of a DOM subtree's descendants, +/// used to represent which elements must have selector matching re-run on them. +/// +/// The least significant bit indicates that selector matching must be re-run +/// for the element itself, the second least significant bit for the element's +/// children, the third its grandchildren, and so on. If the most significant +/// bit it set, it indicates that that selector matching must be re-run for +/// elements at that depth and all of their descendants. +#[derive(Debug, Clone, Copy, PartialEq)] +struct RestyleDepths(u8); + +impl RestyleDepths { + /// Returns a `RestyleDepths` representing no element depths. + fn empty() -> Self { + RestyleDepths(0) + } + + /// Returns a `RestyleDepths` representing the current element depth. + fn for_self() -> Self { + RestyleDepths(0x01) + } + + /// Returns a `RestyleDepths` representing the depths of all descendants of + /// the current element. + fn for_descendants() -> Self { + RestyleDepths(0xfe) + } + + /// Returns a `RestyleDepths` representing the current element depth and the + /// depths of all the current element's descendants. + fn for_self_and_descendants() -> Self { + RestyleDepths(0xff) + } + + /// Returns whether this `RestyleDepths` represents the current element + /// depth and the depths of all the current element's descendants. + fn is_self_and_descendants(&self) -> bool { + self.0 == 0xff + } + + /// Returns whether this `RestyleDepths` includes any element depth. + fn is_any(&self) -> bool { + self.0 != 0 + } + + /// Returns whether this `RestyleDepths` includes the current element depth. + fn has_self(&self) -> bool { + (self.0 & 0x01) != 0 + } + + /// Returns a new `RestyleDepths` with all depth values represented by this + /// `RestyleDepths` reduced by one. + fn propagate(&self) -> Self { + RestyleDepths((self.0 >> 1) | (self.0 & 0x80)) + } + + /// Returns a new `RestyleDepths` that represents the union of the depths + /// from `self` and `other`. + fn insert(&mut self, other: RestyleDepths) { + self.0 |= other.0; + } + + /// Returns whether this `RestyleDepths` includes all depths represented + /// by `other`. + fn contains(&self, other: RestyleDepths) -> bool { + (self.0 & other.0) == other.0 + } +} + /// Asserts that all RestyleReplacements have a matching nsRestyleHint value. #[cfg(feature = "gecko")] #[inline] @@ -116,8 +182,7 @@ impl RestyleHint { #[inline] pub fn empty() -> Self { RestyleHint { - match_self: false, - match_descendants: false, + match_under_self: RestyleDepths::empty(), match_later_siblings: false, replacements: RestyleReplacements::empty(), } @@ -128,8 +193,7 @@ impl RestyleHint { #[inline] pub fn for_self() -> Self { RestyleHint { - match_self: true, - match_descendants: false, + match_under_self: RestyleDepths::for_self(), match_later_siblings: false, replacements: RestyleReplacements::empty(), } @@ -140,8 +204,7 @@ impl RestyleHint { #[inline] pub fn descendants() -> Self { RestyleHint { - match_self: false, - match_descendants: true, + match_under_self: RestyleDepths::for_descendants(), match_later_siblings: false, replacements: RestyleReplacements::empty(), } @@ -152,8 +215,7 @@ impl RestyleHint { #[inline] pub fn later_siblings() -> Self { RestyleHint { - match_self: false, - match_descendants: false, + match_under_self: RestyleDepths::empty(), match_later_siblings: true, replacements: RestyleReplacements::empty(), } @@ -164,8 +226,7 @@ impl RestyleHint { #[inline] pub fn subtree() -> Self { RestyleHint { - match_self: true, - match_descendants: true, + match_under_self: RestyleDepths::for_self_and_descendants(), match_later_siblings: false, replacements: RestyleReplacements::empty(), } @@ -177,8 +238,7 @@ impl RestyleHint { #[inline] pub fn subtree_and_later_siblings() -> Self { RestyleHint { - match_self: true, - match_descendants: true, + match_under_self: RestyleDepths::for_self_and_descendants(), match_later_siblings: true, replacements: RestyleReplacements::empty(), } @@ -189,8 +249,7 @@ impl RestyleHint { #[inline] pub fn for_replacements(replacements: RestyleReplacements) -> Self { RestyleHint { - match_self: false, - match_descendants: false, + match_under_self: RestyleDepths::empty(), match_later_siblings: false, replacements: replacements, } @@ -206,14 +265,16 @@ impl RestyleHint { /// restyle work, and thus any `insert()` calls will have no effect. #[inline] pub fn is_maximum(&self) -> bool { - self.match_self && self.match_descendants && self.match_later_siblings && self.replacements.is_all() + self.match_under_self.is_self_and_descendants() && + self.match_later_siblings && + self.replacements.is_all() } /// Returns whether the hint specifies that some work must be performed on /// the current element. #[inline] pub fn affects_self(&self) -> bool { - self.match_self || !self.replacements.is_empty() + self.match_self() || !self.replacements.is_empty() } /// Returns whether the hint specifies that later siblings must be restyled. @@ -233,7 +294,7 @@ impl RestyleHint { /// animation cascade level replacement. #[inline] pub fn has_non_animation_hint(&self) -> bool { - self.match_self || self.match_descendants || self.match_later_siblings || + self.match_under_self.is_any() || self.match_later_siblings || self.replacements.contains(RESTYLE_STYLE_ATTRIBUTE) } @@ -241,17 +302,17 @@ impl RestyleHint { /// for the element. #[inline] pub fn match_self(&self) -> bool { - self.match_self + self.match_under_self.has_self() } /// Returns a new `RestyleHint` appropriate for children of the current /// element. #[inline] pub fn propagate_for_non_animation_restyle(&self) -> Self { - if self.match_descendants { - Self::subtree() - } else { - Self::empty() + RestyleHint { + match_under_self: self.match_under_self.propagate(), + match_later_siblings: false, + replacements: RestyleReplacements::empty(), } } @@ -271,8 +332,7 @@ impl RestyleHint { /// Unions the specified `RestyleHint` into this one. #[inline] pub fn insert_from(&mut self, other: &Self) { - self.match_self |= other.match_self; - self.match_descendants |= other.match_descendants; + self.match_under_self.insert(other.match_under_self); self.match_later_siblings |= other.match_later_siblings; self.replacements.insert(other.replacements); } @@ -289,9 +349,8 @@ impl RestyleHint { /// work as the specified one. #[inline] pub fn contains(&mut self, other: &Self) -> bool { - !(other.match_self && !self.match_self) && - !(other.match_descendants && !self.match_descendants) && - !(other.match_later_siblings && !self.match_later_siblings) && + self.match_under_self.contains(other.match_under_self) && + (self.match_later_siblings & other.match_later_siblings) == other.match_later_siblings && self.replacements.contains(other.replacements) } } @@ -319,9 +378,16 @@ impl From for RestyleHint { use gecko_bindings::structs::nsRestyleHint_eRestyle_SomeDescendants as eRestyle_SomeDescendants; use gecko_bindings::structs::nsRestyleHint_eRestyle_Subtree as eRestyle_Subtree; + let mut match_under_self = RestyleDepths::empty(); + if (raw.0 & (eRestyle_Self.0 | eRestyle_Subtree.0)) != 0 { + match_under_self.insert(RestyleDepths::for_self()); + } + if (raw.0 & (eRestyle_Subtree.0 | eRestyle_SomeDescendants.0)) != 0 { + match_under_self.insert(RestyleDepths::for_descendants()); + } + RestyleHint { - match_self: (raw.0 & (eRestyle_Self.0 | eRestyle_Subtree.0)) != 0, - match_descendants: (raw.0 & (eRestyle_Subtree.0 | eRestyle_SomeDescendants.0)) != 0, + match_under_self: match_under_self, match_later_siblings: (raw.0 & eRestyle_LaterSiblings.0) != 0, replacements: raw.into(), } From b46e196fa932de3419c6a33f4c8bd89c1ca880c4 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Fri, 19 May 2017 17:39:16 +0800 Subject: [PATCH 7/8] style: Generate RestyleHints to selector match at specific descendant depths. --- components/style/restyle_hints.rs | 67 ++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index ce38c3f89ef..913141f8dd0 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -26,6 +26,7 @@ use smallvec::SmallVec; use std::borrow::Borrow; use std::cell::Cell; use std::clone::Clone; +use std::cmp; use stylist::SelectorMap; /// When the ElementState of an element (like IN_HOVER_STATE) changes, @@ -113,6 +114,12 @@ impl RestyleDepths { RestyleDepths(0xff) } + /// Returns a `RestyleDepths` representing the specified depth, where zero + /// is the current element depth, one is its children's depths, etc. + fn for_depth(depth: u32) -> Self { + RestyleDepths(1u8 << cmp::min(depth, 7)) + } + /// Returns whether this `RestyleDepths` represents the current element /// depth and the depths of all the current element's descendants. fn is_self_and_descendants(&self) -> bool { @@ -210,6 +217,19 @@ impl RestyleHint { } } + /// Creates a new `RestyleHint` that indicates selector matching must be + /// re-run on the descendants of element at the specified depth, where 0 + /// indicates the element itself, 1 is its children, 2 its grandchildren, + /// etc. + #[inline] + pub fn descendants_at_depth(depth: u32) -> Self { + RestyleHint { + match_under_self: RestyleDepths::for_depth(depth), + match_later_siblings: false, + replacements: RestyleReplacements::empty(), + } + } + /// Creates a new `RestyleHint` that indicates selector matching must be /// re-run on all of the element's later siblings and their descendants. #[inline] @@ -682,22 +702,6 @@ fn is_attr_selector(sel: &Component) -> bool { } } -fn combinator_to_restyle_hint(combinator: Option) -> RestyleHint { - match combinator { - None => RestyleHint::for_self(), - Some(c) => match c { - // NB: RestyleHint::subtree() and not RestyleHint::descendants() is - // needed to handle properly eager pseudos, otherwise we may leave - // a stale style on the parent. - Combinator::PseudoElement => RestyleHint::subtree(), - Combinator::Child => RestyleHint::descendants(), - Combinator::Descendant => RestyleHint::descendants(), - Combinator::NextSibling => RestyleHint::later_siblings(), - Combinator::LaterSibling => RestyleHint::later_siblings(), - } - } -} - #[derive(Clone, Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] /// The aspects of an selector which are sensitive. @@ -827,6 +831,8 @@ impl DependencySet { let mut combinator = None; let mut iter = selector.inner.complex.iter(); let mut index = 0; + let mut child_combinators_seen = 0; + let mut saw_descendant_combinator = false; loop { let sequence_start = index; @@ -844,9 +850,34 @@ impl DependencySet { index += 1; // Account for the simple selector. } + // Keep track of how many child combinators we've encountered, + // and whether we've encountered a descendant combinator at all. + match combinator { + Some(Combinator::Child) => child_combinators_seen += 1, + Some(Combinator::Descendant) => saw_descendant_combinator = true, + _ => {} + } + // If we found a sensitivity, add an entry in the dependency set. if !visitor.sensitivities.is_empty() { - let hint = combinator_to_restyle_hint(combinator); + // Compute a RestyleHint given the current combinator and the + // tracked number of child combinators and presence of a + // descendant combinator. + let hint = match combinator { + // NB: RestyleHint::subtree() and not + // RestyleHint::descendants() is needed to handle properly + // eager pseudos, otherwise we may leave a stale style on + // the parent. + Some(Combinator::PseudoElement) => RestyleHint::subtree(), + Some(Combinator::Child) if !saw_descendant_combinator => { + RestyleHint::descendants_at_depth(child_combinators_seen) + } + Some(Combinator::Child) | + Some(Combinator::Descendant) => RestyleHint::descendants(), + Some(Combinator::NextSibling) | + Some(Combinator::LaterSibling) => RestyleHint::later_siblings(), + None => RestyleHint::for_self(), + }; let dep_selector = if sequence_start == 0 { // Reuse the bloom hashes if this is the base selector. @@ -953,7 +984,7 @@ impl DependencySet { return true; } - // We can ignore the selector replacements, since they would have already + // We can ignore the selector flags, since they would have already // been set during original matching for any element that might // change its matching behavior here. let matched_then = From b30d48bf665e312d76e42a279b0ab71131405ce5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 20 May 2017 19:11:36 +0200 Subject: [PATCH 8/8] Mark tests/css/css21_dev/html4/border-collapse-dynamic-table-002.htm as failing. --- .../css21_dev/html4/border-collapse-dynamic-table-002.htm.ini | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-table-002.htm.ini diff --git a/tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-table-002.htm.ini b/tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-table-002.htm.ini new file mode 100644 index 00000000000..0e8db3c02e1 --- /dev/null +++ b/tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-table-002.htm.ini @@ -0,0 +1,3 @@ +[border-collapse-dynamic-table-002.htm] + type: reftest + expected: FAIL