Bug 1388031 - Cleanup code that was used for verifying styling results for throttled animation flush in post traversal. r?bholley draft
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Fri, 11 Aug 2017 22:00:10 +0900
changeset 644896 0077244ab810ac9c0442ffc74807bea6b30c9f7e
parent 644895 51a147a0666fb5763998a78d1b18f401791ffa91
child 725738 f60c6d2ea4d6e40cb641d0a6a90cbdf671c5b558
push id73577
push userhikezoe@mozilla.com
push dateFri, 11 Aug 2017 13:00:39 +0000
reviewersbholley
bugs1388031, 1383319, 1383001
milestone57.0a1
Bug 1388031 - Cleanup code that was used for verifying styling results for throttled animation flush in post traversal. r?bholley Now that we do process normal traversal even in the case of throttled animation flush so that we don't need to do special handling for the case. Note about the comment in has_current_styles(): the remaining animation hints is not caused by either this patch or the previous patch in this patch series, it's been there in the first place, but it should be fixed somehow later. Below is the tests that leave animation hints when an animating element is moved into subtree of a contenteditable element. layout/style/crashtests/1383319.html layout/style/crashtests/1383001.html MozReview-Commit-ID: 7nYr00Tx9cy
layout/base/ServoRestyleManager.cpp
layout/base/nsCSSFrameConstructor.cpp
layout/style/ServoBindingList.h
layout/style/ServoStyleSet.cpp
layout/style/ServoStyleSet.h
servo/components/style/dom.rs
servo/ports/geckolib/glue.rs
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -417,19 +417,17 @@ ServoRestyleManager::ClearRestyleStateFr
     for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {
       if (n->IsElement()) {
         ClearRestyleStateFromSubtree(n->AsElement());
       }
     }
   }
 
   bool wasRestyled;
-  Unused << Servo_TakeChangeHint(aElement,
-                                 ServoTraversalFlags::Empty,
-                                 &wasRestyled);
+  Unused << Servo_TakeChangeHint(aElement, &wasRestyled);
   aElement->UnsetHasDirtyDescendantsForServo();
   aElement->UnsetHasAnimationOnlyDirtyDescendantsForServo();
   aElement->UnsetFlags(NODE_DESCENDANTS_NEED_FRAMES);
 }
 
 /**
  * This struct takes care of encapsulating some common state that text nodes may
  * need to track during the post-traversal.
@@ -703,19 +701,17 @@ ServoRestyleManager::ProcessPostTraversa
     primaryFrame &&
     primaryFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW);
 
   // Grab the change hint from Servo.
   // In case of flushing throttled animations, any restyle hints other than
   // animations are preserved since they are the hints which will be processed
   // in normal restyle later.
   bool wasRestyled;
-  nsChangeHint changeHint = Servo_TakeChangeHint(aElement,
-                                                 aFlags,
-                                                 &wasRestyled);
+  nsChangeHint changeHint = Servo_TakeChangeHint(aElement, &wasRestyled);
 
   // We should really fix the weird primary frame mapping for image maps
   // (bug 135040)...
   if (styleFrame && styleFrame->GetContent() != aElement) {
     MOZ_ASSERT(styleFrame->IsImageFrame());
     styleFrame = nullptr;
   }
 
@@ -797,17 +793,17 @@ ServoRestyleManager::ProcessPostTraversa
   // other elements without frames).
   ServoRestyleState& childrenRestyleState =
     thisFrameRestyleState ? *thisFrameRestyleState : aRestyleState;
 
   RefPtr<ServoStyleContext> newContext = nullptr;
   if (wasRestyled && oldStyleContext) {
     MOZ_ASSERT(styleFrame || displayContentsStyle);
     newContext =
-      aRestyleState.StyleSet().ResolveServoStyle(aElement, aFlags);
+      aRestyleState.StyleSet().ResolveServoStyle(aElement);
     MOZ_ASSERT(oldStyleContext->ComputedData() != newContext->ComputedData());
 
     newContext->ResolveSameStructsAs(oldStyleContext);
 
     // We want to walk all the continuations here, even the ones with different
     // styles.  In practice, the only reason we get continuations with different
     // styles here is ::first-line (::first-letter never affects element
     // styles).  But in that case, newContext is the right context for the
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -1955,18 +1955,17 @@ nsCSSFrameConstructor::CreateGeneratedCo
     if (!hasServoAnimations) {
       Servo_SetExplicitStyle(container, servoStyle);
     } else {
       // If animations are involved, we avoid the SetExplicitStyle optimization
       // above. We need to grab style with animations from the pseudo element
       // and replace old one.
       mPresShell->StyleSet()->AsServo()->StyleNewSubtree(container);
       pseudoStyleContext =
-        styleSet->AsServo()->ResolveServoStyle(container,
-                                               ServoTraversalFlags::Empty);
+        styleSet->AsServo()->ResolveServoStyle(container);
     }
   } else {
     mozilla::GeckoRestyleManager* geckoRM = RestyleManager()->AsGecko();
     GeckoRestyleManager::ReframingStyleContexts* rsc =
       geckoRM->GetReframingStyleContexts();
 
     if (rsc) {
       RefPtr<GeckoStyleContext> newContext =
--- a/layout/style/ServoBindingList.h
+++ b/layout/style/ServoBindingList.h
@@ -499,22 +499,20 @@ SERVO_BINDING_FUNC(Servo_Initialize, voi
 SERVO_BINDING_FUNC(Servo_Shutdown, void)
 
 // Restyle and change hints.
 SERVO_BINDING_FUNC(Servo_NoteExplicitHints, void, RawGeckoElementBorrowed element,
                    nsRestyleHint restyle_hint, nsChangeHint change_hint)
 SERVO_BINDING_FUNC(Servo_TakeChangeHint,
                    nsChangeHint,
                    RawGeckoElementBorrowed element,
-                   mozilla::ServoTraversalFlags flags,
                    bool* was_restyled)
 SERVO_BINDING_FUNC(Servo_ResolveStyle, ServoStyleContextStrong,
                    RawGeckoElementBorrowed element,
-                   RawServoStyleSetBorrowed set,
-                   mozilla::ServoTraversalFlags flags)
+                   RawServoStyleSetBorrowed set)
 SERVO_BINDING_FUNC(Servo_ResolveStyleAllowStale, ServoStyleContextStrong,
                    RawGeckoElementBorrowed element)
 SERVO_BINDING_FUNC(Servo_ResolvePseudoStyle, ServoStyleContextStrong,
                    RawGeckoElementBorrowed element,
                    mozilla::CSSPseudoElementType pseudo_type,
                    bool is_probe,
                    ServoStyleContextBorrowedOrNull inherited_style,
                    RawServoStyleSetBorrowed set)
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -192,17 +192,17 @@ ServoStyleSet::ResolveStyleFor(Element* 
 {
   RefPtr<ServoStyleContext> computedValues;
   if (aMayCompute == LazyComputeBehavior::Allow) {
     PreTraverseSync();
     return ResolveStyleLazilyInternal(
         aElement, CSSPseudoElementType::NotPseudo, nullptr, aParentContext);
   }
 
-  return ResolveServoStyle(aElement, ServoTraversalFlags::Empty);
+  return ResolveServoStyle(aElement);
 }
 
 /**
  * Clears any stale Servo element data that might existing in the specified
  * element's document.  Upon destruction, asserts that the element and all
  * its ancestors still have no element data, if the document has no pres shell.
  */
 class MOZ_STACK_CLASS AutoClearStaleData
