Split active animation interpolation storage between standard and custom properties

third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp

Index: third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
diff --git a/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp b/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
index 75d8f29d3b6dbb1e5457ebc18654887571c8f331..49dfbd07895f57daba370230576f2fd1a04669f9 100644
--- a/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
+++ b/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
@@ -433,12 +433,15 @@ void CSSAnimations::SnapshotCompositorKeyframes(
 }
 
 void CSSAnimations::MaybeApplyPendingUpdate(Element* element) {
-  previous_active_interpolations_for_animations_.clear();
+  previous_active_interpolations_for_custom_animations_.clear();
+  previous_active_interpolations_for_standard_animations_.clear();
   if (pending_update_.IsEmpty())
     return;
 
-  previous_active_interpolations_for_animations_.swap(
-      pending_update_.ActiveInterpolationsForAnimations());
+  previous_active_interpolations_for_custom_animations_.swap(
+      pending_update_.ActiveInterpolationsForCustomAnimations());
+  previous_active_interpolations_for_standard_animations_.swap(
+      pending_update_.ActiveInterpolationsForStandardAnimations());
 
   // FIXME: cancelling, pausing, unpausing animations all query
   // compositingState, which is not necessarily up to date here
@@ -636,12 +639,23 @@ void CSSAnimations::CalculateTransitionUpdateForProperty(
   // FIXME: We should transition if an !important property changes even when an
   // animation is running, but this is a bit hard to do with the current
   // applyMatchedProperties system.
-  if (state.update.ActiveInterpolationsForAnimations().Contains(property) ||
-      (state.animating_element->GetElementAnimations() &&
-       state.animating_element->GetElementAnimations()
-           ->CssAnimations()
-           .previous_active_interpolations_for_animations_.Contains(
-               property))) {
+  if (property.IsCSSCustomProperty()) {
+    if (state.update.ActiveInterpolationsForCustomAnimations().Contains(
+            property) ||
+        (state.animating_element->GetElementAnimations() &&
+         state.animating_element->GetElementAnimations()
+             ->CssAnimations()
+             .previous_active_interpolations_for_custom_animations_.Contains(
+                 property))) {
+      return;
+    }
+  } else if (state.update.ActiveInterpolationsForStandardAnimations().Contains(
+                 property) ||
+             (state.animating_element->GetElementAnimations() &&
+              state.animating_element->GetElementAnimations()
+                  ->CssAnimations()
+                  .previous_active_interpolations_for_standard_animations_
+                  .Contains(property))) {
     return;
   }
 
@@ -935,6 +949,10 @@ void CSSAnimations::Cancel() {
   ClearPendingUpdate();
 }
 
+static bool IsCustomPropertyHandle(const PropertyHandle& property) {

[suzyh_UTC10 (ex-contributor), 2017/05/26 01:56:00]

If this is not inside the CSSAnimations class, why is it static?

[alancutter (OOO until 2018), 2017/05/26 03:09:14]

If you don't make it static then it becomes available for other compilation
units to link to. This function is only meant to be available to this file.

[suzyh_UTC10 (ex-contributor), 2017/05/26 04:02:09]

I thought without having it in the header then that wouldn't be an issue.
Anyway, I think an anonymous namespace is the recommended approach in the style
guide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

If you make this change, remember to do the same with the other static functions
in the file for consistency.

+  return property.IsCSSCustomProperty();
+}
+
 // TODO(alancutter): CSS properties and presentation attributes may have
 // identical effects. By grouping them in the same set we introduce a bug where
 // arbitrary hash iteration will determine the order the apply in and thus which
@@ -942,9 +960,9 @@ void CSSAnimations::Cancel() {
 // the case of effect collisions.
 // Example: Both 'color' and 'svg-color' set the color on ComputedStyle but are
 // considered distinct properties in the ActiveInterpolationsMap.
-static bool IsStylePropertyHandle(const PropertyHandle& property_handle) {
-  return property_handle.IsCSSProperty() ||
-         property_handle.IsPresentationAttribute();
+static bool IsStandardPropertyHandle(const PropertyHandle& property) {
+  return (property.IsCSSProperty() && !property.IsCSSCustomProperty()) ||
+         property.IsPresentationAttribute();
 }
 
 void CSSAnimations::CalculateAnimationActiveInterpolations(
@@ -955,14 +973,30 @@ void CSSAnimations::CalculateAnimationActiveInterpolations(
   EffectStack* effect_stack =
       element_animations ? &element_animations->GetEffectStack() : nullptr;
 
+  const auto& adoptActiveInterpolations =

[suzyh_UTC10 (ex-contributor), 2017/05/26 01:56:00]

I find this difficult to read. What do you see as the major disadvantages of
defining it as a separate helper function above?

[alancutter (OOO until 2018), 2017/05/26 03:09:14]

I don't see any major advantages to it, just minor ones. Changed.

+      [effect_stack, &update](
+          const HeapVector<Member<const InertEffect>>* new_animations,
+          const HeapHashSet<Member<const Animation>>* suppressed_animations) {
+        ActiveInterpolationsMap custom_interpolations(
+            EffectStack::ActiveInterpolations(

[suzyh_UTC10 (ex-contributor), 2017/05/26 01:56:00]

Would this be simplified if EffectStack knew about custom properties vs standard
properties, or is that a distinction we wish to keep out of EffectStack?

[alancutter (OOO until 2018), 2017/05/26 03:09:14]

It's a distiction that would be out of place in EffectStack (it deals with SVG
animations as well) and I don't think it would make this code much easier to
read, they still need to be stored in separate places on the update object.

+                effect_stack, new_animations, suppressed_animations,
+                KeyframeEffectReadOnly::kDefaultPriority,
+                IsCustomPropertyHandle));
+        update.AdoptActiveInterpolationsForCustomAnimations(
+            custom_interpolations);
+
+        ActiveInterpolationsMap standard_interpolations(
+            EffectStack::ActiveInterpolations(
+                effect_stack, new_animations, suppressed_animations,
+                KeyframeEffectReadOnly::kDefaultPriority,
+                IsStandardPropertyHandle));
+        update.AdoptActiveInterpolationsForStandardAnimations(
+            standard_interpolations);
+      };
+
   if (update.NewAnimations().IsEmpty() &&
       update.SuppressedAnimations().IsEmpty()) {
-    ActiveInterpolationsMap active_interpolations_for_animations(
-        EffectStack::ActiveInterpolations(
-            effect_stack, nullptr, nullptr,
-            KeyframeEffectReadOnly::kDefaultPriority, IsStylePropertyHandle));
-    update.AdoptActiveInterpolationsForAnimations(
-        active_interpolations_for_animations);
+    adoptActiveInterpolations(nullptr, nullptr);
     return;
   }
 
@@ -974,29 +1008,16 @@ void CSSAnimations::CalculateAnimationActiveInterpolations(
   for (const auto& updated_animation : update.AnimationsWithUpdates())
     new_effects.push_back(updated_animation.effect);
 
-  ActiveInterpolationsMap active_interpolations_for_animations(
-      EffectStack::ActiveInterpolations(
-          effect_stack, &new_effects, &update.SuppressedAnimations(),
-          KeyframeEffectReadOnly::kDefaultPriority, IsStylePropertyHandle));
-  update.AdoptActiveInterpolationsForAnimations(
-      active_interpolations_for_animations);
+  adoptActiveInterpolations(&new_effects, &update.SuppressedAnimations());
 }
 
-static bool IsCustomStylePropertyHandle(const PropertyHandle& property) {
-  return property.IsCSSCustomProperty();
-}
-
-static bool IsStandardStylePropertyHandle(const PropertyHandle& property) {
-  return IsStylePropertyHandle(property) && !property.IsCSSCustomProperty();
-}
-
-static EffectStack::PropertyHandleFilter StylePropertyFilter(
+static EffectStack::PropertyHandleFilter PropertyFilter(
     CSSAnimations::PropertyPass property_pass) {
   if (property_pass == CSSAnimations::PropertyPass::kCustom) {
-    return IsCustomStylePropertyHandle;
+    return IsCustomPropertyHandle;
   }
   DCHECK_EQ(property_pass, CSSAnimations::PropertyPass::kStandard);
-  return IsStandardStylePropertyHandle;
+  return IsStandardPropertyHandle;
 }
 
 void CSSAnimations::CalculateTransitionActiveInterpolations(
@@ -1014,7 +1035,7 @@ void CSSAnimations::CalculateTransitionActiveInterpolations(
     active_interpolations_for_transitions = EffectStack::ActiveInterpolations(
         effect_stack, nullptr, nullptr,
         KeyframeEffectReadOnly::kTransitionPriority,
-        StylePropertyFilter(property_pass));
+        PropertyFilter(property_pass));
   } else {
     HeapVector<Member<const InertEffect>> new_transitions;
     for (const auto& entry : update.NewTransitions())
@@ -1035,14 +1056,18 @@ void CSSAnimations::CalculateTransitionActiveInterpolations(
     active_interpolations_for_transitions = EffectStack::ActiveInterpolations(
         effect_stack, &new_transitions, &cancelled_animations,
         KeyframeEffectReadOnly::kTransitionPriority,
-        StylePropertyFilter(property_pass));
+        PropertyFilter(property_pass));
   }
 
+  const ActiveInterpolationsMap& animations =
+      property_pass == PropertyPass::kCustom
+          ? update.ActiveInterpolationsForCustomAnimations()
+          : update.ActiveInterpolationsForStandardAnimations();
   // Properties being animated by animations don't get values from transitions
   // applied.
-  if (!update.ActiveInterpolationsForAnimations().IsEmpty() &&
+  if (!animations.IsEmpty() &&
       !active_interpolations_for_transitions.IsEmpty()) {
-    for (const auto& entry : update.ActiveInterpolationsForAnimations())
+    for (const auto& entry : animations)
       active_interpolations_for_transitions.erase(entry.key);
   }
 
@@ -1218,11 +1243,6 @@ bool CSSAnimations::IsAffectedByKeyframesFromScope(
   return ToShadowRoot(tree_scope.RootNode()).host() == element;
 }
 
-bool CSSAnimations::IsCustomPropertyHandle(const PropertyHandle& property) {
-  return property.IsCSSProperty() &&
-         property.CssProperty() == CSSPropertyVariable;
-}
-
 bool CSSAnimations::IsAnimatingCustomProperties(
     const ElementAnimations* element_animations) {
   return element_animations &&