Apply custom property animations with CSSVariableAnimator.

Currently, custom property animations are applied in quite a roundabout
way: StyleResolver applies the value assisted by CSSVariableResolver, which
may call into StyleResolver again depending on the variables it sees
during resolution. It has a const_cast. It side-channels a HashSet via
StyleResolverState. It's just not nice.

To fix this, create CSSVariableAnimator which owns the state and
functionality we need to apply the animated properties (in the right
order).

This is a pure refactor. It is already covered well by tests, specifically
by animations/custom-properties/registered-var*.

Change-Id: I6ed4ef2457f990d89930a5c802e01e64ac420174
Reviewed-on: https://chromium-review.googlesource.com/1243087Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Commit-Queue: Anders Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595030}

third_party/blink/renderer/core/css/BUILD.gn

@@ -848,6 +848,8 @@ blink_core_sources("css") {
     "resolver/css_property_priority.h",
     "resolver/css_to_style_map.cc",
     "resolver/css_to_style_map.h",
+    "resolver/css_variable_animator.cc",
+    "resolver/css_variable_animator.h",
     "resolver/css_variable_resolver.cc",
     "resolver/css_variable_resolver.h",
     "resolver/element_resolve_context.cc",

third_party/blink/renderer/core/css/resolver/css_variable_animator.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/css/resolver/css_variable_animator.h"
+
+#include "third_party/blink/renderer/core/animation/css_interpolation_environment.h"
+#include "third_party/blink/renderer/core/animation/css_interpolation_types_map.h"
+#include "third_party/blink/renderer/core/animation/invalidatable_interpolation.h"
+#include "third_party/blink/renderer/core/animation/transition_interpolation.h"
+
+namespace blink {
+
+namespace {
+
+HashSet<PropertyHandle> CollectPending(const CSSAnimationUpdate& update) {
+  HashSet<PropertyHandle> pending;
+  for (const auto& entry : update.ActiveInterpolationsForCustomAnimations())
+    pending.insert(entry.key);
+  for (const auto& entry : update.ActiveInterpolationsForCustomTransitions())
+    pending.insert(entry.key);
+  return pending;
+}
+
+const ActiveInterpolations& ActiveInterpolationsForCustomProperty(
+    const CSSAnimationUpdate& update,
+    const PropertyHandle& property) {
+  // Interpolations will never be found in both animations_map and
+  // transitions_map. This condition is ensured by
+  // CSSAnimations::CalculateTransitionUpdateForProperty().
+  const ActiveInterpolationsMap& animations_map =
+      update.ActiveInterpolationsForCustomAnimations();
+  const ActiveInterpolationsMap& transitions_map =
+      update.ActiveInterpolationsForCustomTransitions();
+  const auto& animation = animations_map.find(property);
+  if (animation != animations_map.end()) {
+    DCHECK_EQ(transitions_map.find(property), transitions_map.end());
+    return animation->value;
+  }
+  const auto& transition = transitions_map.find(property);
+  DCHECK_NE(transition, transitions_map.end());
+  return transition->value;
+}
+
+}  // namespace
+
+CSSVariableAnimator::CSSVariableAnimator(StyleResolverState& state)
+    : CSSVariableResolver(state),
+      state_(state),
+      update_(state.AnimationUpdate()),
+      pending_properties_(CollectPending(update_)) {}
+
+void CSSVariableAnimator::ApplyAll() {
+  while (!pending_properties_.IsEmpty()) {
+    PropertyHandle property = *pending_properties_.begin();
+    Apply(property);
+    DCHECK_EQ(pending_properties_.find(property), pending_properties_.end());
+  }
+}
+
+void CSSVariableAnimator::ApplyAnimation(const AtomicString& name) {
+  PropertyHandle property(name);
+  if (pending_properties_.Contains(property))
+    Apply(property);
+}
+
+void CSSVariableAnimator::Apply(const PropertyHandle& property) {
+  DCHECK(property.IsCSSCustomProperty());
+  DCHECK(pending_properties_.Contains(property));
+  const ActiveInterpolations& interpolations =
+      ActiveInterpolationsForCustomProperty(update_, property);
+  const Interpolation& interpolation = *interpolations.front();
+  if (interpolation.IsInvalidatableInterpolation()) {
+    CSSInterpolationTypesMap map(state_.GetDocument().GetPropertyRegistry(),
+                                 state_.GetDocument());
+    CSSInterpolationEnvironment environment(map, state_, this);
+    InvalidatableInterpolation::ApplyStack(interpolations, environment);
+  } else {
+    ToTransitionInterpolation(interpolation).Apply(state_);
+  }
+  pending_properties_.erase(property);
+}
+
+}  // namespace blink

third_party/blink/renderer/core/css/resolver/css_variable_animator.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_RESOLVER_CSS_VARIABLE_ANIMATOR_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_RESOLVER_CSS_VARIABLE_ANIMATOR_H_
+
+#include "third_party/blink/renderer/core/animation/interpolation.h"
+#include "third_party/blink/renderer/core/animation/property_handle.h"
+#include "third_party/blink/renderer/core/css/resolver/css_variable_resolver.h"
+
+namespace blink {
+
+class StyleResolverState;
+class CSSAnimationUpdate;
+
+// CSSVariableAnimator is a special CSSVariableResolver which can apply
+// animated values during var()-resolution. In other words, it makes sure that
+// if a var()-reference to a currently animating custom property is encountered,
+// we will first apply the animated value for that property before resolving it.
+class CORE_EXPORT CSSVariableAnimator : public CSSVariableResolver {
+  STACK_ALLOCATED();
+
+ public:
+  explicit CSSVariableAnimator(StyleResolverState&);
+
+  // Apply all custom property animations. After calling this, the set of
+  // pending properties will be empty and further calls to ApplyAll will have
+  // no effect.
+  void ApplyAll();
+
+ protected:
+  void ApplyAnimation(const AtomicString&) override;
+
+ private:
+  // Apply the animated value of a single property. The property must exist
+  // in 'pending_properties_'.
+  void Apply(const PropertyHandle&);
+
+  StyleResolverState& state_;
+  const CSSAnimationUpdate& update_;
+  // Set of custom properties with pending animations. We will apply these
+  // one by one until the set is empty.
+  HashSet<PropertyHandle> pending_properties_;
+};
+
+}  // namespace blink
+
+#endif  // CSSVariableAnimator

third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc

@@ -329,20 +329,14 @@ bool CSSVariableResolver::ResolveVariableReference(CSSParserTokenRange range,
       range.ConsumeIncludingWhitespace().Value().ToAtomicString();
   DCHECK(range.AtEnd() || (range.Peek().GetType() == kCommaToken));
 
-  PropertyHandle property(variable_name);
-  if (state_.AnimationPendingCustomProperties().Contains(property) &&
-      !variables_seen_.Contains(variable_name)) {
-    // We make the StyleResolverState mutable for animated custom properties as
-    // an optimisation. Without this we would need to compute animated values on
-    // the stack without saving the result or perform an expensive and complex
-    // value dependency graph analysis to compute them in the required order.
-    StyleResolver::ApplyAnimatedCustomProperty(
-        const_cast<StyleResolverState&>(state_), *this, property);
+  if (!variables_seen_.Contains(variable_name)) {
+    ApplyAnimation(variable_name);
     // Null custom property storage may become non-null after application, we
     // must refresh these cached values.
     inherited_variables_ = state_.Style()->InheritedVariables();
     non_inherited_variables_ = state_.Style()->NonInheritedVariables();
   }
+
   scoped_refptr<CSSVariableData> variable_data =
       is_env_variable ? ValueForEnvironmentVariable(variable_name)
                       : ValueForCustomProperty(variable_name, options);

third_party/blink/renderer/core/css/resolver/css_variable_resolver.h

@@ -33,7 +33,7 @@ class CORE_EXPORT CSSVariableResolver {
   STACK_ALLOCATED();
 
  public:
-  CSSVariableResolver(const StyleResolverState&);
+  explicit CSSVariableResolver(const StyleResolverState&);
 
   scoped_refptr<CSSVariableData> ResolveCustomPropertyAnimationKeyframe(
       const CSSCustomPropertyDeclaration& keyframe,
@@ -48,6 +48,11 @@ class CORE_EXPORT CSSVariableResolver {
 
   void ComputeRegisteredVariables();
 
+ protected:
+  // Called before looking up the value of some var()-reference to make it
+  // possible to apply animated properties during variable resolution.
+  virtual void ApplyAnimation(const AtomicString& name) {}
+
  private:
   struct Options {
     STACK_ALLOCATED();

third_party/blink/renderer/core/css/resolver/style_resolver.cc

@@ -61,6 +61,7 @@
 #include "third_party/blink/renderer/core/css/part_names.h"
 #include "third_party/blink/renderer/core/css/properties/css_property.h"
 #include "third_party/blink/renderer/core/css/resolver/animated_style_builder.h"
+#include "third_party/blink/renderer/core/css/resolver/css_variable_animator.h"
 #include "third_party/blink/renderer/core/css/resolver/css_variable_resolver.h"
 #include "third_party/blink/renderer/core/css/resolver/match_result.h"
 #include "third_party/blink/renderer/core/css/resolver/media_query_result.h"
@@ -1084,70 +1085,6 @@ void StyleResolver::CollectPseudoRulesForElement(
   }
 }
 
-static void ApplyAnimatedCustomProperties(StyleResolverState& state) {
-  if (!state.IsAnimatingCustomProperties()) {
-    return;
-  }
-  CSSAnimationUpdate& update = state.AnimationUpdate();
-  HashSet<PropertyHandle>& pending = state.AnimationPendingCustomProperties();
-  DCHECK(pending.IsEmpty());
-  for (const auto& interpolations :
-       {update.ActiveInterpolationsForCustomAnimations(),
-        update.ActiveInterpolationsForCustomTransitions()}) {
-    for (const auto& entry : interpolations) {
-      pending.insert(entry.key);
-    }
-  }
-  while (!pending.IsEmpty()) {
-    PropertyHandle property = *pending.begin();
-    CSSVariableResolver variable_resolver(state);
-    StyleResolver::ApplyAnimatedCustomProperty(state, variable_resolver,
-                                               property);
-    // The property must no longer be pending after applying it.
-    DCHECK_EQ(pending.find(property), pending.end());
-  }
-}
-
-static const ActiveInterpolations& ActiveInterpolationsForCustomProperty(
-    const StyleResolverState& state,
-    const PropertyHandle& property) {
-  // Interpolations will never be found in both animations_map and
-  // transitions_map. This condition is ensured by
-  // CSSAnimations::CalculateTransitionUpdateForProperty().
-  const ActiveInterpolationsMap& animations_map =
-      state.AnimationUpdate().ActiveInterpolationsForCustomAnimations();
-  const ActiveInterpolationsMap& transitions_map =
-      state.AnimationUpdate().ActiveInterpolationsForCustomTransitions();
-  const auto& animation = animations_map.find(property);
-  if (animation != animations_map.end()) {
-    DCHECK_EQ(transitions_map.find(property), transitions_map.end());
-    return animation->value;
-  }
-  const auto& transition = transitions_map.find(property);
-  DCHECK_NE(transition, transitions_map.end());
-  return transition->value;
-}
-
-void StyleResolver::ApplyAnimatedCustomProperty(
-    StyleResolverState& state,
-    CSSVariableResolver& variable_resolver,
-    const PropertyHandle& property) {
-  DCHECK(property.IsCSSCustomProperty());
-  DCHECK(state.AnimationPendingCustomProperties().Contains(property));
-  const ActiveInterpolations& interpolations =
-      ActiveInterpolationsForCustomProperty(state, property);
-  const Interpolation& interpolation = *interpolations.front();
-  if (interpolation.IsInvalidatableInterpolation()) {
-    CSSInterpolationTypesMap map(state.GetDocument().GetPropertyRegistry(),
-                                 state.GetDocument());
-    CSSInterpolationEnvironment environment(map, state, &variable_resolver);
-    InvalidatableInterpolation::ApplyStack(interpolations, environment);
-  } else {
-    ToTransitionInterpolation(interpolation).Apply(state);
-  }
-  state.AnimationPendingCustomProperties().erase(property);
-}
-
 bool StyleResolver::ApplyAnimatedStandardProperties(
     StyleResolverState& state,
     const Element* animating_element) {
@@ -1238,9 +1175,8 @@ template <CSSPropertyPriority priority>
 void StyleResolver::ApplyAnimatedStandardProperties(
     StyleResolverState& state,
     const ActiveInterpolationsMap& active_interpolations_map) {
-  static_assert(
-      priority != kResolveVariables,
-      "Use applyAnimatedCustomProperty() for custom property animations");
+  static_assert(priority != kResolveVariables,
+                "Use CSSVariableAnimator for custom property animations");
   // TODO(alancutter): Don't apply presentation attribute animations here,
   // they should instead apply in
   // SVGElement::CollectStyleForPresentationAttribute().
@@ -1744,8 +1680,9 @@ void StyleResolver::ApplyCustomProperties(StyleResolverState& state,
 
   CSSVariableResolver(state).ComputeRegisteredVariables();
 
-  if (apply_animations == kIncludeAnimations) {
-    ApplyAnimatedCustomProperties(state);
+  if (apply_animations == kIncludeAnimations &&
+      state.IsAnimatingCustomProperties()) {
+    CSSVariableAnimator(state).ApplyAll();
   }
 }
 

third_party/blink/renderer/core/css/resolver/style_resolver.h

@@ -52,7 +52,6 @@ class MatchResult;
 class RuleSet;
 class CSSPropertyValueSet;
 class StyleRuleUsageTracker;
-class CSSVariableResolver;
 
 enum RuleMatchingBehavior { kMatchAllRules, kMatchAllRulesExcludingSMIL };
 
@@ -136,10 +135,6 @@ class CORE_EXPORT StyleResolver final
   void SetRuleUsageTracker(StyleRuleUsageTracker*);
   void UpdateMediaType();
 
-  static void ApplyAnimatedCustomProperty(StyleResolverState&,
-                                          CSSVariableResolver&,
-                                          const PropertyHandle&);
-
   static bool HasAuthorBackground(const StyleResolverState&);
 
   void Trace(blink::Visitor*);

third_party/blink/renderer/core/css/resolver/style_resolver_state.h

@@ -125,14 +125,6 @@ class CORE_EXPORT StyleResolverState {
     is_animating_custom_properties_ = value;
   }
 
-  HashSet<PropertyHandle>& AnimationPendingCustomProperties() {
-    return animation_pending_custom_properties_;
-  }
-
-  const HashSet<PropertyHandle>& AnimationPendingCustomProperties() const {
-    return animation_pending_custom_properties_;
-  }
-
   void SetParentStyle(scoped_refptr<const ComputedStyle>);
   const ComputedStyle* ParentStyle() const { return parent_style_.get(); }
 
@@ -220,7 +212,6 @@ class CORE_EXPORT StyleResolverState {
   CSSAnimationUpdate animation_update_;
   bool is_animation_interpolation_map_ready_;
   bool is_animating_custom_properties_;
-  HashSet<PropertyHandle> animation_pending_custom_properties_;
 
   bool apply_property_to_regular_style_;
   bool apply_property_to_visited_link_style_;