Bug 1760222 - Ignore unchanged property for scroll-linked effect detector. r=hiro

I think this is cleaner.

Differential Revision: https://phabricator.services.mozilla.com/D141737

layout/painting/ActiveLayerTracker.cpp

@@ -266,23 +266,6 @@ void ActiveLayerTracker::NotifyRestyle(nsIFrame* aFrame,
   }
 }
 
-/* static */
-void ActiveLayerTracker::NotifyAnimated(nsIFrame* aFrame,
-                                        nsCSSPropertyID aProperty,
-                                        const nsACString& aNewValue,
-                                        nsDOMCSSDeclaration* aDOMCSSDecl) {
-  LayerActivity* layerActivity = GetLayerActivityForUpdate(aFrame);
-  uint8_t& mutationCount = layerActivity->RestyleCountForProperty(aProperty);
-  if (mutationCount != 0xFF) {
-    nsAutoCString oldValue;
-    aDOMCSSDecl->GetPropertyValue(aProperty, oldValue);
-    if (oldValue != aNewValue) {
-      // We know this is animated, so just hack the mutation count.
-      mutationCount = 0xFF;
-    }
-  }
-}
-
 static bool IsPresContextInScriptAnimationCallback(
     nsPresContext* aPresContext) {
   if (aPresContext->RefreshDriver()->IsInRefresh()) {
@@ -296,10 +279,11 @@ static bool IsPresContextInScriptAnimationCallback(
 
 /* static */
 void ActiveLayerTracker::NotifyInlineStyleRuleModified(
-    nsIFrame* aFrame, nsCSSPropertyID aProperty, const nsACString& aNewValue,
-    nsDOMCSSDeclaration* aDOMCSSDecl) {
+    nsIFrame* aFrame, nsCSSPropertyID aProperty) {
   if (IsPresContextInScriptAnimationCallback(aFrame->PresContext())) {
-    NotifyAnimated(aFrame, aProperty, aNewValue, aDOMCSSDecl);
+    LayerActivity* layerActivity = GetLayerActivityForUpdate(aFrame);
+    // We know this is animated, so just hack the mutation count.
+    layerActivity->RestyleCountForProperty(aProperty) = 0xff;
   }
 }
 

layout/painting/ActiveLayerTracker.h

@@ -45,27 +45,13 @@ class ActiveLayerTracker {
    * @param aProperty the property that has changed
    */
   static void NotifyRestyle(nsIFrame* aFrame, nsCSSPropertyID aProperty);
-  /**
-   * Mark aFrame as being known to have an animation of aProperty.
-   * Any such marking will time out after a short period.
-   * aNewValue and aDOMCSSDecl are used to determine whether the property's
-   * value has changed.
-   */
-  static void NotifyAnimated(nsIFrame* aFrame, nsCSSPropertyID aProperty,
-                             const nsACString& aNewValue,
-                             nsDOMCSSDeclaration* aDOMCSSDecl);
+
   /**
    * Notify that a property in the inline style rule of aFrame's element
    * has been modified.
-   * This notification is incomplete --- not all modifications to inline
-   * style will trigger this.
-   * aNewValue and aDOMCSSDecl are used to determine whether the property's
-   * value has changed.
    */
   static void NotifyInlineStyleRuleModified(nsIFrame* aFrame,
-                                            nsCSSPropertyID aProperty,
-                                            const nsACString& aNewValue,
-                                            nsDOMCSSDeclaration* aDOMCSSDecl);
+                                            nsCSSPropertyID aProperty);
   /**
    * Notify that a frame needs to be repainted. This is important for layering
    * decisions where, say, aFrame's transform is updated from JS, but we need

layout/style/ServoTypes.h

@@ -11,6 +11,7 @@
 
 #include "mozilla/RefPtr.h"
 #include "mozilla/TypedEnumBits.h"
+#include "nsCSSPropertyID.h"
 #include "nsCoord.h"
 #include "X11UndefineNone.h"
 
@@ -140,8 +141,9 @@ class ServoStyleSetSizes {
 // A callback that can be sent via FFI which will be invoked _right before_
 // being mutated, and at most once.
 struct DeclarationBlockMutationClosure {
-  // The callback function. The argument is `data`.
-  void (*function)(void*) = nullptr;
+  // The callback function. The first argument is `data`, the second is the
+  // property id that changed.
+  void (*function)(void*, nsCSSPropertyID) = nullptr;
   void* data = nullptr;
 };
 

layout/style/nsDOMCSSAttrDeclaration.cpp

@@ -12,6 +12,7 @@
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/SVGElement.h"
 #include "mozilla/dom/MutationEventBinding.h"
+#include "mozilla/layers/ScrollLinkedEffectDetector.h"
 #include "mozilla/DeclarationBlock.h"
 #include "mozilla/InternalMutationEvent.h"
 #include "mozilla/SMILCSSValueType.h"
@@ -187,32 +188,67 @@ nsresult nsDOMCSSAttributeDeclaration::SetSMILValue(
   });
 }
 
+// Scripted modifications to style.opacity or style.transform (or other
+// transform-like properties, e.g. style.translate, style.rotate, style.scale)
+// could immediately force us into the animated state if heuristics suggest
+// this is a scripted animation.
+//
+// FIXME: This is missing the margin shorthand and the logical versions of
+// the margin properties, see bug 1266287.
+static bool IsActiveLayerProperty(nsCSSPropertyID aPropID) {
+  switch (aPropID) {
+    case eCSSProperty_opacity:
+    case eCSSProperty_transform:
+    case eCSSProperty_translate:
+    case eCSSProperty_rotate:
+    case eCSSProperty_scale:
+    case eCSSProperty_offset_path:
+    case eCSSProperty_offset_distance:
+    case eCSSProperty_offset_rotate:
+    case eCSSProperty_offset_anchor:
+      return true;
+    default:
+      return false;
+  }
+}
+
 void nsDOMCSSAttributeDeclaration::SetPropertyValue(
     const nsCSSPropertyID aPropID, const nsACString& aValue,
     nsIPrincipal* aSubjectPrincipal, ErrorResult& aRv) {
-  // Scripted modifications to style.opacity or style.transform (or other
-  // transform-like properties, e.g. style.translate, style.rotate, style.scale)
-  // could immediately force us into the animated state if heuristics suggest
-  // this is scripted animation.
-  // FIXME: This is missing the margin shorthand and the logical versions of
-  // the margin properties, see bug 1266287.
-  if (aPropID == eCSSProperty_opacity || aPropID == eCSSProperty_transform ||
-      aPropID == eCSSProperty_translate || aPropID == eCSSProperty_rotate ||
-      aPropID == eCSSProperty_scale || aPropID == eCSSProperty_offset_path ||
-      aPropID == eCSSProperty_offset_distance ||
-      aPropID == eCSSProperty_offset_rotate ||
-      aPropID == eCSSProperty_offset_anchor) {
-    nsIFrame* frame = mElement->GetPrimaryFrame();
-    if (frame) {
-      ActiveLayerTracker::NotifyInlineStyleRuleModified(frame, aPropID, aValue,
-                                                        this);
-    }
-  }
   nsDOMCSSDeclaration::SetPropertyValue(aPropID, aValue, aSubjectPrincipal,
                                         aRv);
 }
 
-void nsDOMCSSAttributeDeclaration::MutationClosureFunction(void* aData) {
+static bool IsScrollLinkedEffectiveProperty(const nsCSSPropertyID aPropID) {
+  switch (aPropID) {
+    case eCSSProperty_background_position:
+    case eCSSProperty_background_position_x:
+    case eCSSProperty_background_position_y:
+    case eCSSProperty_transform:
+    case eCSSProperty_translate:
+    case eCSSProperty_rotate:
+    case eCSSProperty_scale:
+    case eCSSProperty_top:
+    case eCSSProperty_left:
+    case eCSSProperty_bottom:
+    case eCSSProperty_right:
+    case eCSSProperty_margin:
+    case eCSSProperty_margin_top:
+    case eCSSProperty_margin_left:
+    case eCSSProperty_margin_bottom:
+    case eCSSProperty_margin_right:
+    case eCSSProperty_margin_inline_start:
+    case eCSSProperty_margin_inline_end:
+    case eCSSProperty_margin_block_start:
+    case eCSSProperty_margin_block_end:
+      return true;
+    default:
+      return false;
+  }
+}
+
+void nsDOMCSSAttributeDeclaration::MutationClosureFunction(
+    void* aData, nsCSSPropertyID aPropID) {
   auto* data = static_cast<MutationClosureData*>(aData);
   MOZ_ASSERT(
       data->mShouldBeCalled,
@@ -220,6 +256,15 @@ void nsDOMCSSAttributeDeclaration::MutationClosureFunction(void* aData) {
   if (data->mWasCalled) {
     return;
   }
+  if (IsScrollLinkedEffectiveProperty(aPropID)) {
+    mozilla::layers::ScrollLinkedEffectDetector::PositioningPropertyMutated();
+  }
+  if (IsActiveLayerProperty(aPropID)) {
+    if (nsIFrame* frame = data->mElement->GetPrimaryFrame()) {
+      ActiveLayerTracker::NotifyInlineStyleRuleModified(frame, aPropID);
+    }
+  }
+
   data->mWasCalled = true;
   data->mElement->InlineStyleDeclarationWillChange(*data);
 }

layout/style/nsDOMCSSAttrDeclaration.h

@@ -60,7 +60,7 @@ class nsDOMCSSAttributeDeclaration final : public nsDOMCSSDeclaration {
                         nsIPrincipal* aSubjectPrincipal,
                         mozilla::ErrorResult& aRv) override;
 
-  static void MutationClosureFunction(void* aData);
+  static void MutationClosureFunction(void* aData, nsCSSPropertyID);
 
   void GetPropertyChangeClosure(
       mozilla::DeclarationBlockMutationClosure* aClosure,

layout/style/nsDOMCSSDeclaration.cpp

@@ -19,7 +19,6 @@
 #include "mozAutoDocUpdate.h"
 #include "mozilla/dom/BindingUtils.h"
 #include "nsQueryObject.h"
-#include "mozilla/layers/ScrollLinkedEffectDetector.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
@@ -55,33 +54,6 @@ void nsDOMCSSDeclaration::SetPropertyValue(const nsCSSPropertyID aPropID,
     return;
   }
 
-  switch (aPropID) {
-    case eCSSProperty_background_position:
-    case eCSSProperty_background_position_x:
-    case eCSSProperty_background_position_y:
-    case eCSSProperty_transform:
-    case eCSSProperty_translate:
-    case eCSSProperty_rotate:
-    case eCSSProperty_scale:
-    case eCSSProperty_top:
-    case eCSSProperty_left:
-    case eCSSProperty_bottom:
-    case eCSSProperty_right:
-    case eCSSProperty_margin:
-    case eCSSProperty_margin_top:
-    case eCSSProperty_margin_left:
-    case eCSSProperty_margin_bottom:
-    case eCSSProperty_margin_right:
-    case eCSSProperty_margin_inline_start:
-    case eCSSProperty_margin_inline_end:
-    case eCSSProperty_margin_block_start:
-    case eCSSProperty_margin_block_end:
-      mozilla::layers::ScrollLinkedEffectDetector::PositioningPropertyMutated();
-      break;
-    default:
-      break;
-  }
-
   if (aValue.IsEmpty()) {
     // If the new value of the property is an empty string we remove the
     // property.
@@ -136,7 +108,7 @@ void nsDOMCSSDeclaration::SetCssText(const nsACString& aCssText,
   // doesn't modify any existing declaration and that is why the callback isn't
   // called implicitly.
   if (closure.function && !closureData.mWasCalled) {
-    closure.function(&closureData);
+    closure.function(&closureData, eCSSProperty_UNKNOWN);
   }
 
   RefPtr<DeclarationBlock> newdecl = DeclarationBlock::FromCssText(

servo/components/style/properties/properties.mako.rs

@@ -474,9 +474,10 @@ impl NonCustomPropertyId {
         self.0
     }
 
+    /// Convert a `NonCustomPropertyId` into a `nsCSSPropertyID`.
     #[cfg(feature = "gecko")]
     #[inline]
-    fn to_nscsspropertyid(self) -> nsCSSPropertyID {
+    pub fn to_nscsspropertyid(self) -> nsCSSPropertyID {
         // unsafe: guaranteed by static_assert_nscsspropertyid above.
         unsafe { std::mem::transmute(self.0 as i32) }
     }

servo/ports/geckolib/glue.rs

@@ -146,14 +146,18 @@ use style_traits::{CssWriter, ParsingMode, StyleParseErrorKind, ToCss};
 use to_shmem::SharedMemoryBuilder;
 
 trait ClosureHelper {
-    fn invoke(&self);
+    fn invoke(&self, property_id: Option<NonCustomPropertyId>);
 }
 
 impl ClosureHelper for DeclarationBlockMutationClosure {
     #[inline]
-    fn invoke(&self) {
+    fn invoke(&self, property_id: Option<NonCustomPropertyId>) {
         if let Some(function) = self.function.as_ref() {
-            unsafe { function(self.data) };
+            let gecko_prop_id = match property_id {
+                Some(p) => p.to_nscsspropertyid(),
+                None => nsCSSPropertyID::eCSSPropertyExtra_variable,
+            };
+            unsafe { function(self.data, gecko_prop_id) }
         }
     }
 }
@@ -4657,6 +4661,7 @@ pub unsafe extern "C" fn Servo_DeclarationBlock_GetPropertyIsImportant(
 
 #[inline(always)]
 fn set_property_to_declarations(
+    non_custom_property_id: Option<NonCustomPropertyId>,
     block: &RawServoDeclarationBlock,
     parsed_declarations: &mut SourcePropertyDeclaration,
     before_change_closure: DeclarationBlockMutationClosure,
@@ -4670,7 +4675,7 @@ fn set_property_to_declarations(
         return false;
     }
 
-    before_change_closure.invoke();
+    before_change_closure.invoke(non_custom_property_id);
     write_locked_arc(block, |decls: &mut PropertyDeclarationBlock| {
         decls.update(parsed_declarations.drain(), importance, &mut updates)
     });
@@ -4691,6 +4696,7 @@ fn set_property(
 ) -> bool {
     let mut source_declarations = SourcePropertyDeclaration::new();
     let reporter = ErrorReporter::new(ptr::null_mut(), loader, data.ptr());
+    let non_custom_property_id = property_id.non_custom_id();
     let result = parse_property_into(
         &mut source_declarations,
         property_id,
@@ -4714,6 +4720,7 @@ fn set_property(
     };
 
     set_property_to_declarations(
+        non_custom_property_id,
         declarations,
         &mut source_declarations,
         before_change_closure,
@@ -4754,10 +4761,13 @@ pub unsafe extern "C" fn Servo_DeclarationBlock_SetPropertyToAnimationValue(
     animation_value: &RawServoAnimationValue,
     before_change_closure: DeclarationBlockMutationClosure,
 ) -> bool {
+    let animation_value = AnimationValue::as_arc(&animation_value);
+    let non_custom_property_id = animation_value.id().into();
     let mut source_declarations =
-        SourcePropertyDeclaration::with_one(AnimationValue::as_arc(&animation_value).uncompute());
+        SourcePropertyDeclaration::with_one(animation_value.uncompute());
 
     set_property_to_declarations(
+        Some(non_custom_property_id),
         declarations,
         &mut source_declarations,
         before_change_closure,
@@ -4806,7 +4816,7 @@ fn remove_property(
         None => return false,
     };
 
-    before_change_closure.invoke();
+    before_change_closure.invoke(property_id.non_custom_id());
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
         decls.remove_property(&property_id, first_declaration)
     });