Bug 1253476 - Use update() to update declarations from Servo_DeclarationBlock_SetPropertyToAnimationValue; r=emilio
authorBrian Birtles <birtles@gmail.com>
Mon, 20 May 2019 05:22:39 +0000
changeset 474494 f26b8ea63ae373c27411d27fbf087df5efeebdb5
parent 474493 180d65431190005d9607ae4ca4725226e1c8568c
child 474495 26b7854271e17646fc743c6025ebd297b2d2516a
push id36040
push userrgurzau@mozilla.com
push dateMon, 20 May 2019 13:43:21 +0000
treeherdermozilla-central@319a369ccde4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1253476
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 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
dom/animation/Animation.cpp
dom/smil/SMILCSSValueType.cpp
servo/components/style/properties/properties.mako.rs
servo/ports/geckolib/glue.rs
testing/web-platform/meta/web-animations/interfaces/Animation/commitStyles.html.ini
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -684,39 +684,43 @@ void Animation::CommitStyles(ErrorResult
   RefPtr<DeclarationBlock> declarationBlock;
   if (auto* existing = target->mElement->GetInlineStyleDeclaration()) {
     declarationBlock = existing->EnsureMutable();
   } else {
     declarationBlock = new DeclarationBlock();
     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();
   for (nsCSSPropertyID property : properties) {
     RefPtr<RawServoAnimationValue> computedValue =
         Servo_AnimationValueMap_GetValue(animationValues.get(), property)
             .Consume();
     if (computedValue) {
       changed |= Servo_DeclarationBlock_SetPropertyToAnimationValue(
-          declarationBlock->Raw(), computedValue);
+          declarationBlock->Raw(), computedValue, beforeChangeClosure);
     }
   }
 
   if (!changed) {
     return;
   }
 
   // Update inline style declaration
-  MutationClosureData closureData;
-  closureData.mClosure = nsDOMCSSAttributeDeclaration::MutationClosureFunction;
-  closureData.mElement = target->mElement;
-
-  target->mElement->InlineStyleDeclarationWillChange(closureData);
   target->mElement->SetInlineStyleDeclaration(*declarationBlock, closureData);
 }
 
 // ---------------------------------------------------------------------------
 //
 // JS wrappers for Animation interface:
 //
 // ---------------------------------------------------------------------------
--- a/dom/smil/SMILCSSValueType.cpp
+++ b/dom/smil/SMILCSSValueType.cpp
@@ -511,18 +511,18 @@ bool SMILCSSValueType::SetPropertyValues
              "Unexpected SMIL value type");
   const ValueWrapper* wrapper = ExtractValueWrapper(aValue);
   if (!wrapper) {
     return false;
   }
 
   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;
 }
 
 // static
 nsCSSPropertyID SMILCSSValueType::PropertyFromValue(const SMILValue& aValue) {
   if (aValue.mType != &SMILCSSValueType::sSingleton) {
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -2332,16 +2332,24 @@ impl SourcePropertyDeclaration {
     #[inline]
     pub fn new() -> Self {
         SourcePropertyDeclaration {
             declarations: ::arrayvec::ArrayVec::new(),
             all_shorthand: AllShorthand::NotSet,
         }
     }
 
+    /// 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 {
             declarations: self.declarations.drain(..),
             all_shorthand: mem::replace(&mut self.all_shorthand, AllShorthand::NotSet),
         }
     }
 
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -4116,16 +4116,38 @@ pub unsafe extern "C" fn Servo_Declarati
     property: *const nsACString,
 ) -> bool {
     let property_id = get_property_id_from_property!(property, false);
     read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| {
         decls.property_priority(&property_id).important()
     })
 }
 
+#[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,
     value: *const nsACString,
     is_important: bool,
     data: *mut URLExtraData,
     parsing_mode: structs::ParsingMode,
     quirks_mode: QuirksMode,
@@ -4148,29 +4170,23 @@ fn set_property(
         return false;
     }
 
     let importance = if is_important {
         Importance::Important
     } 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]
 pub unsafe extern "C" fn Servo_DeclarationBlock_SetProperty(
     declarations: &RawServoDeclarationBlock,
     property: *const nsACString,
     value: *const nsACString,
     is_important: bool,
@@ -4192,23 +4208,27 @@ pub unsafe extern "C" fn Servo_Declarati
         before_change_closure,
     )
 }
 
 #[no_mangle]
 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]
 pub unsafe extern "C" fn Servo_DeclarationBlock_SetPropertyById(
     declarations: &RawServoDeclarationBlock,
     property: nsCSSPropertyID,
     value: *const nsACString,
     is_important: bool,
deleted file mode 100644
--- a/testing/web-platform/meta/web-animations/interfaces/Animation/commitStyles.html.ini
+++ /dev/null
@@ -1,3 +0,0 @@
-[commitStyles.html]
-  [Does NOT trigger mutation observers when the change to style is redundant]
-    expected: FAIL