@@ -463,19 +463,17 @@ ServoStyleSet::ResolvePseudoElementStyle
 
   MOZ_ASSERT(aType < CSSPseudoElementType::Count);
 
   RefPtr<ServoStyleContext> computedValues;
 
   if (aPseudoElement) {
     MOZ_ASSERT(aType == aPseudoElement->GetPseudoElementType());
     computedValues =
-      Servo_ResolveStyle(aPseudoElement,
-                         mRawSet.get(),
-                         ServoTraversalFlags::Empty).Consume();
+      Servo_ResolveStyle(aPseudoElement, mRawSet.get()).Consume();
   } else {
     bool cacheable =
       !nsCSSPseudoElements::IsEagerlyCascadedInServo(aType) && aParentContext;
     computedValues =
       cacheable ? aParentContext->GetCachedLazyPseudoStyle(aType) : nullptr;
 
     if (!computedValues) {
       computedValues = Servo_ResolvePseudoStyle(aOriginatingElement,
@@ -1148,23 +1146,21 @@ UpdateBodyTextColorIfNeeded(
   // NOTE(emilio): We do the ComputedData() dance to avoid triggering the
   // IsInServoTraversal() assertion in StyleColor(), which seems useful enough
   // in the general case, I guess...
   aPresContext.SetBodyTextColor(
       aStyleContext.ComputedData()->GetStyleColor()->mColor);
 }
 
 already_AddRefed<ServoStyleContext>
-ServoStyleSet::ResolveServoStyle(Element* aElement, ServoTraversalFlags aFlags)
+ServoStyleSet::ResolveServoStyle(Element* aElement)
 {
   UpdateStylistIfNeeded();
   RefPtr<ServoStyleContext> result =
-    Servo_ResolveStyle(aElement,
-                       mRawSet.get(),
-                       aFlags).Consume();
+    Servo_ResolveStyle(aElement, mRawSet.get()).Consume();
   UpdateBodyTextColorIfNeeded(*aElement, *result, *mPresContext);
   return result.forget();
 }
 
 void
 ServoStyleSet::ClearNonInheritingStyleContexts()
 {
   for (RefPtr<ServoStyleContext>& ptr : mNonInheritingStyleContexts) {
--- a/layout/style/ServoStyleSet.h
+++ b/layout/style/ServoStyleSet.h
@@ -344,18 +344,17 @@ public:
   void CompatibilityModeChanged();
 
   /**
    * Resolve style for the given element, and return it as a
    * ServoStyleContext.
    *
    * FIXME(emilio): Is there a point in this after bug 1367904?
    */
-  already_AddRefed<ServoStyleContext>
-  ResolveServoStyle(dom::Element* aElement, ServoTraversalFlags aFlags);
+  already_AddRefed<ServoStyleContext> ResolveServoStyle(dom::Element* aElement);
 
   bool GetKeyframesForName(const nsString& aName,
                            const nsTimingFunction& aTimingFunction,
                            nsTArray<Keyframe>& aKeyframes);
 
   nsTArray<ComputedKeyframeValues>
   GetComputedKeyframeValuesFor(const nsTArray<Keyframe>& aKeyframes,
                                dom::Element* aElement,
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -491,16 +491,31 @@ pub trait TElement : Eq + PartialEq + De
 
         if self.has_snapshot() && !self.handled_snapshot() {
             return false;
         }
 
         data.has_styles() && !data.restyle.hint.has_non_animation_invalidations()
     }
 
+    /// Returns whether the element's styles are up-to-date after traversal
+    /// (i.e. in post traversal).
+    fn has_current_styles(&self, data: &ElementData) -> bool {
+        if self.has_snapshot() && !self.handled_snapshot() {
+            return false;
+        }
+
+        data.has_styles() &&
+        // TODO(hiro): When an animating element moved into subtree of
+        // contenteditable element, there remains animation restyle hints in
+        // post traversal. It's generally harmless since the hints will be
+        // processed in a next styling but ideally it should be processed soon.
+        !data.restyle.hint.has_non_animation_invalidations()
+    }
+
     /// Flags an element and its ancestors with a given `DescendantsBit`.
     ///
     /// TODO(emilio): We call this conservatively from restyle_element_internal
     /// because we never flag unstyled stuff. A different setup for this may be
     /// a bit cleaner, but it's probably not worth to invest on it right now
     /// unless necessary.
     unsafe fn note_descendants<B: DescendantsBit<Self>>(&self);
 
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -2770,77 +2770,53 @@ pub extern "C" fn Servo_CSSSupports(cond
 pub extern "C" fn Servo_NoteExplicitHints(element: RawGeckoElementBorrowed,
                                           restyle_hint: nsRestyleHint,
                                           change_hint: nsChangeHint) {
     GeckoElement(element).note_explicit_hints(restyle_hint, change_hint);
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_TakeChangeHint(element: RawGeckoElementBorrowed,
-                                       raw_flags: ServoTraversalFlags,
                                        was_restyled: *mut bool) -> nsChangeHint
 {
-    let flags = TraversalFlags::from_bits_truncate(raw_flags);
     let mut was_restyled =  unsafe { was_restyled.as_mut().unwrap() };
     let element = GeckoElement(element);
 
     let damage = match element.mutate_data() {
         Some(mut data) => {
             *was_restyled = data.restyle.is_restyle();
 
             let damage = data.restyle.damage;
-            if flags.for_animation_only() {
-                if !*was_restyled {
-                    // Don't touch elements if the element was not restyled
-                    // in throttled animation flush.
-                    debug!("Skip post traversal for throttled animation flush {:?} restyle={:?}",
-                           element, data.restyle);
-                    return nsChangeHint(0);
-                }
-                // In the case where we call this function for post traversal for
-                // flusing throttled animations (i.e. without normal restyle
-                // traversal), we need to preserve restyle hints for normal restyle
-                // traversal. Restyle hints for animations have been already
-                // removed during animation-only traversal.
-                debug_assert!(!data.restyle.hint.has_animation_hint(),
-                              "Animation restyle hints should have been already removed");
-                data.clear_restyle_flags_and_damage();
-            } else {
-                data.clear_restyle_state();
-            }
+            data.clear_restyle_state();
             damage
         }
         None => {
             warn!("Trying to get change hint from unstyled element");
             *was_restyled = false;
             GeckoRestyleDamage::empty()
         }
     };
 
     debug!("Servo_TakeChangeHint: {:?}, damage={:?}", element, damage);
     damage.as_change_hint()
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_ResolveStyle(element: RawGeckoElementBorrowed,
-                                     _raw_data: RawServoStyleSetBorrowed,
-                                     raw_flags: ServoTraversalFlags)
+                                     _raw_data: RawServoStyleSetBorrowed)
                                      -> ServoStyleContextStrong
 {
-    let flags = TraversalFlags::from_bits_truncate(raw_flags);
     let element = GeckoElement(element);
     debug!("Servo_ResolveStyle: {:?}", element);
     let data =
         element.borrow_data().expect("Resolving style on unstyled element");
 
     // TODO(emilio): Downgrade to debug assertions when close to release.
     assert!(data.has_styles(), "Resolving style on unstyled element");
-    // In the case where we process for throttled animation, there remaings
-    // restyle hints other than animation hints.
-    debug_assert!(element.has_current_styles_for_traversal(&*data, flags),
+    debug_assert!(element.has_current_styles(&*data),
                   "Resolving style on {:?} without current styles: {:?}", element, data);
     data.styles.primary().clone().into()
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_ResolveStyleAllowStale(element: RawGeckoElementBorrowed)
                                                -> ServoStyleContextStrong
 {