Revert "[BlinkGenPropertyTrees] Fix ElementIsAnimatingChanged logic"
This reverts commit b0eab4a3e36612417196e6766d7a770f675169c4.
Reason for revert: Investigating flakiness https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vYjBlYWI0YTNlMzY2MTI0MTcxOTZlNjc2NmQ3YTc3MGY2NzUxNjljNAw
Original change's description:
> [BlinkGenPropertyTrees] Fix ElementIsAnimatingChanged logic
>
> As of https://crrev.com/2a64f7b5, there is no longer a single ElementId
> for an entire 'animating target' in cc. However ElementIsAnimatingChanged
> still assumes that it can lookup the different types of nodes
> (TransformNode, EffectNode) using a single ElementId. This caused both
> incorrect logic (code that should have been executed was not as the node
> could not be found) as well as DCHECK failures (again from the incorrect
> branch).
>
> This fix is essentially a hack, and it is important to note that we don't
> have a clear idea of what ElementIsAnimatingChanged is required for (i.e.
> what the consequences of it not working -outside of DCHECKs - are). The
> goal of this CL is just to restore the function to how it was working
> before https://crrev.com/2a64f7b5.
>
> Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
>
> Change-Id: I0d0eb0c9add83ff3c1b6d6f30858804aed300aac
> Bug: 912574
> Reviewed-on: https://chromium-review.googlesource.com/c/1372173
> Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
> Reviewed-by: Majid Valipour <majidvp@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#621198}
TBR=flackr@chromium.org,majidvp@chromium.org,smcgruer@chromium.org
Change-Id: I5448fe9ff246b984916b07fbf2adfdf90ca0d896
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 912574
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/c/1404158
Reviewed-by: Roman Sorokin <rsorokin@chromium.org>
Commit-Queue: Roman Sorokin <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621553}diff --git a/cc/animation/animation_host.cc b/cc/animation/animation_host.cc
index f5d6109..b78c578 100644
--- a/cc/animation/animation_host.cc
+++ b/cc/animation/animation_host.cc
@@ -158,17 +158,10 @@
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_id_map);
+ element_animations->ClearAffectedElementTypes();
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 dda8db9..01722e1 100644
--- a/cc/animation/element_animations.cc
+++ b/cc/animation/element_animations.cc
@@ -84,8 +84,7 @@
return properties;
}
-void ElementAnimations::ClearAffectedElementTypes(
- const PropertyToElementIdMap& element_id_map) {
+void ElementAnimations::ClearAffectedElementTypes() {
DCHECK(animation_host_);
TargetProperties disable_properties = GetPropertiesMaskForAnimationState();
@@ -97,7 +96,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_map, ElementListType::ACTIVE, disabled_state_mask,
+ element_id(), ElementListType::ACTIVE, disabled_state_mask,
disabled_state);
}
set_has_element_in_active_list(false);
@@ -105,7 +104,7 @@
if (has_element_in_pending_list() &&
animation_host()->mutator_host_client()) {
animation_host()->mutator_host_client()->ElementIsAnimatingChanged(
- element_id_map, ElementListType::PENDING, disabled_state_mask,
+ element_id(), ElementListType::PENDING, disabled_state_mask,
disabled_state);
}
set_has_element_in_pending_list(false);
@@ -350,17 +349,15 @@
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_map, ElementListType::ACTIVE, diff_active, active_state_);
+ element_id(), 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_map, ElementListType::PENDING, diff_pending, pending_state_);
+ element_id(), ElementListType::PENDING, diff_pending, pending_state_);
}
}
@@ -470,60 +467,6 @@
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 629379a..7c51dc7 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(const PropertyToElementIdMap& element_id_map);
+ void ClearAffectedElementTypes();
void ElementRegistered(ElementId element_id, ElementListType list_type);
void ElementUnregistered(ElementId element_id, ElementListType list_type);
@@ -164,15 +164,6 @@
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 a015f50..4199443 100644
--- a/cc/test/animation_timelines_test_common.cc
+++ b/cc/test/animation_timelines_test_common.cc
@@ -126,17 +126,18 @@
}
void TestHostClient::ElementIsAnimatingChanged(
- const PropertyToElementIdMap& element_id_map,
+ ElementId element_id,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) {
- for (const auto& it : element_id_map) {
- TestLayer* layer = FindTestLayer(it.second, list_type);
- if (!layer)
- continue;
+ TestLayer* layer = FindTestLayer(element_id, list_type);
+ if (!layer)
+ return;
- TargetProperty::Type target_property = it.first;
- int property = static_cast<int>(target_property);
+ for (int property = TargetProperty::FIRST_TARGET_PROPERTY;
+ property <= TargetProperty::LAST_TARGET_PROPERTY; ++property) {
+ TargetProperty::Type target_property =
+ static_cast<TargetProperty::Type>(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 b18a516..ff76f1d 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(const PropertyToElementIdMap& element_id_map,
+ void ElementIsAnimatingChanged(ElementId element_id,
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 9889b94..605b3723 100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -1718,12 +1718,12 @@
}
void LayerTreeHost::ElementIsAnimatingChanged(
- const PropertyToElementIdMap& element_id_map,
+ ElementId element_id,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) {
DCHECK_EQ(ElementListType::ACTIVE, list_type);
- property_trees()->ElementIsAnimatingChanged(mutator_host(), element_id_map,
+ property_trees()->ElementIsAnimatingChanged(mutator_host(), element_id,
list_type, mask, state, true);
}
diff --git a/cc/trees/layer_tree_host.h b/cc/trees/layer_tree_host.h
index b8489c3..59563f3 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(const PropertyToElementIdMap& element_id_map,
+ void ElementIsAnimatingChanged(ElementId element_id,
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 9c82ca5..318b967 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -5544,7 +5544,7 @@
}
void LayerTreeHostImpl::ElementIsAnimatingChanged(
- const PropertyToElementIdMap& element_id_map,
+ ElementId element_id,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state) {
@@ -5552,9 +5552,8 @@
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_map, list_type, mask, state, false))
+ if (tree && tree->property_trees()->ElementIsAnimatingChanged(
+ mutator_host(), element_id, 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 25aceca..11489c6 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(const PropertyToElementIdMap& element_id_map,
+ void ElementIsAnimatingChanged(ElementId element_id,
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 f070e67..d3a3e0e 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(
- const PropertyToElementIdMap& element_id_map,
+ ElementId element_id,
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 2b78d1a2..1e74c89 100644
--- a/cc/trees/property_tree.cc
+++ b/cc/trees/property_tree.cc
@@ -1837,7 +1837,7 @@
bool PropertyTrees::ElementIsAnimatingChanged(
const MutatorHost* mutator_host,
- const PropertyToElementIdMap& element_id_map,
+ ElementId element_id,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state,
@@ -1849,19 +1849,6 @@
!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 27281e0..dcf8cb9 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,
- const PropertyToElementIdMap& element_id_map,
+ ElementId element_id,
ElementListType list_type,
const PropertyAnimationState& mask,
const PropertyAnimationState& state,
diff --git a/cc/trees/target_property.h b/cc/trees/target_property.h
index f28649c..93fa839 100644
--- a/cc/trees/target_property.h
+++ b/cc/trees/target_property.h
@@ -7,8 +7,6 @@
#include <bitset>
-#include "base/containers/flat_map.h"
-
namespace cc {
static constexpr size_t kMaxTargetPropertyIndex = 32u;
@@ -34,13 +32,6 @@
// 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 dd76551..f0fb8fe 100644
--- a/third_party/blink/web_tests/TestExpectations
+++ b/third_party/blink/web_tests/TestExpectations
@@ -4333,6 +4333,8 @@
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 ]
