Bug 1540220 - Cleanup unused style traversal flags. r=dholbert
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 09 Apr 2019 18:03:41 +0000
changeset 468608 9194b56c65fa97430a2badbb583565e33a1c5275
parent 468607 8cbdae26bf7b3727f7a108fb2af8e5012d07b3d7
child 468609 ae9783be99324502017f3c9bfc92cb811fd1993e
push id35843
push usernbeleuzu@mozilla.com
push dateTue, 09 Apr 2019 22:08:13 +0000
treeherdermozilla-central@a31032a16330 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1540220
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1540220 - Cleanup unused style traversal flags. r=dholbert Some of these were unused, some of them were only used in combination with others, so I've unified them. In particular, Forgetful and ClearAnimationOnlyDirtyDescendants were used only together for a very specific task (the final animation traversal), so I merged them into something that has that name. ClearDirtyBits was unused, so I removed along with some code that would no longer be called. Differential Revision: https://phabricator.services.mozilla.com/D25454
layout/style/ServoStyleSet.cpp
layout/style/ServoTypes.h
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/matching.rs
servo/components/style/traversal.rs
servo/components/style/traversal_flags.rs
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -908,18 +908,18 @@ void ServoStyleSet::StyleNewSubtree(Elem
   // Annoyingly, the newly-styled content may have animations that need
   // starting, which requires traversing them again. Mark the elements
   // that need animation processing, then do a forgetful traversal to
   // update the styles and clear the animation bits.
   if (GetPresContext()->EffectCompositor()->PreTraverseInSubtree(flags,
                                                                  aRoot)) {
     postTraversalRequired = Servo_TraverseSubtree(
         aRoot, mRawSet.get(), &snapshots,
-        ServoTraversalFlags::AnimationOnly | ServoTraversalFlags::Forgetful |
-            ServoTraversalFlags::ClearAnimationOnlyDirtyDescendants);
+        ServoTraversalFlags::AnimationOnly |
+            ServoTraversalFlags::FinalAnimationTraversal);
     MOZ_ASSERT(!postTraversalRequired);
   }
 }
 
 void ServoStyleSet::MarkOriginsDirty(OriginFlags aChangedOrigins) {
   SetStylistStyleSheetsDirty();
   Servo_StyleSet_NoteStyleSheetsChanged(mRawSet.get(), aChangedOrigins);
 }
