Bug 1253476 - Use update() to update declarations from Servo_DeclarationBlock_SetPropertyToAnimationValue; r=emilio

This method is used when updating the SMIL override style and from Web
Animations' Animation.commitStyles method. By using update we accurately return
false when no change is made to a declaration block.

For SMIL this simply acts as an optimization, meaning we can avoid updating the
SMIL override style ub some cases.

For Animation.commitStyles, however, this allows us to avoid generating
a mutation record. Normally making a redundant change to an attribute *does*
generate a mutation record but the style attribute is different. All browsers
avoid generating a mutation record for a redundant change to inline style.
This is specified in the behavior for setProperty[1] which does not update the
style attribute if updated is false.

[1] https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty

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

--HG--
extra : moz-landing-system : lando

dom/animation/Animation.cpp

@@ -689,6 +689,15 @@ void Animation::CommitStyles(ErrorResult& aRv) {
     declarationBlock->SetDirty();
   }
 
+  // Prepare the callback
+  MutationClosureData closureData;
+  closureData.mClosure = nsDOMCSSAttributeDeclaration::MutationClosureFunction;
+  closureData.mElement = target->mElement;
+  DeclarationBlockMutationClosure beforeChangeClosure = {
+      nsDOMCSSAttributeDeclaration::MutationClosureFunction,
+      &closureData,
+  };
+
   // Set the animated styles
   bool changed = false;
   nsCSSPropertyIDSet properties = keyframeEffect->GetPropertySet();
@@ -698,7 +707,7 @@ void Animation::CommitStyles(ErrorResult& aRv) {
             .Consume();
     if (computedValue) {
       changed |= Servo_DeclarationBlock_SetPropertyToAnimationValue(
-          declarationBlock->Raw(), computedValue);
+          declarationBlock->Raw(), computedValue, beforeChangeClosure);
     }
   }
 
@@ -707,11 +716,6 @@ void Animation::CommitStyles(ErrorResult& aRv) {
   }
 
   // Update inline style declaration
-  MutationClosureData closureData;
-  closureData.mClosure = nsDOMCSSAttributeDeclaration::MutationClosureFunction;
-  closureData.mElement = target->mElement;
-
-  target->mElement->InlineStyleDeclarationWillChange(closureData);
   target->mElement->SetInlineStyleDeclaration(*declarationBlock, closureData);
 }
 

dom/smil/SMILCSSValueType.cpp

@@ -516,8 +516,8 @@ bool SMILCSSValueType::SetPropertyValues(const SMILValue& aValue,
 
   bool changed = false;
   for (const auto& value : wrapper->mServoValues) {
-    changed |=
-        Servo_DeclarationBlock_SetPropertyToAnimationValue(aDecl.Raw(), value);
+    changed |= Servo_DeclarationBlock_SetPropertyToAnimationValue(aDecl.Raw(),
+                                                                  value, {});
   }
 
   return changed;

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

@@ -2337,6 +2337,14 @@ impl SourcePropertyDeclaration {
         }
     }
 
+    /// Create one with a single PropertyDeclaration.
+    #[inline]
+    pub fn with_one(decl: PropertyDeclaration) -> Self {
+        let mut result = Self::new();
+        result.declarations.push(decl);
+        result
+    }
+
     /// Similar to Vec::drain: leaves this empty when the return value is dropped.
     pub fn drain(&mut self) -> SourcePropertyDeclarationDrain {
         SourcePropertyDeclarationDrain {

servo/ports/geckolib/glue.rs

@@ -4121,6 +4121,28 @@ pub unsafe extern "C" fn Servo_DeclarationBlock_GetPropertyIsImportant(
     })
 }
 
+#[inline(always)]
+fn set_property_to_declarations(
+    block: &RawServoDeclarationBlock,
+    parsed_declarations: &mut SourcePropertyDeclaration,
+    before_change_closure: DeclarationBlockMutationClosure,
+    importance: Importance,
+) -> bool {
+    let mut updates = Default::default();
+    let will_change = read_locked_arc(block, |decls: &PropertyDeclarationBlock| {
+        decls.prepare_for_update(&parsed_declarations, importance, &mut updates)
+    });
+    if !will_change {
+        return false;
+    }
+
+    before_change_closure.invoke();
+    write_locked_arc(block, |decls: &mut PropertyDeclarationBlock| {
+        decls.update(parsed_declarations.drain(), importance, &mut updates)
+    });
+    true
+}
+
 fn set_property(
     declarations: &RawServoDeclarationBlock,
     property_id: PropertyId,
@@ -4153,19 +4175,13 @@ fn set_property(
     } else {
         Importance::Normal
     };
-    let mut updates = Default::default();
-    let will_change = read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| {
-        decls.prepare_for_update(&source_declarations, importance, &mut updates)
-    });
-    if !will_change {
-        return false;
-    }
 
-    before_change_closure.invoke();
-    write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.update(source_declarations.drain(), importance, &mut updates)
-    });
-    true
+    set_property_to_declarations(
+        declarations,
+        &mut source_declarations,
+        before_change_closure,
+        importance,
+    )
 }
 
 #[no_mangle]
@@ -4197,13 +4213,17 @@ pub unsafe extern "C" fn Servo_DeclarationBlock_SetProperty(
 pub unsafe extern "C" fn Servo_DeclarationBlock_SetPropertyToAnimationValue(
     declarations: &RawServoDeclarationBlock,
     animation_value: &RawServoAnimationValue,
+    before_change_closure: DeclarationBlockMutationClosure,
 ) -> bool {
-    write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(
-            AnimationValue::as_arc(&animation_value).uncompute(),
-            Importance::Normal,
-        )
-    })
+    let mut source_declarations =
+        SourcePropertyDeclaration::with_one(AnimationValue::as_arc(&animation_value).uncompute());
+
+    set_property_to_declarations(
+        declarations,
+        &mut source_declarations,
+        before_change_closure,
+        Importance::Normal,
+    )
 }
 
 #[no_mangle]

testing/web-platform/meta/web-animations/interfaces/Animation/commitStyles.html.ini

@@ -1,3 +0,0 @@
-[commitStyles.html]
-  [Does NOT trigger mutation observers when the change to style is redundant]
-    expected: FAIL