Reland "[BlinkGenPropertyTrees] Fix ElementIsAnimatingChanged logic"
This relands the commit b0eab4a3e36612417196e6766d7a770f675169c4 from
https://chromium-review.googlesource.com/c/chromium/src/+/1372173
The only change is to mark virtual/threaded/animations/skew-notsequential-compositor.html
as flaky; it has been decided that it is worth landing the CL anyway
and investigating the flakiness afterwards.
TBR=majidvp@chromium.org,pdr@chromium.org
Bug: 912574, 921105
Change-Id: I93dfc5db4056e3567f99254f28b1072a66c4f532
Reviewed-on: https://chromium-review.googlesource.com/c/1407385
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622123}diff --git a/cc/animation/animation_host.cc b/cc/animation/animation_host.cc
index b78c578..f5d6109 100644
--- a/cc/animation/animation_host.cc
+++ b/cc/animation/animation_host.cc
@@ -158,10 +158,17 @@
scoped_refptr<ElementAnimations> element_animations =
GetElementAnimationsForElementId(element_id);
DCHECK(element_animations);
+
+ // |ClearAffectedElementTypes| requires an ElementId map in order to update
+ // the property trees. Generating that map requires walking the keyframe
+ // effects, so we have to do it before removing this one.
+ PropertyToElementIdMap element_id_map =
+ element_animations->GetPropertyToElementIdMap();
+
element_animations->RemoveKeyframeEffect(keyframe_effect);
if (element_animations->IsEmpty()) {
- element_animations->ClearAffectedElementTypes();
+ element_animations->ClearAffectedElementTypes(element_id_map);
element_to_animations_map_.erase(element_animations->element_id());
element_animations->SetAnimationHost(nullptr);
}
diff --git a/cc/animation/element_animations.cc b/cc/animation/element_animations.cc
index 01722e1..dda8db9 100644
--- a/cc/animation/element_animations.cc
+++ b/cc/animation/element_animations.cc
@@ -84,7 +84,8 @@
return properties;
}
-void ElementAnimations::ClearAffectedElementTypes() {
+void ElementAnimations::ClearAffectedElementTypes(
+ const PropertyToElementIdMap& element_id_map) {
DCHECK(animation_host_);
TargetProperties disable_properties = GetPropertiesMaskForAnimationState();
@@ -96,7 +97,7 @@
// mutator_host_client() to be null.
if (has_element_in_active_list() && animation_host()->mutator_host_client()) {
animation_host()->mutator_host_client()->ElementIsAnimatingChanged(
- element_id(), ElementListType::ACTIVE, disabled_state_mask,
+ element_id_map, ElementListType::ACTIVE, disabled_state_mask,
disabled_state);
}
set_has_element_in_active_list(false);
@@ -104,7 +105,7 @@
if (has_element_in_pending_list() &&
animation_host()->mutator_host_client()) {
animation_host()->mutator_host_client()->ElementIsAnimatingChanged(
- element_id(), ElementListType::PENDING, disabled_state_mask,
+ element_id_map, ElementListType::PENDING, disabled_state_mask,
disabled_state);
}
set_has_element_in_pending_list(false);
@@ -349,15 +350,17 @@
DCHECK(pending_state_.IsValid());
DCHECK(active_state_.IsValid());
+ PropertyToElementIdMap element_id_map = GetPropertyToElementIdMap();
+
if (has_element_in_active_list() && prev_active != active_state_) {
PropertyAnimationState diff_active = prev_active ^ active_state_;
animation_host()->mutator_host_client()->ElementIsAnimatingChanged(
- element_id(), ElementListType::ACTIVE, diff_active, active_state_);
+ element_id_map, ElementListType::ACTIVE, diff_active, active_state_);
}
if (has_element_in_pending_list() && prev_pending != pending_state_) {
PropertyAnimationState diff_pending = prev_pending ^ pending_state_;
animation_host()->mutator_host_client()->ElementIsAnimatingChanged(
- element_id(), ElementListType::PENDING, diff_pending, pending_state_);
+ element_id_map, ElementListType::PENDING, diff_pending, pending_state_);
}
}
@@ -467,6 +470,60 @@
return gfx::ScrollOffset();
}
+PropertyToElementIdMap ElementAnimations::GetPropertyToElementIdMap() const {
+ // As noted in the header documentation, this method assumes that each
+ // property type maps to at most one ElementId. This is not conceptually true
+ // for cc/animations, but it is true for the current clients:
+ //
+ // * ui/ does not set per-keyframe-model ElementIds, so this map will be
+ // each property type mapping to the same ElementId (i.e. element_id()).
+ //
+ // * blink guarantees that any two keyframe models that it creates which
+ // target the same property on the same target will have the same ElementId.
+ //
+ // In order to make this as little of a footgun as possible for future-us,
+ // this method DCHECKs that the assumption holds.
+
+ std::vector<PropertyToElementIdMap::value_type> entries;
+ for (int property_index = TargetProperty::FIRST_TARGET_PROPERTY;
+ property_index <= TargetProperty::LAST_TARGET_PROPERTY;
+ ++property_index) {
+ TargetProperty::Type property =
+ static_cast<TargetProperty::Type>(property_index);
+ ElementId element_id_for_property;
+ for (auto& keyframe_effect : keyframe_effects_list_) {
+ KeyframeModel* model = keyframe_effect.GetKeyframeModel(property);
+ if (model) {
+ // We deliberately use two branches here so that the DCHECK can
+ // differentiate between models with different element ids, and the case
+ // where some models don't have an element id.
+ // TODO(crbug.com/900241): All KeyframeModels should have an ElementId.
+ if (model->element_id()) {
+ DCHECK(!element_id_for_property ||
+ element_id_for_property == model->element_id())
+ << "Different KeyframeModels for the same target must have the "
+ << "same ElementId";
+ element_id_for_property = model->element_id();
+ } else {
+ // This DCHECK isn't perfect; you could have a case where one model
+ // has an ElementId and the other doesn't, but model->element_id() ==
+ // this->element_id() and so the DCHECK passes. That is unlikely
+ // enough that we don't bother guarding against it specifically.
+ DCHECK(!element_id_for_property ||
+ element_id_for_property == element_id())
+ << "Either all models should have an ElementId or none should";
+ element_id_for_property = element_id();
+ }
+ }
+ }
+
+ if (element_id_for_property)
+ entries.emplace_back(property, element_id_for_property);
+ }
+
+ return PropertyToElementIdMap(std::move(entries));
+}
+
bool ElementAnimations::KeyframeModelAffectsActiveElements(
KeyframeModel* keyframe_model) const {
// When we force a keyframe_model update due to a notification, we do not have
diff --git a/cc/animation/element_animations.h b/cc/animation/element_animations.h
index 7c51dc7..629379a 100644
--- a/cc/animation/element_animations.h
+++ b/cc/animation/element_animations.h
@@ -55,7 +55,7 @@
void SetAnimationHost(AnimationHost* host);
void InitAffectedElementTypes();
- void ClearAffectedElementTypes();
+ void ClearAffectedElementTypes(const PropertyToElementIdMap& element_id_map);
void ElementRegistered(ElementId element_id, ElementListType list_type);
void ElementUnregistered(ElementId element_id, ElementListType list_type);
@@ -164,6 +164,15 @@
gfx::ScrollOffset ScrollOffsetForAnimation() const;
+ // Returns a map of target property to the ElementId for that property, for
+ // KeyframeEffects associated with this ElementAnimations.
+ //
+ // This method makes the assumption that a given target property doesn't map
+ // to more than one ElementId. While conceptually this isn't true for
+ // cc/animations, it is true for the two current clients (ui/ and blink) and
+ // this is required to let BGPT ship (see http://crbug.com/912574).
+ PropertyToElementIdMap GetPropertyToElementIdMap() const;
+
private:
friend class base::RefCounted<ElementAnimations>;
diff --git a/cc/test/animation_timelines_test_common.cc b/cc/test/animation_timelines_test_common.cc
index 4199443..a015f50 100644
--- a/cc/test/animation_timelines_test_common.cc
+++ b/cc/test/animation_timelines_test_common.cc
@@ -126,18 +126,17 @@
}
void TestHostClient::ElementIsAnimatingChanged(
- ElementId element_id,
+ const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) {
- TestLayer* layer = FindTestLayer(element_id, list_type);
- if (!layer)
- return;
+ for (const auto& it : element_id_map) {
+ TestLayer* layer = FindTestLayer(it.second, list_type);
+ if (!layer)
+ continue;
- for (int property = TargetProperty::FIRST_TARGET_PROPERTY;
- property <= TargetProperty::LAST_TARGET_PROPERTY; ++property) {
- TargetProperty::Type target_property =
- static_cast<TargetProperty::Type>(property);
+ TargetProperty::Type target_property = it.first;
+ int property = static_cast<int>(target_property);
if (mask.potentially_animating[property])
layer->set_has_potential_animation(target_property,
state.potentially_animating[property]);
diff --git a/cc/test/animation_timelines_test_common.h b/cc/test/animation_timelines_test_common.h
index ff76f1d..b18a516 100644
--- a/cc/test/animation_timelines_test_common.h
+++ b/cc/test/animation_timelines_test_common.h
@@ -119,7 +119,7 @@
ElementListType list_type,
const gfx::ScrollOffset& scroll_offset) override;
- void ElementIsAnimatingChanged(ElementId element_id,
+ void ElementIsAnimatingChanged(const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) override;
diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index 0197be96..a70c34a 100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -1733,12 +1733,12 @@
}
void LayerTreeHost::ElementIsAnimatingChanged(
- ElementId element_id,
+ const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) {
DCHECK_EQ(ElementListType::ACTIVE, list_type);
- property_trees()->ElementIsAnimatingChanged(mutator_host(), element_id,
+ property_trees()->ElementIsAnimatingChanged(mutator_host(), element_id_map,
list_type, mask, state, true);
}
diff --git a/cc/trees/layer_tree_host.h b/cc/trees/layer_tree_host.h
index 8b0ae028..96907643 100644
--- a/cc/trees/layer_tree_host.h
+++ b/cc/trees/layer_tree_host.h
@@ -638,7 +638,7 @@
ElementListType list_type,
const gfx::ScrollOffset& scroll_offset) override;
- void ElementIsAnimatingChanged(ElementId element_id,
+ void ElementIsAnimatingChanged(const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) override;
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index 91a7d3d..352f458d 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -5569,7 +5569,7 @@
}
void LayerTreeHostImpl::ElementIsAnimatingChanged(
- ElementId element_id,
+ const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) {
@@ -5577,8 +5577,9 @@
list_type == ElementListType::ACTIVE ? active_tree() : pending_tree();
// TODO(wkorman): Explore enabling DCHECK in ElementIsAnimatingChanged()
// below. Currently enabling causes batch of unit test failures.
- if (tree && tree->property_trees()->ElementIsAnimatingChanged(
- mutator_host(), element_id, list_type, mask, state, false))
+ if (tree &&
+ tree->property_trees()->ElementIsAnimatingChanged(
+ mutator_host(), element_id_map, list_type, mask, state, false))
tree->set_needs_update_draw_properties();
}
diff --git a/cc/trees/layer_tree_host_impl.h b/cc/trees/layer_tree_host_impl.h
index b8b2af7..77f8763 100644
--- a/cc/trees/layer_tree_host_impl.h
+++ b/cc/trees/layer_tree_host_impl.h
@@ -354,7 +354,7 @@
ElementId element_id,
ElementListType list_type,
const gfx::ScrollOffset& scroll_offset) override;
- void ElementIsAnimatingChanged(ElementId element_id,
+ void ElementIsAnimatingChanged(const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) override;
diff --git a/cc/trees/mutator_host_client.h b/cc/trees/mutator_host_client.h
index d3a3e0e..f070e67 100644
--- a/cc/trees/mutator_host_client.h
+++ b/cc/trees/mutator_host_client.h
@@ -44,7 +44,7 @@
// Allows to change IsAnimating value for a set of properties.
virtual void ElementIsAnimatingChanged(
- ElementId element_id,
+ const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) = 0;
diff --git a/cc/trees/property_tree.cc b/cc/trees/property_tree.cc
index 1e74c89..2b78d1a2 100644
--- a/cc/trees/property_tree.cc
+++ b/cc/trees/property_tree.cc
@@ -1837,7 +1837,7 @@
bool PropertyTrees::ElementIsAnimatingChanged(
const MutatorHost* mutator_host,
- ElementId element_id,
+ const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state,
@@ -1849,6 +1849,19 @@
!mask.potentially_animating[property])
continue;
+ // The mask represents which properties have had their state changed. This
+ // can include properties for which there are no longer any animations, in
+ // which case there will not be an entry in the map.
+ //
+ // It is unclear whether this is desirable; it may be that we are missing
+ // updates to property nodes here because we no longer have the required
+ // ElementId to look them up. See http://crbug.com/912574 for context around
+ // why this code was rewritten.
+ auto it = element_id_map.find(static_cast<TargetProperty::Type>(property));
+ if (it == element_id_map.end())
+ continue;
+
+ const ElementId element_id = it->second;
switch (property) {
case TargetProperty::TRANSFORM:
if (TransformNode* transform_node =
diff --git a/cc/trees/property_tree.h b/cc/trees/property_tree.h
index dcf8cb9..27281e0 100644
--- a/cc/trees/property_tree.h
+++ b/cc/trees/property_tree.h
@@ -657,7 +657,7 @@
// this property tree. Returns whether a draw property update is
// needed.
bool ElementIsAnimatingChanged(const MutatorHost* mutator_host,
- ElementId element_id,
+ const PropertyToElementIdMap& element_id_map,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state,
diff --git a/cc/trees/target_property.h b/cc/trees/target_property.h
index 93fa839..f28649c 100644
--- a/cc/trees/target_property.h
+++ b/cc/trees/target_property.h
@@ -7,6 +7,8 @@
#include <bitset>
+#include "base/containers/flat_map.h"
+
namespace cc {
static constexpr size_t kMaxTargetPropertyIndex = 32u;
@@ -32,6 +34,13 @@
// A set of target properties.
using TargetProperties = std::bitset<kMaxTargetPropertyIndex>;
+// A map of target property to ElementId.
+// flat_map was chosen because there are expected to be relatively few entries
+// in the map. For low number of entries, flat_map is known to perform better
+// than other map implementations.
+struct ElementId;
+using PropertyToElementIdMap = base::flat_map<TargetProperty::Type, ElementId>;
+
} // namespace cc
#endif // CC_TREES_TARGET_PROPERTY_H_
diff --git a/third_party/blink/web_tests/TestExpectations b/third_party/blink/web_tests/TestExpectations
index 6ac2cca..f4e38a4 100644
--- a/third_party/blink/web_tests/TestExpectations
+++ b/third_party/blink/web_tests/TestExpectations
@@ -393,6 +393,8 @@
crbug.com/836886 virtual/prefer_compositing_to_lcd_text/compositing/overflow/scaled-overflow.html [ Failure ]
crbug.com/836886 compositing/overflow/scaled-overflow.html [ Failure ]
crbug.com/836886 compositing/scrollbars/nested-overlay-scrollbars.html [ Failure ]
+# Flaky subpixel AA difference (not necessarily incorrect, but flaky)
+crbug.com/921105 virtual/threaded/animations/skew-notsequential-compositor.html [ Failure Pass ]
# ====== Paint team owned tests to here ======
@@ -4237,8 +4239,6 @@
crbug.com/912362 external/wpt/web-animations/timing-model/timelines/timelines.html [ Failure ]
-crbug.com/912574 virtual/threaded/animations/responsive/transform-responsive-neutral-keyframe.html [ Crash ]
-
# Decoding test timeout on Win7. Marked flaky to get imported in case it's flaky timeout everywhere.
crbug.com/862938 external/wpt/encoding/textdecoder-fatal-single-byte.any.worker.html [ Pass Timeout ]