--- a/layout/style/ServoTypes.h
+++ b/layout/style/ServoTypes.h
@@ -38,27 +38,19 @@ enum class LazyComputeBehavior {
 
 // Various flags for the servo traversal.
 enum class ServoTraversalFlags : uint32_t {
   Empty = 0,
   // Perform animation processing but not regular styling.
   AnimationOnly = 1 << 0,
   // Traverses as normal mode but tries to update all CSS animations.
   ForCSSRuleChanges = 1 << 1,
-  // A forgetful traversal ignores the previous state of the frame tree, and
-  // thus does not compute damage or maintain other state describing the styles
-  // pre-traversal. A forgetful traversal is usually the right thing if you
-  // aren't going to do a post-traversal.
-  Forgetful = 1 << 3,
-  // Clears all the dirty bits (dirty descendants, animation-only
-  // dirty-descendants, needs frame, descendants need frames) on the elements
-  // traversed. in the subtree.
-  ClearDirtyBits = 1 << 5,
-  // Clears only the animation-only dirty descendants bit in the subtree.
-  ClearAnimationOnlyDirtyDescendants = 1 << 6,
+  // The final animation-only traversal, which shouldn't really care about other
+  // style changes anymore.
+  FinalAnimationTraversal = 1 << 2,
   // Allows the traversal to run in parallel if there are sufficient cores on
   // the machine.
   ParallelTraversal = 1 << 7,
   // Flush throttled animations. By default, we only update throttled animations
   // when we have other non-throttled work to do. With this flag, we
   // unconditionally tick and process them.
   FlushThrottledAnimations = 1 << 8,
 };
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -637,25 +637,16 @@ pub trait TElement:
     ///
     /// In Gecko, this corresponds to the regular dirty descendants bit, the
     /// animation-only dirty descendants bit, and the lazy frame construction
     /// descendants bit.
     unsafe fn clear_descendant_bits(&self) {
         self.unset_dirty_descendants();
     }
 
-    /// Clear all element flags related to dirtiness.
-    ///
-    /// In Gecko, this corresponds to the regular dirty descendants bit, the
-    /// animation-only dirty descendants bit, the lazy frame construction bit,
-    /// and the lazy frame construction descendants bit.
-    unsafe fn clear_dirty_bits(&self) {
-        self.unset_dirty_descendants();
-    }
-
     /// Returns true if this element is a visited link.
     ///
     /// Servo doesn't support visited styles yet.
     fn is_visited_link(&self) -> bool {
         false
     }
 
     /// Returns true if this element is in a native anonymous subtree.
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -1418,26 +1418,16 @@ impl<'le> TElement for GeckoElement<'le>
     unsafe fn clear_descendant_bits(&self) {
         self.unset_flags(
             ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
                 ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
                 NODE_DESCENDANTS_NEED_FRAMES as u32,
         )
     }
 
-    #[inline]
-    unsafe fn clear_dirty_bits(&self) {
-        self.unset_flags(
-            ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
-                ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
-                NODE_DESCENDANTS_NEED_FRAMES as u32 |
-                NODE_NEEDS_FRAME as u32,
-        )
-    }
-
     fn is_visited_link(&self) -> bool {
         self.state().intersects(ElementState::IN_VISITED_STATE)
     }
 
     /// This logic is duplicated in Gecko's nsINode::IsInNativeAnonymousSubtree.
     #[inline]
     fn is_in_native_anonymous_subtree(&self) -> bool {
         use crate::gecko_bindings::structs::NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE;
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -466,17 +466,17 @@ trait PrivateMatchMethods: TElement {
         damage: &mut RestyleDamage,
         old_values: &ComputedValues,
         new_values: &ComputedValues,
         pseudo: Option<&PseudoElement>,
     ) -> ChildCascadeRequirement {
         debug!("accumulate_damage_for: {:?}", self);
         debug_assert!(!shared_context
             .traversal_flags
-            .contains(TraversalFlags::Forgetful));
+            .contains(TraversalFlags::FinalAnimationTraversal));
 
         let difference = self.compute_style_difference(old_values, new_values, pseudo);
 
         *damage |= difference.damage;
 
         debug!(" > style difference: {:?}", difference);
 
         // We need to cascade the children in order to ensure the correct
@@ -718,21 +718,21 @@ pub trait MatchMethods: TElement {
                 let device = context.shared.stylist.device();
 
                 // Needed for the "inherit from body" quirk.
                 let text_color = new_primary_style.get_color().clone_color();
                 device.set_body_text_color(text_color);
             }
         }
 
-        // Don't accumulate damage if we're in a forgetful traversal.
+        // Don't accumulate damage if we're in the final animation traversal.
         if context
             .shared
             .traversal_flags
-            .contains(TraversalFlags::Forgetful)
+            .contains(TraversalFlags::FinalAnimationTraversal)
         {
             return ChildCascadeRequirement::MustCascadeChildren;
         }
 
         // Also, don't do anything if there was no style.
         let old_primary_style = match old_styles.primary {
             Some(s) => s,
             None => return ChildCascadeRequirement::MustCascadeChildren,
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -530,50 +530,28 @@ pub fn recalc_style_at<E, D, F>(
     }
 
     // FIXME(bholley): Make these assertions pass for servo.
     if cfg!(feature = "gecko") && cfg!(debug_assertions) && data.styles.is_display_none() {
         debug_assert!(!element.has_dirty_descendants());
         debug_assert!(!element.has_animation_only_dirty_descendants());
     }
 
-    debug_assert!(
-        flags.for_animation_only() ||
-            !flags.contains(TraversalFlags::ClearDirtyBits) ||
-            !element.has_animation_only_dirty_descendants(),
-        "Should have cleared animation bits already"
-    );
     clear_state_after_traversing(element, data, flags);
 }
 
 fn clear_state_after_traversing<E>(element: E, data: &mut ElementData, flags: TraversalFlags)
 where
     E: TElement,
 {
-    // If we are in a forgetful traversal, drop the existing restyle
-    // data here, since we won't need to perform a post-traversal to pick up
-    // any change hints.
-    if flags.contains(TraversalFlags::Forgetful) {
+    if flags.intersects(TraversalFlags::FinalAnimationTraversal) {
+        debug_assert!(flags.for_animation_only());
         data.clear_restyle_flags_and_damage();
-    }
-
-    // Clear dirty bits as appropriate.
-    if flags.for_animation_only() {
-        if flags.intersects(
-            TraversalFlags::ClearDirtyBits | TraversalFlags::ClearAnimationOnlyDirtyDescendants,
-        ) {
-            unsafe {
-                element.unset_animation_only_dirty_descendants();
-            }
-        }
-    } else if flags.contains(TraversalFlags::ClearDirtyBits) {
-        // The animation traversal happens first, so we don't need to guard against
-        // clearing the animation bit on the regular traversal.
         unsafe {
-            element.clear_dirty_bits();
+            element.unset_animation_only_dirty_descendants();
         }
     }
 }
 
 fn compute_style<E>(
     traversal_data: &PerLevelTraversalData,
     context: &mut StyleContext<E>,
     element: E,
--- a/servo/components/style/traversal_flags.rs
+++ b/servo/components/style/traversal_flags.rs
@@ -11,25 +11,19 @@
 bitflags! {
     /// Flags that control the traversal process.
     pub struct TraversalFlags: u32 {
         /// Traverse only elements for animation restyles.
         const AnimationOnly = 1 << 0;
         /// Traverse and update all elements with CSS animations since
         /// @keyframes rules may have changed. Triggered by CSS rule changes.
         const ForCSSRuleChanges = 1 << 1;
-        /// A forgetful traversal ignores the previous state of the frame tree, and
-        /// thus does not compute damage or maintain other state describing the styles
-        /// pre-traversal. A forgetful traversal is usually the right thing if you
-        /// aren't going to do a post-traversal.
-        const Forgetful = 1 << 3;
-        /// Clears all the dirty bits on the elements traversed.
-        const ClearDirtyBits = 1 << 5;
-        /// Clears the animation-only dirty descendants bit in the subtree.
-        const ClearAnimationOnlyDirtyDescendants = 1 << 6;
+        /// The final animation-only traversal, which shouldn't really care about other
+        /// style changes anymore.
+        const FinalAnimationTraversal = 1 << 2;
         /// Allows the traversal to run in parallel if there are sufficient cores on
         /// the machine.
         const ParallelTraversal = 1 << 7;
         /// Flush throttled animations. By default, we only update throttled animations
         /// when we have other non-throttled work to do. With this flag, we
         /// unconditionally tick and process them.
         const FlushThrottledAnimations = 1 << 8;
 
@@ -53,20 +47,17 @@ pub fn assert_traversal_flags_match() {
                 assert_eq!(modes, TraversalFlags::empty(), "all TraversalFlags bits should have an assertion");
             }
         }
     }
 
     check_traversal_flags! {
         ServoTraversalFlags_AnimationOnly => TraversalFlags::AnimationOnly,
         ServoTraversalFlags_ForCSSRuleChanges => TraversalFlags::ForCSSRuleChanges,
-        ServoTraversalFlags_Forgetful => TraversalFlags::Forgetful,
-        ServoTraversalFlags_ClearDirtyBits => TraversalFlags::ClearDirtyBits,
-        ServoTraversalFlags_ClearAnimationOnlyDirtyDescendants =>
-            TraversalFlags::ClearAnimationOnlyDirtyDescendants,
+        ServoTraversalFlags_FinalAnimationTraversal => TraversalFlags::FinalAnimationTraversal,
         ServoTraversalFlags_ParallelTraversal => TraversalFlags::ParallelTraversal,
         ServoTraversalFlags_FlushThrottledAnimations => TraversalFlags::FlushThrottledAnimations,
     }
 }
 
 impl TraversalFlags {
     /// Returns true if the traversal is for animation-only restyles.
     #[inline]