From 8f152563dddfa1f8750f48a084e84c0eabc51726 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 22 Jul 2017 01:32:05 +0200 Subject: [PATCH 1/7] stylo: Stop claiming to support unstyled children traversals for throttled animations. The whole concept made me laugh a bit actually :) MozReview-Commit-ID: 1KJrUYuaHxL --- ports/geckolib/glue.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index bdbf856203e..be1cb1005ef 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -275,8 +275,7 @@ pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed, (Root::Normal, Restyle::ForThrottledAnimationFlush) => TraversalFlags::empty(), (Root::UnstyledChildrenOnly, Restyle::Normal) | - (Root::UnstyledChildrenOnly, Restyle::ForNewlyBoundElement) | - (Root::UnstyledChildrenOnly, Restyle::ForThrottledAnimationFlush) + (Root::UnstyledChildrenOnly, Restyle::ForNewlyBoundElement) => UNSTYLED_CHILDREN_ONLY, (Root::Normal, Restyle::ForCSSRuleChanges) => FOR_CSS_RULE_CHANGES, (Root::Normal, Restyle::ForReconstruct) => FOR_RECONSTRUCT, From 9d0023873e0b15c0fa295b5a85015630845c0e65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 22 Jul 2017 01:33:33 +0200 Subject: [PATCH 2/7] stylo: Improve the information the "styles not up-to-date" assertion gives back. Always I end up debugging it I end up adding a bunch of printlns near it. This should be clearer. MozReview-Commit-ID: ITpfi9tXAw3 --- ports/geckolib/glue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index be1cb1005ef..99e6ef6d259 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -2813,7 +2813,7 @@ pub extern "C" fn Servo_ResolveStyle(element: RawGeckoElementBorrowed, TraversalFlags::empty() }; debug_assert!(element.has_current_styles_for_traversal(&*data, flags), - "Resolving style on element without current styles"); + "Resolving style on {:?} without current styles: {:?}", element, data); data.styles.primary().clone().into() } From 689ee54db30e4c10827f82516566a5abb8bb5ef9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 22 Jul 2017 01:34:47 +0200 Subject: [PATCH 3/7] stylo: Don't try to do an animation-only restyle if we're styling newly-inserted content. It makes just no sense, and I'd rather not complicate the logic in the traversal implementations, seems easy to just handle it here. MozReview-Commit-ID: 3PnT2Jyta8g --- ports/geckolib/glue.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 99e6ef6d259..0026c7212e8 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -282,13 +282,19 @@ pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed, _ => panic!("invalid combination of TraversalRootBehavior and TraversalRestyleBehavior"), }; - let needs_animation_only_restyle = element.has_animation_only_dirty_descendants() || - element.has_animation_restyle_hints(); - if needs_animation_only_restyle { - traverse_subtree(element, - raw_data, - traversal_flags | ANIMATION_ONLY, - unsafe { &*snapshots }); + // It makes no sense to do an animation restyle when we're restyling + // newly-inserted content. + if !traversal_flags.contains(UNSTYLED_CHILDREN_ONLY) { + let needs_animation_only_restyle = + element.has_animation_only_dirty_descendants() || + element.has_animation_restyle_hints(); + + if needs_animation_only_restyle { + traverse_subtree(element, + raw_data, + traversal_flags | ANIMATION_ONLY, + unsafe { &*snapshots }); + } } if restyle_behavior == Restyle::ForThrottledAnimationFlush { From 55d9151dd3298618355dfc4387559a451b92aa03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 22 Jul 2017 14:38:36 +0200 Subject: [PATCH 4/7] style: Minor reformatting. MozReview-Commit-ID: KVCfX3LqccF --- components/style/data.rs | 24 ++++++++++++++---------- components/style/dom.rs | 8 +++++--- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index 47cf4c8a8af..faf4b0fc15f 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -305,9 +305,10 @@ impl ElementData { /// Returns the kind of restyling that we're going to need to do on this /// element, based of the stored restyle hint. - pub fn restyle_kind(&self, - shared_context: &SharedStyleContext) - -> RestyleKind { + pub fn restyle_kind( + &self, + shared_context: &SharedStyleContext + ) -> RestyleKind { if shared_context.traversal_flags.for_animation_only() { return self.restyle_kind_for_animation(shared_context); } @@ -335,9 +336,10 @@ impl ElementData { } /// Returns the kind of restyling for animation-only restyle. - pub fn restyle_kind_for_animation(&self, - shared_context: &SharedStyleContext) - -> RestyleKind { + fn restyle_kind_for_animation( + &self, + shared_context: &SharedStyleContext, + ) -> RestyleKind { debug_assert!(shared_context.traversal_flags.for_animation_only()); debug_assert!(self.has_styles(), "Unstyled element shouldn't be traversed during \ @@ -350,8 +352,8 @@ impl ElementData { if hint.has_animation_hint() { return RestyleKind::CascadeWithReplacements(hint & RestyleHint::for_animations()); } - return RestyleKind::CascadeOnly; + return RestyleKind::CascadeOnly; } /// Return true if important rules are different. @@ -362,9 +364,11 @@ impl ElementData { /// the check which properties do they want. /// If it costs too much, get_properties_overriding_animations() should return a set /// containing only opacity and transform properties. - pub fn important_rules_are_different(&self, - rules: &StrongRuleNode, - guards: &StylesheetGuards) -> bool { + pub fn important_rules_are_different( + &self, + rules: &StrongRuleNode, + guards: &StylesheetGuards + ) -> bool { debug_assert!(self.has_styles()); let (important_rules, _custom) = self.styles.primary().rules().get_properties_overriding_animations(&guards); diff --git a/components/style/dom.rs b/components/style/dom.rs index a7f68a96541..bf62e0f532b 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -480,9 +480,11 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone + } /// Returns whether the element's styles are up-to-date for |traversal_flags|. - fn has_current_styles_for_traversal(&self, - data: &ElementData, - traversal_flags: TraversalFlags) -> bool { + fn has_current_styles_for_traversal( + &self, + data: &ElementData, + traversal_flags: TraversalFlags, + ) -> bool { if traversal_flags.for_animation_only() { // In animation-only restyle we never touch snapshots and don't // care about them. But we can't assert '!self.handled_snapshot()' From 9d21f9e6cebb932e973b5813a138bb96528b562d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 22 Jul 2017 14:53:57 +0200 Subject: [PATCH 5/7] style: Remove (mostly) unused has_current_styles. MozReview-Commit-ID: DV9HfvbUjBY --- components/style/dom.rs | 9 --------- components/style/style_resolver.rs | 12 +----------- components/style/traversal.rs | 2 +- 3 files changed, 2 insertions(+), 21 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index bf62e0f532b..1dc01d8ee99 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -470,15 +470,6 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone + /// Flags this element as having handled already its snapshot. unsafe fn set_handled_snapshot(&self); - /// Returns whether the element's styles are up-to-date. - fn has_current_styles(&self, data: &ElementData) -> bool { - if self.has_snapshot() && !self.handled_snapshot() { - return false; - } - - data.has_styles() && !data.has_invalidations() - } - /// Returns whether the element's styles are up-to-date for |traversal_flags|. fn has_current_styles_for_traversal( &self, diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 204f6021907..306aba45e19 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -52,17 +52,7 @@ where { let parent_el = element.inheritance_parent(); let parent_data = parent_el.as_ref().and_then(|e| e.borrow_data()); - let parent_style = parent_data.as_ref().map(|d| { - // Sometimes Gecko eagerly styles things without processing - // pending restyles first. In general we'd like to avoid this, - // but there can be good reasons (for example, needing to - // construct a frame for some small piece of newly-added - // content in order to do something specific with that frame, - // but not wanting to flush all of layout). - debug_assert!(cfg!(feature = "gecko") || - parent_el.unwrap().has_current_styles(d)); - d.styles.primary() - }); + let parent_style = parent_data.as_ref().map(|d| d.styles.primary()); let mut layout_parent_el = parent_el.clone(); let layout_parent_data; diff --git a/components/style/traversal.rs b/components/style/traversal.rs index a3495fac924..42aba19c30a 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -376,7 +376,7 @@ pub trait DomTraversal : Sync { ) -> bool { // See the comment on `cascade_node` for why we allow this on Gecko. debug_assert!(cfg!(feature = "gecko") || - parent.has_current_styles(parent_data)); + parent.has_current_styles_for_traversal(parent_data, context.shared.traversal_flags)); // If the parent computed display:none, we don't style the subtree. if parent_data.styles.is_display_none() { From f2fe3facf338404b58ce5f505890c1d171b53e77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 22 Jul 2017 14:58:07 +0200 Subject: [PATCH 6/7] style: Update has_current_styles_for_this_traversal to not look at animation hints in non-animation traversals. MozReview-Commit-ID: 4bwwIGcoXqA --- components/style/data.rs | 7 ------- components/style/dom.rs | 2 +- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index faf4b0fc15f..21d35252304 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -298,11 +298,6 @@ impl ElementData { self.styles.primary.is_some() } - /// Returns whether we have any outstanding style invalidation. - pub fn has_invalidations(&self) -> bool { - self.restyle.hint.has_self_invalidations() - } - /// Returns the kind of restyling that we're going to need to do on this /// element, based of the stored restyle hint. pub fn restyle_kind( @@ -313,8 +308,6 @@ impl ElementData { return self.restyle_kind_for_animation(shared_context); } - debug_assert!(!self.has_styles() || self.has_invalidations(), - "Should've stopped earlier"); if !self.has_styles() { return RestyleKind::MatchAndCascade; } diff --git a/components/style/dom.rs b/components/style/dom.rs index 1dc01d8ee99..4b69a8e2c8a 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -492,7 +492,7 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone + return false; } - data.has_styles() && !data.has_invalidations() + data.has_styles() && !data.restyle.hint.has_non_animation_hint() } /// Flags an element and its ancestors with a given `DescendantsBit`. From 6e3397b9077efc290350ac43e0b159aa8580d526 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 24 Jul 2017 10:07:39 +0200 Subject: [PATCH 7/7] style: Remove outdated comments. --- components/style/traversal.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 42aba19c30a..1f917aaee2a 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -374,7 +374,6 @@ pub trait DomTraversal : Sync { parent: E, parent_data: &ElementData, ) -> bool { - // See the comment on `cascade_node` for why we allow this on Gecko. debug_assert!(cfg!(feature = "gecko") || parent.has_current_styles_for_traversal(parent_data, context.shared.traversal_flags));